[PATCH] D95199: [ELF] Write output sections in PT_LOAD segment order
Patrick Oppenlander via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 31 19:19:43 PST 2021
pattop added a comment.
Thanks for taking the time to review.
This causes problems any time the sections are not in segment order.
I agree that it would be good if lld's behaviours can more closely match BFD ld. https://lld.llvm.org/ even says that lld is supposed to be a drop in replacement for GNU linkers.
I've commented on most of the test case changes and will update the patch to address your review comments.
================
Comment at: lld/test/ELF/linkerscript/at8.test:23
# CHECK: Name Type Address Off
-# CHECK: .text PROGBITS 0000000008000000 000158
+# CHECK: .text PROGBITS 0000000008000000 001018
# CHECK: .sec1 PROGBITS 0000000020000000 001000
----------------
This change in file offest happens because .text is not part of a LOAD segment as it's empty. Is checking the offset of an empty section sensible? (this happens a few more times below).
Maybe, as a separate change, we should set the offset for empty sections to 0?
The rest of the offsets are not affected as .text is empty.
================
Comment at: lld/test/ELF/linkerscript/empty-sections-expressions.test:12
# CHECK-NEXT: NULL 0000000000000000 000000 000000
-# CHECK-NEXT: .empty PROGBITS 0000000000080000 000158 000000
+# CHECK-NEXT: .empty PROGBITS 0000000000080000 001002 000000
# CHECK-NEXT: .text PROGBITS 0000000000080000 001000 000001
----------------
The same reasoning as at8.test applies.
================
Comment at: lld/test/ELF/linkerscript/implicit-program-header.test:10
# CHECK: Segment Sections...
-# CHECK-NEXT: 00 .dynsym .hash .dynstr .foo .text .dynamic
+# CHECK-NEXT: 00 .dynsym .hash .dynstr .text .dynamic
# CHECK-NEXT: 01 .foo
----------------
This new result aligns closely with what BFD ld 2.35.1 produces which is:
Section to Segment mapping:
Segment Sections...
00 .text .dynsym .dynstr .hash .dynamic
01 .foo
02 .foo
The SECTIONS command says that .bar should go into the ph_exec and ph_note segments. I think the expectation is that .foo should be the same as it does not specify a new segment.
Previously .foo was also included in the ph_write segment which I think is incorrect.
The segments lld produces for this test are a bit weird compared to BFD ld both before and after this change.
BFD ld:
Type Offset VirtAddr PhysAddr FileSiz MemSiz Flags Align
LOAD 0x0000000000001008 0x0000000000000008 0x0000000000000008 0x00000000000000e8 0x00000000000000e8 W 0x1000
LOAD 0x0000000000001000 0x0000000000000000 0x0000000000000000 0x0000000000000008 0x0000000000000008 E 0x1000
NOTE 0x0000000000001000 0x0000000000000000 0x0000000000000000 0x0000000000000008 0x0000000000000008 R E 0x1
lld without this patch:
Type Offset VirtAddr PhysAddr FileSiz MemSiz Flags Align
LOAD 0x0000000000001000 0x0000000000000000 0x0000000000000000 0x00000000000000a0 0x00000000000000a0 W 0x1000
LOAD 0x0000000000001029 0x0000000000000029 0x0000000000000029 0x0000000000000008 0x0000000000000008 E 0x1000
NOTE 0x0000000000001029 0x0000000000000029 0x0000000000000029 0x0000000000000008 0x0000000000000008 R E 0x1
lld with this patch:
Type Offset VirtAddr PhysAddr FileSiz MemSiz Flags Align
LOAD 0x0000000000001000 0x0000000000000000 0x0000000000000000 0x00000000000000a0 0x00000000000000a0 W 0x1000
LOAD 0x0000000000002029 0x0000000000000029 0x0000000000000029 0x0000000000000008 0x0000000000000008 E 0x1000
NOTE 0x0000000000002029 0x0000000000000029 0x0000000000000029 0x0000000000000008 0x0000000000000008 R E 0x1
With this change lld no longer produces overlapping load segments which I think is a step in the right direction.
================
Comment at: lld/test/ELF/linkerscript/nobits-offset.s:18
+# CHECK-NEXT: .text PROGBITS 0000000000000000 000400 000000
+# CHECK-NEXT: .sec1 NOBITS 0000000000000000 000400 000001
# CHECK-NEXT: .bss NOBITS 0000000000000400 000400 000001
----------------
The file offset of .text is not important as it is empty.
The file offset of .sec1 is not important as it has no file data.
================
Comment at: lld/test/ELF/linkerscript/tbss.s:38
# CHECK-NEXT: Address: 0x[[ADDR:.*]]
-# CHECK-NEXT: Offset: 0x[[ADDR]]
+# CHECK-NEXT: Offset: 0x[[OFFS]]
# CHECK-NEXT: Size: 4
----------------
As the comment states, what matters here is that a thread local bss section doesn't take up any address space. This effectively means that the location counter must not be advanced when processing such a section.
I don't think this change is correct. What's important is that the address of the section foo is the same as the address of the next section bar. That validates the assertion that no address space has been used.
I'm not sure why the file offset was considered in this test. It doesn't seem relevant?
The reason the file offsets change at all is because bar is now placed in the file first followed by .text and foo.
I will update this.
================
Comment at: lld/test/ELF/partition-synthetic-sections.s:195
// FILL-NEXT: *
-// FILL-NEXT: 005000
+// FILL-NEXT: 006000
----------------
This looks harmless to me as the output is shifted by exactly one page and can be explained by a slight reordering of sections in the output. Everything else is identical.
I haven't done a detailed analysis of why this moves. If there are concerns I can investigate further.
================
Comment at: lld/test/ELF/tls-offset.s:58
// CHECK2-NEXT: Address: 0x202010
-// CHECK2-NEXT: Offset: 0x2004
+// CHECK2-NEXT: Offset: 0x2008
// CHECK-NEXT: Size: 16
----------------
I don't think the file offsets here are important. .tbss is a NOBITS section and has no file data associated with it.
Somewhat concerningly the TLS segment changes size from 4 bytes to 8 byes (not covered by this test) which should probably be investigated.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95199/new/
https://reviews.llvm.org/D95199
More information about the llvm-commits
mailing list