[LLD][ELF] /DISCARD/ output sections should not be orphans

[andrewng]

Summary

/DISCARD/ output sections were being treated as orphans. As a result, if
a /DISCARD/ output section has been assigned a PHDR, it could cause
incorrect assignment of sections to segments.

[grimar]

From what I see in the output for this case LLD creates an /DISCARD/ output section.
This does not look correct to me, because /DISCARD/ is kind of key word, but not a section name I believe.
Also, bfd does not do that.

So I think issue first of all is that we are trying to create output section with this name, it should never be in the output.

ELF/LinkerScript.cpp

485 ↗

(On Diff #196842)

For other reviewers, this is for the following place, right?
https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L1385

test/ELF/linkerscript/discard-phdr.s

13 ↗

(On Diff #196842)

I would suggest to use section explicitly listed in the asm.

44 ↗

(On Diff #196842)

Does not seem you need all of this sections for the test? Please reduce the number.

[andrewng]

In D61186#1480437, @grimar wrote:

From what I see in the output for this case LLD creates an /DISCARD/ output section.
This does not look correct to me, because /DISCARD/ is kind of key word, but not a section name I believe.
Also, bfd does not do that.

So I think issue first of all is that we are trying to create output section with this name, it should never be in the output.

Yes, this is a difference compared to bfd and gold, but our policy of keeping empty output sections already differs from bfd and gold, so you could argue that this is not an exception, although definitely undesirable. The '/DISCARD/' output section is only output in this unusual case where it has a PHDR assigned to it, as it is this property that makes it "not discardable".

This patch is only trying to address the problems caused by the '/DISCARD/' being treated as an orphan. I think that fixing the behaviour of '/DISCARD/' to be more like bfd/gold is a more complex change and should be addressed by a separate patch.

ELF/LinkerScript.cpp

485 ↗

(On Diff #196842)

Yes, that's right.

test/ELF/linkerscript/discard-phdr.s

44 ↗

(On Diff #196842)

These are actually required to provoke the issue. It is a somewhat unusual situation (based on a real scenario), so whether this test should be "kept" is debatable.

[MaskRay]

ELF/LinkerScript.cpp

485 ↗

(On Diff #196842)

Yes, the auto NonScriptI; logic.

test/ELF/linkerscript/discard-phdr.s

16 ↗

(On Diff #196842)

Are all these .foo .bar .baz relevant? This script can probably be simplified to contain just /DISCARD/ : { *(.comment) } :NONE and just one another input section description.

Currently we'll see a section named /DISCARD/ in the readelf -S %t output. This patch fixes that.

[grimar]

In D61186#1480496, @andrewng wrote:

In D61186#1480437, @grimar wrote:

From what I see in the output for this case LLD creates an /DISCARD/ output section.
This does not look correct to me, because /DISCARD/ is kind of key word, but not a section name I believe.
Also, bfd does not do that.

So I think issue first of all is that we are trying to create output section with this name, it should never be in the output.

Yes, this is a difference compared to bfd and gold, but our policy of keeping empty output sections already differs from bfd and gold, so you could argue that this is not an exception, although definitely undesirable. The '/DISCARD/' output section is only output in this unusual case where it has a PHDR assigned to it, as it is this property that makes it "not discardable".

This patch is only trying to address the problems caused by the '/DISCARD/' being treated as an orphan. I think that fixing the behaviour of '/DISCARD/' to be more like bfd/gold is a more complex change and should be addressed by a separate patch.

I think it is 2 lines of code change actually :) I believe the much more correct way to fix it is this: https://reviews.llvm.org/D61251
With this approach there is no /DISCARD/ in the output.
(note, I did not change the original test case there).

test/ELF/linkerscript/discard-phdr.s

16 ↗

(On Diff #196842)

Currently we'll see a section named /DISCARD/ in the readelf -S %t output. This patch fixes that.

Nope, it doesn't fix that. /DISCARD/ is still there:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .text             PROGBITS         0000000000000000  00001000
       0000000000000001  0000000000000000  AX       0     0     4
  [ 2] .data             PROGBITS         0000000000000001  00001001
       0000000000000001  0000000000000000  WA       0     0     1
  [ 3] .foo              PROGBITS         0000000000000002  00001002
       0000000000000001  0000000000000000  WA       0     0     1
  [ 4] .bar              PROGBITS         0000000000000003  00001003
       0000000000000001  0000000000000000   A       0     0     1
  [ 5] .baz              PROGBITS         0000000000000004  00001004
       0000000000000001  0000000000000000  WA       0     0     1
  [ 6] /DISCARD/         PROGBITS         0000000000000005  00001005
       0000000000000000  0000000000000000  WA       0     0     1
  [ 7] .symtab           SYMTAB           0000000000000000  00001008
       0000000000000018  0000000000000018           9     1     8
  [ 8] .shstrtab         STRTAB           0000000000000000  00001020
       0000000000000040  0000000000000000           0     0     1
  [ 9] .strtab           STRTAB           0000000000000000  00001060
       0000000000000001  0000000000000000           0     0     1

44 ↗

(On Diff #196842)

No need to replicate the full real case. The test case should show the minimal reproducible scenario of the different output before/after the patch. That is how we do in LLD.

[andrewng]

In D61186#1482074, @grimar wrote:

I think it is 2 lines of code change actually :) I believe the much more correct way to fix it is this: https://reviews.llvm.org/D61251
With this approach there is no /DISCARD/ in the output.
(note, I did not change the original test case there).

Unfortunately, this doesn't completely work, although it does "work" for this particular issue. The problem is if the "/DISCARD/" has a PHDR assignment that needs to propagate to other sections, which is why the "isDiscardable" function returns false for sections with explicit PHDR assignment. It's an unlikely but still possible scenario.

test/ELF/linkerscript/discard-phdr.s

44 ↗

(On Diff #196842)

Just to be clear, you are saying that there is no need to demonstrate the issue being addressed? So the only requirement for a test is to show a change in the output?

[grimar]

test/ELF/linkerscript/discard-phdr.s

44 ↗

(On Diff #196842)

I am saying that I think this test case can be reduced. For example,
I was able to remove 2 sections and 1 PT_LOAD and I think it still demonstrates
that the issue you're fixing is addressed:

# RUN: echo "PHDRS { \
# RUN:   text PT_LOAD FLAGS(0x4 | 0x1); \
# RUN: } \
# RUN: SECTIONS { \
# RUN:  .text : { *(.text) } :text \
# RUN:  .bar : { *(.bar) } \
# RUN:  .baz : { *(.baz) } \
# RUN:  /DISCARD/ : { *(.comment) } :NONE \
# RUN: }" > %t.script
# RUN: ld.lld -o %t --script %t.script %t.o
# RUN: llvm-readelf -S -l %t | FileCheck %s dd

.....

.section .text,"ax"
 ret

.section .bar,"a"
 .byte 0

.section .baz,"ax"
 .byte 0

[grimar]

test/ELF/linkerscript/discard-phdr.s

44 ↗

(On Diff #196842)

Output before patch:

Section to Segment mapping:
  Segment Sections...
   00     .text
   None   /DISCARD/ .bar .baz .symtab .shstrtab .strtab

Output after patch:

Section to Segment mapping:
 Segment Sections...
 00     .text .bar .baz
 None   /DISCARD/ .symtab .shstrtab .strtab

[andrewng]

Simplified test as suggested in review.

[andrewng]

test/ELF/linkerscript/discard-phdr.s

44 ↗

(On Diff #196842)

I have simplified as suggested, although this use case is even "stranger" than the original.

[grimar]

I am still not sure we should not just remove the /DISCARD/ like D61251 did (your explanation makes sense to me, though).
So I would be happy to hear from other people about the current approach.

(I'll also add Peter who works with linker scripts closely.)

[andrewng]

In D61186#1482350, @grimar wrote:

I am still not sure we should not just remove the /DISCARD/ like D61251 did (your explanation makes sense to me, though).

I agree that it would be nice to ensure that '/DISCARD/' it not output, but the focus of this patch is to fix the undesirable side-effects of the current handling of '/DISCARD/'.

[grimar]

In D61186#1482391, @andrewng wrote:

In D61186#1482350, @grimar wrote:

I am still not sure we should not just remove the /DISCARD/ like D61251 did (your explanation makes sense to me, though).

I agree that it would be nice to ensure that '/DISCARD/' it not output, but the focus of this patch is to fix the undesirable side-effects of the current handling of '/DISCARD/'.

Yes, I understand. And I think we probably can go with it (if other people have no objectons about leaving it in the output, at least for now).

[peter.smith]

In D61186#1482193, @andrewng wrote:

In D61186#1482074, @grimar wrote:

I think it is 2 lines of code change actually :) I believe the much more correct way to fix it is this: https://reviews.llvm.org/D61251
With this approach there is no /DISCARD/ in the output.
(note, I did not change the original test case there).

Unfortunately, this doesn't completely work, although it does "work" for this particular issue. The problem is if the "/DISCARD/" has a PHDR assignment that needs to propagate to other sections, which is why the "isDiscardable" function returns false for sections with explicit PHDR assignment. It's an unlikely but still possible scenario.

Just to clarify my understanding, are you referring to (https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/Using_ld_the_GNU_Linker/sections.html#OUTPUT-SECTION-PHDR)

If a section is assigned to one or more segments, then all subsequent allocated sections will be assigned to those segments as well, unless they use an explicitly :phdr modifier. You can use :NONE to tell the linker to not put the section in any segment at all.

Something like:

/DISCARD/ : { *(.comment) } :my_phdr
 .name : { *(.name) }
...

Where we'd want .name to to be assigned to my_phdr and any following OutputSections? I can see that this is how ld.bfd and ld.gold behave, although as you say this is somewhat of a contrived example.

I'm with George in that we shouldn't output the /DISCARD/ as a SectionHeader as it isn't really an OutputSection, although as I understand it this is not a correctness problem. If we don't fix it in this patch can we add a FIXME ann/or raise a PR so that we know that we have a problem?

[andrewng]

In D61186#1482564, @peter.smith wrote:

Just to clarify my understanding, are you referring to (https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/4/html/Using_ld_the_GNU_Linker/sections.html#OUTPUT-SECTION-PHDR)

If a section is assigned to one or more segments, then all subsequent allocated sections will be assigned to those segments as well, unless they use an explicitly :phdr modifier. You can use :NONE to tell the linker to not put the section in any segment at all.

Something like:

/DISCARD/ : { *(.comment) } :my_phdr
 .name : { *(.name) }
...

Where we'd want .name to to be assigned to my_phdr and any following OutputSections? I can see that this is how ld.bfd and ld.gold behave, although as you say this is somewhat of a contrived example.

Yes, very unlikely but unfortunately possible.

I'm with George in that we shouldn't output the /DISCARD/ as a SectionHeader as it isn't really an OutputSection, although as I understand it this is not a correctness problem. If we don't fix it in this patch can we add a FIXME ann/or raise a PR so that we know that we have a problem?

I think a PR would be the way to go.

[andrewng]

Can I land this patch?

[grimar]

LGTM.

[andrewng]

Submitted bug https://bugs.llvm.org/show_bug.cgi?id=41673 regarding the issue that the /DISCARD/ section is in the output.

lld/trunk/ELF/LinkerScript.cpp

for (BaseCommand *Base : SectionCommands) {
     if (auto *Sec = dyn_cast<OutputSection>(Base)) {
       std::vector<InputSection *> V = createInputSectionList(*Sec);
 
       // The output section name `/DISCARD/' is special.
       // Any input section assigned to it is discarded.
       if (Sec->Name == "/DISCARD/") {
         discard(V);
         Sec->SectionCommands.clear();
+        Sec->SectionIndex = 0; // Not an orphan.
         continue;
       }
 
       // This is for ONLY_IF_RO and ONLY_IF_RW. An output section directive
       // ".foo : ONLY_IF_R[OW] { ... }" is handled only if all member input
       // sections satisfy a given constraint. If not, a directive is handled
       // as if it wasn't present from the beginning.
       //

lld/trunk/test/ELF/linkerscript/discard-phdr.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: echo "PHDRS { \
+# RUN:   exec PT_LOAD FLAGS(0x4 | 0x1); \
+# RUN: } \
+# RUN: SECTIONS { \
+# RUN:  .text : { *(.text) } :exec \
+# RUN:  .foo : { *(.foo) } \
+# RUN:  .bar : { *(.bar) } \
+# RUN:  /DISCARD/ : { *(.discard) } :NONE \
+# RUN: }" > %t.script
+# RUN: ld.lld -o %t --script %t.script %t.o
+# RUN: llvm-readelf -S -l %t | FileCheck %s
+
+## Check that /DISCARD/ does not interfere with the assignment of segments to
+## sections.
+
+# CHECK: Section Headers
+# CHECK: .text
+# CHECK-NEXT: .foo
+# CHECK-NEXT: .bar
+
+# CHECK: Segment Sections
+# CHECK-NEXT: .text .foo .bar
+
+.section .text,"ax"
+ ret
+
+.section .foo,"a"
+ .byte 0
+
+.section .bar,"ax"
+ ret
+
+.section .discard
+ .byte 0