Page MenuHomePhabricator

[ELF] Consider that NOLOAD sections should be placed in a PT_LOAD segment
ClosedPublic

Authored by kschwarz on Jun 7 2021, 7:48 AM.

Details

Summary

During PHDR creation, the case where an output section does not require a
PT_LOAD header but still occupies memory in the current VMA region was not handled.

If such an output section interleaves two output sections that have the same
VMA and LMA regions set, we would previously re-use the existing PT_LOAD header
for the second output section.
However, since the memory region is not contiguous, we need to start a new PT_LOAD
segment.

This fixes https://bugs.llvm.org/show_bug.cgi?id=50558

Diff Detail

Event Timeline

kschwarz created this revision.Jun 7 2021, 7:48 AM
kschwarz requested review of this revision.Jun 7 2021, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2021, 7:48 AM

During PHDR creation, the case where an output section does not require a PT_LOAD header but still occupies memory in the current VMA region was not handled.

I think our needsPtLoad may not match GNU ld. "does not require a PT_LOAD" may be incorrect.

The latest GNU ld code has a comment from
https://sourceware.org/git/?p=binutils-gdb.git;a=blobdiff;f=ld/ldlang.c;h=9038ebfff3b40298572b3eaf10eb0531e105d4fa;hp=798d7a1109712033d7764e2ef8e59351476d89b2;hb=f4eaaf7fceed097ed62542f9940986b1de01aa48;hpb=7cdfa31841aea9665682cadb03aaade2821721b1

The semantics appear to be different from what is documented here https://sourceware.org/binutils/docs/ld/Output-Section-Type.html

lld/test/ELF/linkerscript/phdrs-noload.test
37 ↗(On Diff #350290)

This style is too verbose. { can be at the line end.

If the input section descriptions are short, you may even consider place the whole thing on one line.

72 ↗(On Diff #350290)

No newline at end of file

MaskRay requested changes to this revision.Jun 7 2021, 12:58 PM
This revision now requires changes to proceed.Jun 7 2021, 12:58 PM
kschwarz updated this revision to Diff 350563.Jun 8 2021, 3:27 AM

Test formatting

kschwarz marked 2 inline comments as done.Jun 8 2021, 4:10 AM

Thanks for your comments! I've applied the suggested formatting changes.

Indeed the comment you referenced

[...] ELF gets a .bss style noload, alloc, no contents section. [...]

seems to disagree with the documentation, where a NOLOAD section can have contents.
I'm not too familiar with the GNU ld internals, so I'm not sure what the effect of clearing SEC_HAS_CONTENTS is.

What would be your suggestion how to proceed?

Remove the noload check from needsPtLoad

https://reviews.llvm.org/D45264 specifically introduced the check for sec->noload, stating that this conforms to GNU ld 2.29

I checked on my local system with GNU ld 2.34, and it does create PT_LOAD segments for NOLOAD output sections.
So, is the only effect of NOLOAD on an output section to mark it as SHT_NOBITS?

kschwarz updated this revision to Diff 350886.Jun 9 2021, 6:53 AM

Remove the check for noload, adapt existing tests

kschwarz added inline comments.Jun 9 2021, 7:52 AM
lld/test/ELF/linkerscript/nobits-offset.s
22–23

Independent from this change, GNU ld assigns WA section flags to output sections defined as

.sec1 (NOLOAD) : { . += 1; }

, while ld.lld assigns A. Is this intended?

This has also an effect on the number of program headers we create. In the test, the bss section has WA
flags, thus getting its own header.

lld/test/ELF/linkerscript/noload.s
13

GNU ld creates the following program headers:

LOAD           0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x001000 RW  0x1000
LOAD           0x000000 0x0000000000010000 0x0000000000010000 0x000000 0x001001 RW  0x1000
LOAD           0x001000 0x0000000000020000 0x0000000000020000 0x000001 0x000001 R E 0x1000

Should we create a new header whenever a section has an addrExpr set, similar to our condition for lmaExpr?

GNU ld will only create a new header if the current section is placed at a discontiguous address.

MaskRay added inline comments.Jun 10 2021, 5:45 PM
lld/test/ELF/linkerscript/nobits-offset.s
22–23

I think our behavior is intended.

The GNU ld behavior (I don't know what the current state is) was not ideal: https://sourceware.org/bugzilla/show_bug.cgi?id=26378#c8

MaskRay added inline comments.Jun 10 2021, 5:46 PM
lld/test/ELF/linkerscript/noload.s
13

Try merging PT_LOAD with same flags and contiguous address ranges.

Doing this rigidly can make the code more complex. In such cases we may trade the merged behavior for simpler implementation.

kschwarz added inline comments.Jun 14 2021, 3:36 AM
lld/test/ELF/linkerscript/noload.s
13

I had a look when/where the merging of PT_LOAD segments with contiguous address ranges should occur.
If no addrExprs are used, the VMAs will usually be contiguous and we can keep the current behavior, i.e. check whether the memregions are identical.

However if addrExprs are used, we cannot know whether two output sections will have contiguous address ranges until after the final layout is fixed. In that case, we would need a final "merge pass", maybe at the same time we remove empty PT_LOAD segments.

If you agree, I'd like to keep these changes separate and first commit this change (creating PT_LOAD headers for NOLOAD sections).

Creating a new PT_LOAD header for every section that uses an addrExpr breaks quite some tests in the test suite, so that definitely needs a closer look.

MaskRay accepted this revision.Jun 15 2021, 1:14 PM

LGTM, but the subject and description need to be updated.

We drop the special case that (NOLOAD) is not placed in a PT_LOAD. The new logic aligns with modern GNU ld.

If merging PT_LOAD will add more complexity to sameLMARegion, I'd suggest we don't do that.

This revision is now accepted and ready to land.Jun 15 2021, 1:14 PM
MaskRay retitled this revision from [LLD][ELF] Fix PT_LOAD program header creation for NO_LOAD sections to [ELF] Consider that NOLOAD sections should be placed in a PT_LOAD segment.Jun 15 2021, 1:15 PM

LGTM, but the subject and description need to be updated.

Thanks! I updated the commit message locally, but wasn't aware that arc land will fetch the existing description in Phabricator, so that got overwritten again.. sorry for that.

After modifying the commit description, use arc diff --verbatim --head=HEAD 'HEAD^' to update the Phabricator subject/summary.