[ELF] Don't create an output section named `/DISCARD/` if it is assigned to the special phdr `NONE`

[MaskRay]

Summary

Fixes the remaining issue of PR41673 after D61186: with /DISCARD/ { ... } :NONE,
we may create an output section named /DISCARD/.

Note, if an input section is named /DISCARD/, ld.bfd discards it but
lld keeps it. It is probably not worth copying this behavior as it is unrealistic.

[ruiu]

LGTM

[grimar]

Note: The way implemented in this patch was discussed in D61186 thread,
but since this one was landed, we do not need the change done by D61186.
So I committed rL362367 which reverts the change done by D61186.

[andrewng]

George's comment reminds me why we didn't take this approach in the first place, as it doesn't work correctly for the following test case:

# 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:   rodata PT_LOAD FLAGS(0x4); \
# RUN: } \
# RUN: SECTIONS { \
# RUN:  .text : { *(.text) } :exec \
# RUN:  /DISCARD/ : { *(.discard) } :rodata \
# RUN:  .rodata : { *(.rodata) } \
# RUN: }" > %t.script
# RUN: ld.lld -o %t --script %t.script %t.o
# RUN: llvm-readelf -S -l %t | FileCheck --implicit-check-not=/DISCARD/ %s

## Check that /DISCARD/ phdr is assigned to subsequent sections.

# CHECK: Section Headers
# CHECK: .text
# CHECK-NEXT: .rodata

# CHECK: Segment Sections
# CHECK-NEXT: .text
# CHECK-NEXT: .rodata

.section .text,"ax"
 ret

.section .rodata,"a"
 .byte 0

.section .discard,"a"
 .byte 0

However, this use case is a very unusual edge case, although bfd does handle it.

[MaskRay]

In D62768#1527184, @andrewng wrote:

George's comment reminds me why we didn't take this approach in the first place, as it doesn't work correctly for the following test case:

# 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:   rodata PT_LOAD FLAGS(0x4); \
# RUN: } \
# RUN: SECTIONS { \
# RUN:  .text : { *(.text) } :exec \
# RUN:  /DISCARD/ : { *(.discard) } :rodata \
# RUN:  .rodata : { *(.rodata) } \
# RUN: }" > %t.script
# RUN: ld.lld -o %t --script %t.script %t.o
# RUN: llvm-readelf -S -l %t | FileCheck --implicit-check-not=/DISCARD/ %s

## Check that /DISCARD/ phdr is assigned to subsequent sections.

# CHECK: Section Headers
# CHECK: .text
# CHECK-NEXT: .rodata

# CHECK: Segment Sections
# CHECK-NEXT: .text
# CHECK-NEXT: .rodata

.section .text,"ax"
 ret

.section .rodata,"a"
 .byte 0

.section .discard,"a"
 .byte 0

However, this use case is a very unusual edge case, although bfd does handle it.

If I read correctly, this is to assign /DISCARD to the memory region rodata and expect .rodata to inherit the memory region?

Sorry I didn't follow the discussion in D61186 and didn't notice D61251. It seems @grimar and @peter.smith were fine with D61251 (this patch is essentially D61251). @andrewng if you are ok with letting this somewhat contrived example fail, we can probably still mark this as resolved (wontfix).

[andrewng]

If I read correctly, this is to assign /DISCARD to the memory region rodata and expect .rodata to inherit the memory region?

Sorry I didn't follow the discussion in D61186 and didn't notice D61251. It seems @grimar and @peter.smith were fine with D61251 (this patch is essentially D61251). @andrewng if you are ok with letting this somewhat contrived example fail, we can probably still mark this as resolved (wontfix).

I'm OK with this unusual test case failing, if everyone else is fine with it too. It was just something I came across whilst creating D61186 and was why I didn't take this approach with isDiscardable.

[peter.smith]

Sorry I didn't follow the discussion in D61186 and didn't notice D61251. It seems @grimar and @peter.smith were fine with D61251 (this patch is essentially D61251). @andrewng if you are ok with letting this somewhat contrived example fail, we can probably still mark this as resolved (wontfix).

Just to confirm, I've no objections to D61251.

lld/trunk/ELF/LinkerScript.cpp

for (BaseCommand *Base : Sec->SectionCommands) {
     // It calculates and assigns the offsets for each section and also
     // updates the output section size.
     for (InputSection *Sec : cast<InputSectionDescription>(Base)->Sections)
       output(Sec);
   }
 }
 
 static bool isDiscardable(OutputSection &Sec) {
+  if (Sec.Name == "/DISCARD/")
+    return true;
+
   // We do not remove empty sections that are explicitly
   // assigned to any segment.
   if (!Sec.Phdrs.empty())
     return false;
 
   // We do not want to remove OutputSections with expressions that reference
   // symbols even if the OutputSection is empty. We want to ensure that the
   // expressions can be evaluated and report an error if they cannot.

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
+# RUN: llvm-readelf -S -l %t | FileCheck --implicit-check-not=/DISCARD/ %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