Page MenuHomePhabricator

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

Authored by andrewng on Apr 26 2019, 6:48 AM.

Details

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.

Diff Detail

Event Timeline

andrewng created this revision.Apr 26 2019, 6:48 AM

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

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

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

44

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

andrewng marked 2 inline comments as done.Apr 26 2019, 8:19 AM

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

Yes, that's right.

test/ELF/linkerscript/discard-phdr.s
44

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 added inline comments.Apr 27 2019, 8:03 AM
ELF/LinkerScript.cpp
485

Yes, the auto NonScriptI; logic.

test/ELF/linkerscript/discard-phdr.s
16

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 requested changes to this revision.Apr 29 2019, 1:28 AM

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

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

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.

This revision now requires changes to proceed.Apr 29 2019, 1:28 AM
andrewng marked 2 inline comments as done.Apr 29 2019, 2:56 AM

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

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 added inline comments.Apr 29 2019, 4:05 AM
test/ELF/linkerscript/discard-phdr.s
44

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 added inline comments.Apr 29 2019, 4:09 AM
test/ELF/linkerscript/discard-phdr.s
44

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 updated this revision to Diff 197095.Apr 29 2019, 6:07 AM
andrewng marked an inline comment as done.

Simplified test as suggested in review.

andrewng marked 4 inline comments as done.Apr 29 2019, 6:10 AM
andrewng added inline comments.
test/ELF/linkerscript/discard-phdr.s
44

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

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.)

grimar added a subscriber: psmith.Apr 29 2019, 6:34 AM

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/'.

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).

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?

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.

Can I land this patch?

grimar accepted this revision.Apr 30 2019, 6:45 AM

LGTM.

This revision is now accepted and ready to land.Apr 30 2019, 6:45 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2019, 7:29 AM

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