feat: support --nmagic#1884
Conversation
davidlattimore
left a comment
davidlattimore
left a comment
There was a problem hiding this comment.
Did you come across some software that uses this flag? It's the sort of flag that if someone is using it in software that's actively maintained, they should probably be working to migrate off of it. In that regard, it wouldn't say this is an essential flag for us to implement. That said, I don't object to implementing it, so long as it doesn't add substantial complexity.
| output_section_id::PROGRAM_HEADERS, | ||
| output_section_id::SECTION_HEADERS, | ||
| output_section_id::SHSTRTAB, | ||
| output_section_id::RELRO_PADDING, |
There was a problem hiding this comment.
Why is RELRO_PADDING removed?
There was a problem hiding this comment.
The short answer is that the x64 binaries segfault if it's not removed. The long answer is that a few lines down, the segment gets conditionally kept if args.relro is set, but since it's force kept above, the relro check doesn't have any effect. Is this something that should be split into another PR?
There was a problem hiding this comment.
Ah, good catch. Yes, I think a separate PR for that would be good, that way we can make sure it's tested and list the change in the release notes.
| Ok(InputRecord::Object(object)) | ||
| if object.is_dynamic_object() && !input_ref.file.modifiers.allow_shared { | ||
| bail!( | ||
| "attempted static link of dynamic object {}", |
There was a problem hiding this comment.
This looks like it's adding some error detection that we were perhaps missing before. It could be good to do this as a separate PR, ideally with a test to detect this error scenario.
| if args.nmagic { | ||
| // PT_GNU_RELRO requires segments to start on page boundaries and cover an entire page | ||
| args.relro = false; | ||
| if args.max_page_size.is_some() { |
There was a problem hiding this comment.
Checking this here means that we'll only error if max-page-size is set before nmagic. Probably best to check for invalid flag combinations after parsing is complete.
| // GNU ld merges segment.LOAD.R and segment.LOAD.RX, which wild and lld do not | ||
| //#DiffIgnore: segment.LOAD.RX.alignment | ||
| //#DiffIgnore:segment.LOAD.RWX.alignment | ||
| // QEMU requires segments to be page-aligned |
There was a problem hiding this comment.
Given that the binaries are segfaulting when run in CI, it seems likely that the cause is the same reason QEMU was rejecting them - i.e. lack of page alignment. Perhaps see if you can fix the page alignment, then maybe both qemu and CI will be happy.
There was a problem hiding this comment.
Hmm. Given that --nmagic is there to disable page alignment, I'm not sure how best to test this...
There was a problem hiding this comment.
If unaligned segments are a characteristic of the flag, but the hardware (and emulators) that we run on don't support running with unaligned segments, then perhaps we need "RunEnabled:false", then add whatever assertions we can about the output file to convince ourselves that it's correct.
It's part of a lot of embedded projects for boards with unaligned flash memory (source). While this is far from the largest blocker to using wild in embedded projects, it's one I felt was realistically within my capabilities to contribute. |
2 participants
--nmagicdisables page alignment for loadable sections and prevents linking against shared libraries. Support for--omagicwill follow once this is cleaned up and merged.ld will merge the R and RX loadable segments with
--nmagic, whereas lld will not. I have chosen to match the LLD behaviour.I'm aware of two shortcomings so far in this PR: