Reiterate layout step until section addresses converge #688
Conversation
parth-07
force-pushed
the
SectionAddressConvergence
branch
5 times, most recently
from
81a1a0b to
7b61c10
Compare
parth-07
force-pushed
the
SectionAddressConvergence
branch
from
7b61c10 to
2d6821f
Compare
partaror
force-pushed
the
SectionAddressConvergence
branch
from
2d6821f to
93a16f2
Compare
quic-seaswara
left a comment
quic-seaswara
left a comment
There was a problem hiding this comment.
Why are you exposing maxPreRelaxationPasses to the function ?
I feel createProgramHeaders is becoming overly iterative.
The logic can be contained in GNULDBackend.
We need to use
To push the logic in GNULDBackend, we will need to refactor |
parth-07
force-pushed
the
SectionAddressConvergence
branch
from
93a16f2 to
42473f8
Compare
parth-07
force-pushed
the
SectionAddressConvergence
branch
from
42473f8 to
485a38d
Compare
| while (out != outEnd) { | ||
| bool useSetLMA = false; | ||
| int preRelaxPassIter = 0; | ||
| do { |
There was a problem hiding this comment.
This is a massive loop that requires indenting everything and makes the diff really large. I wonder if this can be avoided somehow? Can we isolate the single-pass into a new function and then have a small loop in relax() or somewhere else?
There was a problem hiding this comment.
Can we isolate the single-pass into a new function and then have a small loop in relax() or somewhere else?
That will also result in a huge diff, same as it is right now.
I wonder if this can be avoided somehow?
We can keep the indentation of the original loop as before, but still enclose it within do .. while(), however, then the indentation would be incorrect and reading the code in future might be misleading in the future due to incorrect indentation.
There was a problem hiding this comment.
I was thinking to first refactor the single-pass in a separate patch, and then this PR just becomes adding a loop
If refactoring is not an option, can the loop be a while() loop instead of a do...while? Since the function is massive it's not obvious what condition we are looping over until we find the end of the loop many lines down.
Regardless of whatever decision is taken here I think the two .hpp files could benefit from refactoring somewhere down the line.
| GNUVerDefFragment *GNUVerDefFrag = nullptr; | ||
| std::unordered_map<const ResolveInfo *, uint16_t> OutputVersionIDs; | ||
| #endif | ||
| OutputSectionEntry * changedOutputSection = nullptr; |
There was a problem hiding this comment.
Layout iterations only continue when section addresses change, but what about symbol-only forward references?
cat > 1.c << \!
int foo() {return 0;}
!
cat > cycle.t << \!
SECTIONS {
. = 0x1000;
.text : {*(.text*)}
a = b + 4;
b = a + 4;
}
!
cat > sym.t << \!
PHDRS {TEXT PT_LOAD;}
SECTIONS {
. = 0x1000;
.text : {*(.text*)} : TEXT
a = b + 4;
b = c + 4;
c = 0x2000;
}
!
clang -c 1.c
ld.eld 1.o -T sym.t # a,b,c = 0x8, 0x2004, 0x2000
ld.lld 1.o -T sym.t # a,b,c = 0x2008, 0x2004, 0x2000
ld.lld 1.o -T cycle.t # error: assignment to symbol a does not converge
ld.eld 1.o -T cycle.t # a = 0x14; b=0x18There was a problem hiding this comment.
Layout iterations only continue when section addresses change, but what about symbol-only forward references?
I plan to handle symbol address convergence in the next patch. The subsequent patches will add support for symbol address convergence, improved diagnostics related to layout iterations and convergence, and some heuristics to reduce the number of iterations wherever possible.
This commit modifies the layout step to reiterate until the section addresses converge. The reiteration has an upper limit of 4. On reaching this limit, a note is emitted and the layout reiteration is stopped. It is not warning as of now because it can cause link failure when --fatal-warnings is used. This note will later be converted to a warning once the feature is complete. Please note that this reiteration limit is for layout iterations before relaxations take place. After a layout relaxation pass, the reiteration count is reset to 0. Resolves qualcomm#687 Signed-off-by: Parth Arora <partaror@qti.qualcomm.com>
parth-07
force-pushed
the
SectionAddressConvergence
branch
from
485a38d to
27b20eb
Compare
| }; | ||
|
|
||
| bool linkerScriptHasMemoryCommand = m_Module.getScript().hasMemoryCommand(); | ||
| auto reset_state = [&](OutputSectionEntry *OS = nullptr) { |
There was a problem hiding this comment.
we reuse the segment table across passes; segments from a transient layout can stick around:
In the first pass we create a new PT_LOAD for .bar due to the forward reference that remains even after v is fixed:
cat > 1.c << \!
[[gnu::section(".foo")]] int foo = 1;
[[gnu::section(".bar")]] int bar = 2;
!
cat > fwd.t << \!
SECTIONS {
. = u;
.foo : {*(.foo)}
. = v;
.bar : {*(.bar)}
u = 0x1000;
v = u + 0x10;
}
!
cat > nofwd.t << \!
SECTIONS {
. = 0x1000;
.foo : {*(.foo)}
. = 0x1010;
.bar : {*(.bar)}
}
!
clang -c 1.c
ld.eld 1.o -T fwd.t -o fwd # two PT_LOADs with .foo and .bar separate.
ld.eld 1.o -T nofwd.t -o nofwd # single PT_LOAD with .foo and .bar together.
ld.lld 1.o -T fwd.t -o fwd
ld.lld 1.o -T fwd.t -o nofwd
llvm-readelf -l fwd nofwd| #endif | ||
| OutputSectionEntry * changedOutputSection = nullptr; | ||
| const int maxPreRelaxationPasses = 4; | ||
| }; |
There was a problem hiding this comment.
It looks like lld's max passes is 5. Is there any reason we picked a different number?
| DIAG(warn_empty_segment, DiagnosticEngine::Warning, | ||
| "Empty segment: '%0'") | ||
| "Empty segment: '%0'") | ||
| DIAG(note_section_address_not_converging, DiagnosticEngine::Note, |
There was a problem hiding this comment.
should this be a warning instead?
| hasError = true; | ||
| } | ||
| setVMA(*cur, vma, /*ignoreChangedSection=*/false); | ||
| cur->setAddr(vma); |
There was a problem hiding this comment.
do we need both setVMA and cur->setAddr()?
3 participants
This commit modifies the layout step to reiterate until the section addresses converge. The reiteration has an upper limit of 4. On reaching this limit, a warning is emitted and the layout reiteration is stopped.
Please note that this reiteration limit is for layout iterations before relaxations take place. After a layout relaxation pass, the reiteration count is reset to 0.
Resolves #687