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

[George Rimar via Phabricator via llvm-commits]

grimar requested changes to this revision.
grimar added a comment.
This revision now requires changes to proceed.

In D61186#1480496 <https://reviews.llvm.org/D61186#1480496>, @andrewng wrote:

> In D61186#1480437 <https://reviews.llvm.org/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).



================
Comment at: test/ELF/linkerscript/discard-phdr.s:16
+# RUN: ld.lld -o %t --script %t.script %t.o
+# RUN: llvm-readelf -S -l %t | FileCheck %s
+
----------------
MaskRay wrote:
> 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.
> 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

```


================
Comment at: test/ELF/linkerscript/discard-phdr.s:44
+.section .baz,"aw"
+ .byte 0
----------------
andrewng wrote:
> grimar wrote:
> > Does not seem you need all of this sections for the test? Please reduce the number.
> 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.
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. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61186/new/

https://reviews.llvm.org/D61186