Page MenuHomePhabricator

[LLD][ELF] - Don't keep empty output sections which have explicit program headers
ClosedPublic

Authored by grimar on Nov 30 2020, 1:23 AM.

Details

Summary

This reverts a side effect introduced in the code cleanup patch D43571:
LLD started to emit empty output sections that are explicitly assigned to a segment.

This patch fixes the issue by removing the !sec.phdrs.empty() special case from
isDiscardable. As compensation, we add an early phdrs propagation step (see the inline comment).
This is similar to one that we do in adjustSectionsAfterSorting.

Diff Detail

Event Timeline

grimar created this revision.Nov 30 2020, 1:23 AM
grimar requested review of this revision.Nov 30 2020, 1:23 AM

It makes me happy that doing this doesn't seem to be all that complex. Thanks for looking into it!

lld/ELF/LinkerScript.cpp
990

Nit: delete extra blank line too.

1016
1082
1083
lld/test/ELF/linkerscript/implicit-program-header.test
10

Unrelated to this patch, but it looks incorrect that .foo is assigned to this program header? It should be in ph_exec, and only ph_exec, by my understanding.

19

Is it worth extending this test case to show that ALL program headers are propagated, e.g. if .bar was inside : ph_exec : ph_other then .foo is too?

grimar updated this revision to Diff 308318.Nov 30 2020, 4:11 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
lld/test/ELF/linkerscript/implicit-program-header.test
10

This happens because .foo is placed between .dynsym and .dynamic which belongs to "W" segment:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg L
k Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00
0   0  0
  [ 1] .dynsym           DYNSYM          0000000000000000 001000 000018 18   A
3   1  8
  [ 2] .hash             HASH            0000000000000018 001018 000010 04   A
1   0  4
  [ 3] .dynstr           STRTAB          0000000000000028 001028 000001 00   A
0   0  1
  [ 4] .foo              PROGBITS        0000000000000029 001029 000008 00  AX
0   0  1
  [ 5] .text             PROGBITS        0000000000000034 001034 000008 00  AX
0   0  4
  [ 6] .dynamic          DYNAMIC         0000000000000040 001040 000060 10  WA
3   0  8
  [ 7] .comment          PROGBITS        0000000000000000 0010a0 000008 01  MS
0   0  1
  [ 8] .symtab           SYMTAB          0000000000000000 0010a8 000030 18     1
0   2  8
  [ 9] .shstrtab         STRTAB          0000000000000000 0010d8 00004e 00
0   0  1
  [10] .strtab           STRTAB          0000000000000000 001126 00000a 00
0   0  1

Program Headers:
  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz
  Flg Align
  LOAD           0x001000 0x0000000000000000 0x0000000000000000 0x0000a0 0x0000a
0  W  0x1000
  LOAD           0x001029 0x0000000000000029 0x0000000000000029 0x000008 0x00000
8   E 0x1000

 Section to Segment mapping:
  Segment Sections...
   00     .dynsym .hash .dynstr .foo .text .dynamic
   01     .foo

The output might looks strange, but I am not sure this is not correct. Sections are sorted by their flags as expected it seems.
I guess the linker script should be more precise about orphpans first of all for having a better and more stable behavior.

19

Done.

jhenderson accepted this revision.Nov 30 2020, 4:23 AM

LGTM. Probably worth waiting for another reviewer too though.

lld/test/ELF/linkerscript/implicit-program-header.test
16
This revision is now accepted and ready to land.Nov 30 2020, 4:23 AM
grimar updated this revision to Diff 308587.Dec 1 2020, 1:56 AM
grimar marked an inline comment as done.
  • Addressed review comment.
MaskRay accepted this revision.EditedDec 1 2020, 11:54 AM

Suggest a rewrite of the description:

This reverts a side effect introduced in the code cleanup patch D43571: LLD started to emit empty output sections that are explicitly assigned to a segment.

This patch fixes the issue by removing the !sec.phdrs.empty() special case from isDiscardable. As compensation, we add an early phdrs propagation step (see the inline comment).
This is similar to one that we do in adjustSectionsAfterSorting.

lld/ELF/LinkerScript.cpp
1074

Suggest:

// The code below may remove empty output sections. We should save the specified program headers (if exist) and propagate them to subsequent sections which do not specify program headers.
// An example of such linker script is:
// SECTIONS { .empty : { *(.empty) } :rw
// .foo : { *(.foo) } }
// Note: at this point the order of output sections has not been finalized, because orphans have not been inserted into their expected positions. We will handle them in adjustSectionsAfterSorting().
MaskRay added inline comments.Dec 1 2020, 11:55 AM
lld/ELF/LinkerScript.cpp
1074

such linker script -> such a linker script

MaskRay retitled this revision from [LLD][ELF] - Don't keep empty output sections that are explicitly assigned to segment. to [LLD][ELF] - Don't keep empty output sections which have explicit program headers.Dec 1 2020, 12:03 PM
grimar edited the summary of this revision. (Show Details)Dec 2 2020, 12:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 12:22 AM