Page MenuHomePhabricator

[ELF] Allow sections after a non-SHF_ALLOC section to be covered by PT_LOAD
ClosedPublic

Authored by MaskRay on Aug 2 2020, 1:29 PM.

Details

Summary

GNU ld allows sections after a non-SHF_ALLOC section to be covered by PT_LOAD
(PR37607) and assigns addresses to non-SHF_ALLOC output sections (similar to
SHF_ALLOC NOBITS sections. The location counter is not advanced).

This patch tries to fix PR37607 (remove a special case in
Writer<ELFT>::createPhdrs). To make the created PT_LOAD meaningful, we cannot
reset dot to 0 for a middle non-SHF_ALLOC output section. This results in
removal of two special cases in LinkerScript::assignOffsets. Non-SHF_ALLOC
non-orphan sections can have non-zero addresses like in GNU ld.

The zero address rule for non-SHF_ALLOC sections is weakened to apply to orphan
only. This results in a special case in createSection and findOrphanPos, respectively.

Diff Detail

Event Timeline

MaskRay created this revision.Aug 2 2020, 1:29 PM
MaskRay requested review of this revision.Aug 2 2020, 1:29 PM

I'm just trying to make sure I've got this straight. Current LLD behaviour is to ignore linker script positioning of non-allocatable sections and put them after all allocatable sections, and this changes to placing them in the output in their requested position. If a program header were to cover these sections as a result of covering the allocatable sections either side of them, the normally non-alloc sections are given addresses to maintain the address space. Makes sense to me.

Do we have/need a test case for non-alloc between two sections which are in different segments, e.g:

| PT_LOAD1 |           | PT_LOAD2 |
| alloc1   | non-alloc | alloc2   |

I'd expect non-alloc to not be covered by the segment, as it doesn't need to be (though I could be convinced it does need to be).

lld/ELF/Writer.cpp
1237

I think the term is "non-allocatable".

1238
lld/test/ELF/linkerscript/compress-debug-sections-custom.s
3
lld/test/ELF/linkerscript/symbols-non-alloc.test
2–3
MaskRay updated this revision to Diff 282724.Aug 3 2020, 2:20 PM
MaskRay marked an inline comment as done.

Current LLD behaviour is to ignore linker script positioning of non-allocatable sections and put them after all allocatable sections, and this changes to placing them in the output in their requested position

For PT_LOAD creation, current LLD behaviors ignores every section after a non-allocable section.
This changes them to be allocated into some PT_LOAD. The non-allocable placement is arbitrary: their observed behaviors are not guaranteed property.

Do we have/need a test case for non-alloc between two sections which are in different segments, e.g:

Added a test that .data .bss .comment .other can be in one PT_LOAD.
This is not something we strive to satisfy, just a result of current implementation choice.

lld/ELF/Writer.cpp
1237

gABI uses "non-allocable" in one place (SHF_COMPRESSED, contributed by Solaris folks)

SHF_COMPRESSED

This flag identifies a section containing compressed data. SHF_COMPRESSED applies only to non-allocable sections, and cannot be used in conjunction with SHF_ALLOC. In addition, SHF_COMPRESSED cannot be applied to sections of type SHT_NOBITS.

I don't find a use of "non-allocatable".

Somebody else should probably verify that the logic makes sense to them.

lld/ELF/Writer.cpp
1237

Hmmm... that's an unforutnate word-usage there in the spec. Thinking about it further: SHF_ALLOC is presumably short for some variation of the verb "allocate", whose equivalent adjectives are "allocated" (is assigned memory) and "allocatable" (can be allocated). The opposite of these would actually be "unallocated" (is not assigned memory) and "unallocatable" (cannot be allocated to memory). However, "cannot" is probably too strong a meaning in this context. I don't think "non-alloc[at]able" is really a valid English word.

Maybe we can dodge the issue and just use "non-alloc"? That's what I colloquially use for referring to sections that don't have SHF_ALLOC.

lld/test/ELF/linkerscript/symbols-non-alloc.test
2–3

(Regardless of the non-allocable conversation, the "to" still needs deleting from this comment).

MaskRay updated this revision to Diff 283130.Aug 4 2020, 10:59 PM
MaskRay marked 5 inline comments as done.
MaskRay retitled this revision from [ELF] Allow sections after a non-allocable section to be covered by PT_LOAD to [ELF] Allow sections after a non-SHF_ALLOC section to be covered by PT_LOAD.
MaskRay edited the summary of this revision. (Show Details)

non-allocable -> non-SHF_ALLOC
equals to -> equals

jhenderson accepted this revision.Aug 5 2020, 12:54 AM

Looks good from my point of view, but I'd appreciate another pair of eyes to confirm that the behaviour makes sense.

This revision is now accepted and ready to land.Aug 5 2020, 12:54 AM
psmith added a comment.Aug 5 2020, 8:19 AM

FWIW this looks good to me too.

lld/ELF/Writer.cpp
1237

I suggest rewording:

// To make the addresses of orphan sections not affected by orphan
// non-SHF_ALLOC sections in the middle, place orphan non-SHF_ALLOC sections
// at the end.

as

// OutputSections without the SHF_ALLOC flag are not part of the memory image and
// are given an address starting at 0. Place any orphan sections without the SHF_ALLOC
// flag at the end so that these do not affect the address assignment of OutputSections
// with the SHF_ALLOC flag.
MaskRay updated this revision to Diff 283274.Aug 5 2020, 9:27 AM

reword a comment

MaskRay marked an inline comment as done.Aug 5 2020, 9:29 AM
MaskRay added inline comments.
lld/ELF/Writer.cpp
1237

Thanks. I reworded the part "are given an address starting at 0" as we actually assign addresses to OutputSections without the SHF_ALLOC flag created from non-orphan sections.

MaskRay edited the summary of this revision. (Show Details)Aug 5 2020, 9:29 AM
This revision was landed with ongoing or failed builds.Aug 5 2020, 9:30 AM
This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.
omjavaid reopened this revision.Aug 6 2020, 4:33 AM
omjavaid added a subscriber: omjavaid.

FYI I have reverted this change as it was breaking multiple buildbots.

Example:
http://lab.llvm.org:8011/builders/lldb-aarch64-ubuntu
http://lab.llvm.org:8011/builders/lldb-arm-ubuntu

This revision is now accepted and ready to land.Aug 6 2020, 4:33 AM
omjavaid requested changes to this revision.Aug 6 2020, 4:33 AM
This revision now requires changes to proceed.Aug 6 2020, 4:33 AM
This revision was not accepted when it landed; it landed in state Needs Revision.Aug 6 2020, 8:27 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2020, 8:27 AM