Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

llvm-objcopy: Disable huge sec offset #49

Closed

Conversation

djtodoro
Copy link
Collaborator

@djtodoro djtodoro commented Sep 6, 2024

Match GNU objcopy's behavior.
This is controlled by an option.

Copy link
Member

@farazs-github farazs-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review comments

llvm/lib/ObjCopy/ELF/ELFObject.cpp Outdated Show resolved Hide resolved
llvm/include/llvm/ObjCopy/ELF/ELFConfig.h Outdated Show resolved Hide resolved
llvm/lib/ObjCopy/ELF/ELFObject.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjCopy/ELF/ELFObject.cpp Outdated Show resolved Hide resolved
@farazs-github farazs-github changed the base branch from mtk/nanomips-llvm16 to nanomips-llvm16 September 6, 2024 12:33
@djtodoro djtodoro force-pushed the syrmia/objcopy-disable-huge-sec-offset branch from fde7ae0 to b39e492 Compare September 7, 2024 11:15
@djtodoro
Copy link
Collaborator Author

djtodoro commented Sep 7, 2024

@farazs-github Thanks for the feedback!

@djtodoro djtodoro requested a review from cme September 9, 2024 08:12
@djtodoro
Copy link
Collaborator Author

I've gotten some comments on the upstream PR. I will apply them here when it is ready/merged there.

By default, GNU objcopy issues a warning for large offsets in ELF
files. To align with this behavior, we introduce an option to
define a maximum file offset for a section. This allows us to skip
creating excessively large files by specifying a threshold beyond
which the file offset will trigger an error.
@djtodoro djtodoro force-pushed the syrmia/objcopy-disable-huge-sec-offset branch from b39e492 to 1f3a427 Compare September 16, 2024 13:40
@djtodoro
Copy link
Collaborator Author

I have incorporated all comments from reviewers from the upstream PR. They are happy with implementation, but there is ongoing discussion about the option being accepted at first place.

@djtodoro
Copy link
Collaborator Author

Merged internally (review process also done on upstream - the implementation is OK, but there is a discussion if we even want this option in mainline).

@djtodoro djtodoro closed this Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants