-
Notifications
You must be signed in to change notification settings - Fork 47
Fix SUBALIGN linker script feature #752
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
base: main
Are you sure you want to change the base?
Conversation
| ELFSection *InSect = R->getSection(); | ||
| for (Fragment *F : InSect->getFragmentList()) { | ||
| ELFSection *owningSect = F->getOwningSection(); | ||
| if (owningSect && seen.insert(owningSect).second) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
early return if either of these are false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no statement after this if-command, and thus, I am not sure if early return would be beneficial here.
f2715a1 to
23fed03
Compare
| optSubAlign = Prolog.subAlign().result(); | ||
| } | ||
|
|
||
| for (RuleContainer *R : *O) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!Prolog.hasSubAlign())
continue;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot skip output sections that do not have SUBALIGN. The way we define SUBALIGN functionality in eld is: subalign changes the alignment of the fragments in the output sections that are the first fragments seen from their corresponding input sections. in the output section table.
So, if a plugin moves fragments of a section into two output sections, we need to determine which of them appears first in the output section table, and thus we need to go through all the output sections and fragments.
| InSect->setAddrAlign( | ||
| std::max(static_cast<uint64_t>(InSect->getAddrAlign()), | ||
| optSubAlign.value())); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this at the time of calling mergeSections.
I would think we would implement by first
- inspecting if the output section has subalign
- update rule container with the maximum alignment of the section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want also want to make sure the map file is annotated with this information for debugging reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this at the time of calling mergeSections.
merging input sections handles entirely different responsibility than handling subAlign. Why should we do this together with merging input sections? I would like to follow the unix philosophy, "do one thing well", wherever possible.
You might want also want to make sure the map file is annotated with this information for debugging reasons.
I will add map-file improvements as a follow-up patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging input sections already takes care of choosing the maximum alignment, and SUBALIGN is just one additional kind for choosing maximum alignment.
I am very concerned about link time.
- The usage of subalign is very rare and we are iterating through all the output sections for rare use-cases.
- We can mention as undefined behavior for now if users use SUBALIGN with linker plugin chunk movements.
Independent of the current approach, this patch traverses through every output section (allocatable, non allocatable sections), which can be improved too.
Overall comment : If you are covering some use-case, please add tests for all use cases.
For example
- DISCARD sections
- Emit relocs
- Empty sections with SUBALIGN, Sections with explicit data - BYTE/QUAD/...
- Partial linking
- Shared libraries
- Non allocatable sections (probably we should warn here and not do anything)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like map file improvements need to go in the same patch just because it would be a way to check of what the linker did explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging input sections already takes care of choosing the maximum alignment, and SUBALIGN is just one additional kind for choosing maximum alignment.
Merging input sections only sets the alignment of the rule-container internal input section, it is not responsible of reading linker script directives to change the alignments and it does not change the alignment of fragments / owning input sections.
I am very concerned about link time.
The link time would be the same whether we apply subalign alignments as part of the merging input sections, or
as a separate step after it. This is because the number of operations remains the same.
The usage of subalign is very rare and we are iterating through all the output sections for rare use-cases.
It is one pass over the fragments. We have many such passes over the fragments. I do not expect to see any noticeable
time difference. Do you think it would be better to go over the output sections once and check whether there is any output section with subalign, and only traverse fragments if there is indeed a use of subalign in the link?
We can change the code to only traverse fragments of output sections with SUBALIGN but that will slightly change the meaning of SUBALIGN for us. Please let me know if that would be fine.
We can mention as undefined behavior for now if users use SUBALIGN with linker plugin chunk movements.
Disabling plugin chunk movements depending upon whether SUBALIGN is used or not seems to be a very big restriction to me, and I believe that we should avoid it if possible. Additionally, even without the SUBALIGN, if plugins move the fragments incorrectly, then the image will be invalid and we do not report any warning / error for it.
Independent of the current approach, this patch traverses through every output section (allocatable, non allocatable sections), which can be improved too.
Overall comment : If you are covering some use-case, please add tests for all use cases.
I will add more tests, but I think as a functionality-wise, SUBALIGN should work for all the output sections. Other similar linker script functionality such as output section alignment can be specified for all the outputs sections as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that is what I am trying to say do this only for output sections with SUBALIGN.
Restrict plugins to not move chunks between output sections with SUBALIGN. This is totally ok for me. We can relax this case if we have a genuine requirement.
It does not make sense to use SUBALIGN on sections such as debug. We should warn on those usecases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that is what I am trying to say do this only for output sections with SUBALIGN.
I have changed the code to iterate input sections of only output sections that have SUBALIGN.
Restrict plugins to not move chunks between output sections with SUBALIGN. This is totally ok for me. We can relax this case if we have a genuine requirement.
The issue of incorrectly moving chunks of the same input section is not limited to SUBALIGN usage. If a plugin moves the chunk of the same input section to different output sections, then the image would be wrong even with no SUBALIGN usage.
It does not make sense to use SUBALIGN on sections such as debug. We should warn on those usecases.
Can we add handle such diagnostic improvements separately? Also, I believe that if there is no harm on using SUBALIGN for non-alloc sections, then we do not need to report a warning for it. We do not report any warning for using ALIGN on non-alloc sections as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example
DISCARD sections
Emit relocs
Empty sections with SUBALIGN, Sections with explicit data - BYTE/QUAD/...
Partial linking
Shared libraries
Non allocatable sections
I have added tests for all these cases. I am not currently warning for non-allocatable sections because we also do not warn if ALIGN linker script directive or the output section address linker script directive is used on non-allocatable sections.
I have added a test and a diagnostic for when SUBALIGN reduces an input section alignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we dont do this, but its an area that we can make the user understand what he might be doing or improve his build.
We should try and improve, but I agree this can be a follow up too.
lib/Object/ObjectLinker.cpp
Outdated
| ELFSection *owningSect = F->getOwningSection(); | ||
| if (owningSect && seen.insert(owningSect).second && optSubAlign) { | ||
| F->setAlignment(optSubAlign.value()); | ||
| InSect->setAddrAlign( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we emit a warning for sections that bumping subalign can cause issues
Examples of them may be dynamic sections or eh frame sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not aware of cases where making alignment larger can cause issues. Why would increasing alignment of .dynamic / .eh_frame sections cause issues?
I think we should emit warning if subalign is used to decrease the alignment of an input section.
quic-seaswara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed a bit thorougly.
| DIAG(warn_non_power_of_2_value_to_align_builtin, DiagnosticEngine::Warning, | ||
| "%0: non-power-of-2 value 0x%1 passed to ALIGN builtin function") | ||
| DIAG(warn_subalign_less_than_section_alignment, DiagnosticEngine::Warning, | ||
| "SUBALIGN(0x%0) is less than section alignment (0x%1) for section '%2'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the input file as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion. I have improved this diagnostic.
| } | ||
|
|
||
| void ObjectLinker::applySubAlign() { | ||
| std::unordered_set<ELFSection *> seen; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable can moved to line 1329 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it has to be before the for (auto : sectionMap()) loop
| if (owningSect && seen.insert(owningSect).second) { | ||
| // Warn if SUBALIGN is reducing the section alignment | ||
| if (ThisConfig.showLinkerScriptWarnings() && F->alignment() > subAlign) { | ||
| ThisConfig.raise(Diag::warn_subalign_less_than_section_alignment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an error ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, as per GNU ld docs (https://sourceware.org/binutils/docs/ld/Forced-Input-Alignment.html), SUBALIGN is allowed to reduce the input section alignment.
| void ObjectLinker::applySubAlign() { | ||
| std::unordered_set<ELFSection *> seen; | ||
|
|
||
| for (auto *O : ThisModule->getScript().sectionMap()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow up : multithread by output section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot do this computation multi-threaded, otherwise the subalign behavior will become non-deterministic for the input sections which consist of multiple fragments.
| InSect->setAddrAlign( | ||
| std::max(static_cast<uint64_t>(InSect->getAddrAlign()), | ||
| optSubAlign.value())); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we dont do this, but its an area that we can make the user understand what he might be doing or improve his build.
We should try and improve, but I agree this can be a follow up too.
| for (Fragment *F : inSect->getFragmentList()) { | ||
| ELFSection *owningSect = F->getOwningSection(); | ||
| if (owningSect->getKind() == LDFileFormat::Kind::OutputSectData) { | ||
| continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome!
Should we do this for internal sections ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to take this as case-by-case basis. Most internal sections do indeed represent a section and thus SUBALIGN should affect them. OutputSectData is an exception because it is not a section at all as per GNU ld docs.
a303120 to
a8ac9c6
Compare
ba5f13e to
a8ac9c6
Compare
This commit fixes the SUBALIGN linker script feature. SUBALIGN feature is used to modify the alignment of all input sections present in an output section. Until now, we were only changing the alignment of the first fragment of each rule of the output sections with SUBALIGN specified. This commit fixes the SUBALIGN behavior. SUBALIGN affects the alignment of the first fragment as is seen by the linker when traversing the output sections for the input sections that may contain more than 1 fragment such as .eh_frame, PLT / GOT sections, and .comment section. Resolves qualcomm#343 Signed-off-by: Parth Arora <[email protected]>
| DIAG(warn_non_power_of_2_value_to_align_builtin, DiagnosticEngine::Warning, | ||
| "%0: non-power-of-2 value 0x%1 passed to ALIGN builtin function") | ||
| DIAG(warn_subalign_less_than_section_alignment, DiagnosticEngine::Warning, | ||
| "SUBALIGN(0x%0) is less than the section alignment (0x%1) for section '%2(%3)'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use ELFSection::getLocation for diagnostics for sections? it will conveniently print the input file too, and section offset in hex if any
This commit fixes the SUBALIGN linker script feature. SUBALIGN feature is used to modify the alignment of all input sections present in an output section. Until now, we were only changing the alignment of the first fragment of each rule of the output sections with SUBALIGN specified. This commit fixes the SUBALIGN behavior.
SUBALIGN affects the alignment of the first fragment as is seen by the linker when traversing the output sections for the input sections that may contain more than 1 fragment such as .eh_frame, PLT / GOT sections, and .comment section.
Resolves #343