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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/ELF/linkerscript/discard-phdr.s:44
+.section .baz,"aw"
+ .byte 0
----------------
grimar wrote:
> andrewng wrote:
> > grimar wrote:
> > > 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. 
> > 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?
> 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
> ```
 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
```




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

https://reviews.llvm.org/D61186