Page MenuHomePhabricator

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

Authored by MaskRay on May 31 2019, 9:27 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.May 31 2019, 9:27 PM
ruiu accepted this revision.Jun 2 2019, 9:35 PM

LGTM

This revision is now accepted and ready to land.Jun 2 2019, 9:35 PM
This revision was automatically updated to reflect the committed changes.
grimar added a comment.Jun 3 2019, 2:27 AM

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.

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.

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

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.

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.