[ELF] Don't resolve relocations referencing SHN_ABS to tombstone in non-SHF_ALLOC sections #79238

MaskRay · 2024-01-24T01:53:35Z

A SHN_ABS symbol has never been considered for InputSection::relocateNonAlloc.
Before #74686, the code did made it work in the absence of -z dead-reloc-in-nonalloc=.
There is now a report about such SHN_ABS uses
(#74686 (comment))
and I think it makes sense for non-SHF_ALLOC to support SHN_ABS, like
SHF_ALLOC sections do.

// clang -g
__attribute__((weak)) int symbol;
int *foo() { return &symbol; }

0x00000023:   DW_TAG_variable [2]   (0x0000000c)
                ...
                DW_AT_location [DW_FORM_exprloc]        (DW_OP_addrx 0x0)

.debug_addr references symbol, which can be redefined by a symbol
assignment or --defsym to become a SHN_ABS symbol.

The problem is that !sym.getOutputSection() cannot discern SHN_ABS
from a symbol whose section has been discarded. Since commit
1981b1b, a symbol relative to a
discarded section is changed to Undefined, so the SHN_ABS check
become trivial.

We currently apply tombstone for a relocation referencing
SharedSymbol. This patch does not change the behavior.

Created using spr 1.3.4

llvmbot · 2024-01-24T01:54:06Z

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Author: Fangrui Song (MaskRay)

Changes

A SHN_ABS symbol has never been considered for InputSection::relocateNonAlloc.
Before #74686, the code did made it work in the absence of -z dead-reloc-in-nonalloc=.
There is now a report about such SHN_AVS uses
(#74686 (comment))
and I think it makes sense for non-SHF_ALLOC to support SHN_ABS, like
SHF_ALLOC sections do.

The problem is that !sym.getOutputSection() cannot discern SHN_ABS
from a symbol whose section has been discarded. Since commit
1981b1b, a symbol relative to a
discarded section is changed to Undefined, so the SHN_ABS check
become trivial.

We currently apply tombstone for a relocation referencing
SharedSymbol. This patch does not change the behavior.

Full diff: https://github.com/llvm/llvm-project/pull/79238.diff

2 Files Affected:

(modified) lld/ELF/InputSection.cpp (+6-7)
(modified) lld/test/ELF/dead-reloc-in-nonalloc.s (+1-1)

diff --git a/lld/ELF/InputSection.cpp b/lld/ELF/InputSection.cpp
index c728dd6c6306aa0..0e0b9783bd88a0f 100644
--- a/lld/ELF/InputSection.cpp
+++ b/lld/ELF/InputSection.cpp
@@ -961,12 +961,11 @@ void InputSection::relocateNonAlloc(uint8_t *buf, ArrayRef<RelTy> rels) {
       // vector. The computed value is st_value plus a non-negative offset.
       // Negative values are invalid, so -1 can be used as the tombstone value.
       //
-      // If the referenced symbol is discarded (made Undefined), or the
-      // section defining the referenced symbol is garbage collected,
-      // sym.getOutputSection() is nullptr. `ds->folded` catches the ICF folded
-      // case. However, resolving a relocation in .debug_line to -1 would stop
-      // debugger users from setting breakpoints on the folded-in function, so
-      // exclude .debug_line.
+      // If the referenced symbol is relative to a discarded section (due to
+      // --gc-sections, COMDAT, etc), it has been converted to a Undefined.
+      // `ds->folded` catches the ICF folded case. However, resolving a
+      // relocation in .debug_line to -1 would stop debugger users from setting
+      // breakpoints on the folded-in function, so exclude .debug_line.
       //
       // For pre-DWARF-v5 .debug_loc and .debug_ranges, -1 is a reserved value
       // (base address selection entry), use 1 (which is used by GNU ld for
@@ -974,7 +973,7 @@ void InputSection::relocateNonAlloc(uint8_t *buf, ArrayRef<RelTy> rels) {
       //
       // TODO To reduce disruption, we use 0 instead of -1 as the tombstone
       // value. Enable -1 in a future release.
-      if (!sym.getOutputSection() || (ds && ds->folded && !isDebugLine)) {
+      if (!ds || (ds->folded && !isDebugLine)) {
         // If -z dead-reloc-in-nonalloc= is specified, respect it.
         uint64_t value = SignExtend64<bits>(*tombstone);
         // For a 32-bit local TU reference in .debug_names, X86_64::relocate
diff --git a/lld/test/ELF/dead-reloc-in-nonalloc.s b/lld/test/ELF/dead-reloc-in-nonalloc.s
index 145604eb883a9af..b675fc50fc2ea2f 100644
--- a/lld/test/ELF/dead-reloc-in-nonalloc.s
+++ b/lld/test/ELF/dead-reloc-in-nonalloc.s
@@ -17,7 +17,7 @@
 # AA:          Contents of section .debug_info:
 # AA-NEXT:      0000 [[ADDR]] 00000000 aaaaaaaa 00000000
 # AA:          Contents of section .not_debug:
-# AA-NEXT:      0000 bbbbbbbb bbbbbbbb 00000000          .
+# AA-NEXT:      0000 bbbbbbbb 2a000000 00000000          .
 
 ## Specifying zero can get a behavior similar to GNU ld.
 # RUN: ld.lld --icf=all -z dead-reloc-in-nonalloc=.debug_info=0 %t.o %tabs.o -o %tzero

bevin-hansson · 2024-01-24T07:53:47Z

This patch resolves the issue we have. I posted an example of the kind of code that triggers it in the previous PR.

MaskRay · 2024-01-24T08:24:55Z

This patch resolves the issue we have. I posted an example of the kind of code that triggers it in the previous PR.

Thanks for verification. Do you have more information about SHN_ABS in debug sections? Ideally you can share a reproduce tarball (ld.lld --reproduce=rep.tar) after minimization.

edit: figured out. added to the description.

Created using spr 1.3.4

bevin-hansson

This looks good to me, but someone else should probably chime in as well to approve.

smithp35

Code changes LGTM too. I can't think of a case when a C/C++ compiler would directly generate a reference to an SHN_ABS symbol as I don't think there is a way of expressing such a symbol in C/C++ and debug generation concentrates on describing entities in the same module. I've asked a colleague that knows more about debug generation than I do. If I get an answer I'll post another comment.

There is a typo in the description (SHN_ANS) -> (SHN_ABS)

Created using spr 1.3.4

MaskRay · 2024-01-24T16:53:09Z

Fixed a typo and added an example

// clang -g
__attribute__((weak)) int symbol;
int *foo() { return &symbol; }

0x00000023:   DW_TAG_variable [2]   (0x0000000c)
                ...
                DW_AT_location [DW_FORM_exprloc]        (DW_OP_addrx 0x0)
.debug_addr references symbol, which can be redefined by a symbol
assignment or --defsym to become a SHN_ABS symbol.

ayermolo · 2024-01-24T18:04:36Z

Hmm, interesting. Thanks for the fix.

…on-SHF_ALLOC sections (llvm#79238) A SHN_ABS symbol has never been considered for InputSection::relocateNonAlloc. Before llvm#74686, the code did made it work in the absence of `-z dead-reloc-in-nonalloc=`. There is now a report about such SHN_ABS uses (llvm#74686 (comment)) and I think it makes sense for non-SHF_ALLOC to support SHN_ABS, like SHF_ALLOC sections do. ``` // clang -g __attribute__((weak)) int symbol; int *foo() { return &symbol; } 0x00000023: DW_TAG_variable [2] (0x0000000c) ... DW_AT_location [DW_FORM_exprloc] (DW_OP_addrx 0x0) ``` .debug_addr references `symbol`, which can be redefined by a symbol assignment or --defsym to become a SHN_ABS symbol. The problem is that `!sym.getOutputSection()` cannot discern SHN_ABS from a symbol whose section has been discarded. Since commit 1981b1b, a symbol relative to a discarded section is changed to `Undefined`, so the `SHN_ABS` check become trivial. We currently apply tombstone for a relocation referencing `SharedSymbol`. This patch does not change the behavior. (cherry picked from commit 8abf8d1)

…on-SHF_ALLOC sections (#79238) A SHN_ABS symbol has never been considered for InputSection::relocateNonAlloc. Before #74686, the code did made it work in the absence of `-z dead-reloc-in-nonalloc=`. There is now a report about such SHN_ABS uses (#74686 (comment)) and I think it makes sense for non-SHF_ALLOC to support SHN_ABS, like SHF_ALLOC sections do. ``` // clang -g __attribute__((weak)) int symbol; int *foo() { return &symbol; } 0x00000023: DW_TAG_variable [2] (0x0000000c) ... DW_AT_location [DW_FORM_exprloc] (DW_OP_addrx 0x0) ``` .debug_addr references `symbol`, which can be redefined by a symbol assignment or --defsym to become a SHN_ABS symbol. The problem is that `!sym.getOutputSection()` cannot discern SHN_ABS from a symbol whose section has been discarded. Since commit 1981b1b, a symbol relative to a discarded section is changed to `Undefined`, so the `SHN_ABS` check become trivial. We currently apply tombstone for a relocation referencing `SharedSymbol`. This patch does not change the behavior. (cherry picked from commit 8abf8d1)

…on-SHF_ALLOC sections (llvm#79238) A SHN_ABS symbol has never been considered for InputSection::relocateNonAlloc. Before llvm#74686, the code did made it work in the absence of `-z dead-reloc-in-nonalloc=`. There is now a report about such SHN_ABS uses (llvm#74686 (comment)) and I think it makes sense for non-SHF_ALLOC to support SHN_ABS, like SHF_ALLOC sections do. ``` // clang -g __attribute__((weak)) int symbol; int *foo() { return &symbol; } 0x00000023: DW_TAG_variable [2] (0x0000000c) ... DW_AT_location [DW_FORM_exprloc] (DW_OP_addrx 0x0) ``` .debug_addr references `symbol`, which can be redefined by a symbol assignment or --defsym to become a SHN_ABS symbol. The problem is that `!sym.getOutputSection()` cannot discern SHN_ABS from a symbol whose section has been discarded. Since commit 1981b1b, a symbol relative to a discarded section is changed to `Undefined`, so the `SHN_ABS` check become trivial. We currently apply tombstone for a relocation referencing `SharedSymbol`. This patch does not change the behavior. (cherry picked from commit 8abf8d1)

ayermolo · 2024-02-29T00:04:05Z

@MaskRay Is there an equivalent check that can be done before 1981b1b?

MaskRay · 2024-02-29T00:17:22Z

@MaskRay Is there an equivalent check that can be done before 1981b1b?

Do you want to cherry pick #74686 and this follow-up #79238 with an older code base like lld 17?
I believe it's difficult before 1981b1b . I personally would not try doing that...

You can consider cherry picking 1981b1b as well, but there are multiple follow-up commits.

Note, if your llvm is locked into a certain version, it should be quite straightforward to sync just lld/ to the trunk version, and ninja lld will likely still compile (if not it'd be straightforward to fix). I've locally done this to check the performance improvement of lld across several llvm releases.

ayermolo · 2024-02-29T00:23:54Z

@MaskRay Is there an equivalent check that can be done before 1981b1b?

Do you want to cherry pick #74686 and this follow-up #79238 with an older code base like lld 17? I believe it's difficult before 1981b1b . I personally would not try doing that...

You can consider cherry picking 1981b1b as well, but there are multiple follow-up commits.

Note, if you llvm locked into a certain version, it should be quite straightforward to sync just lld/ to the trunk version, and ninja lld will likely still compile (if not it'd be straightforward to fix).

It's even worse I am trying to cherry pick #74686 and #79238 onto llvm-15 (although llvm-17 is next).

I looked into cherry picking 1981b1b, but it touches a lot of code that is not even in llvm-15. Thus I was wondering if check is possible some other way.

So without this LLD will tombstone some addresses in a .debug section OR if dead-reloc-in-nonalloc is specified, correct?

MaskRay · 2024-02-29T00:31:27Z

@MaskRay Is there an equivalent check that can be done before 1981b1b?

Do you want to cherry pick #74686 and this follow-up #79238 with an older code base like lld 17? I believe it's difficult before 1981b1b . I personally would not try doing that...
You can consider cherry picking 1981b1b as well, but there are multiple follow-up commits.
Note, if you llvm locked into a certain version, it should be quite straightforward to sync just lld/ to the trunk version, and ninja lld will likely still compile (if not it'd be straightforward to fix).

It's even worse I am trying to cherry pick #74686 and #79238 onto llvm-15 (although llvm-17 is next).

I looked into cherry picking 1981b1b, but it touches a lot of code that is not even in llvm-15. Thus I was wondering if check is possible some other way.

So without this LLD will tombstone some addresses in a .debug section OR if dead-reloc-in-nonalloc is specified, correct?

Without checking 1981b1b , you can check a SHN_ABS symbol with something like !cast<Defined>(sym).section. I haven't tested the use in relocateNonAlloc, though.

ayermolo · 2024-02-29T00:40:37Z

@MaskRay Is there an equivalent check that can be done before 1981b1b?

Do you want to cherry pick #74686 and this follow-up #79238 with an older code base like lld 17? I believe it's difficult before 1981b1b . I personally would not try doing that...
You can consider cherry picking 1981b1b as well, but there are multiple follow-up commits.
Note, if you llvm locked into a certain version, it should be quite straightforward to sync just lld/ to the trunk version, and ninja lld will likely still compile (if not it'd be straightforward to fix).

It's even worse I am trying to cherry pick #74686 and #79238 onto llvm-15 (although llvm-17 is next).
I looked into cherry picking 1981b1b, but it touches a lot of code that is not even in llvm-15. Thus I was wondering if check is possible some other way.
So without this LLD will tombstone some addresses in a .debug section OR if dead-reloc-in-nonalloc is specified, correct?

Without checking 1981b1b , you can check a SHN_ABS symbol with something like !cast<Defined>(sym).section. I haven't tested the use in relocateNonAlloc, though.

ok thanks. Appreciate the help.

[𝘀𝗽𝗿] initial version

97b500a

Created using spr 1.3.4

llvmbot added lld lld:ELF labels Jan 24, 2024

MaskRay requested review from dwblaikie, cmtice, ayermolo, bevin-hansson and smithp35 January 24, 2024 01:54

edit description, include C example

5296604

Created using spr 1.3.4

bevin-hansson reviewed Jan 24, 2024

View reviewed changes

smithp35 approved these changes Jan 24, 2024

View reviewed changes

fix typo. clarify .debug_addr

14fab61

Created using spr 1.3.4

MaskRay merged commit 8abf8d1 into main Jan 24, 2024
3 of 4 checks passed

MaskRay deleted the users/MaskRay/spr/elf-dont-resolve-relocations-referencing-shn_abs-to-tombstone-in-non-shf_alloc-sections branch January 24, 2024 16:53

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ELF] Don't resolve relocations referencing SHN_ABS to tombstone in non-SHF_ALLOC sections #79238

[ELF] Don't resolve relocations referencing SHN_ABS to tombstone in non-SHF_ALLOC sections #79238

MaskRay commented Jan 24, 2024 •

edited

Loading

llvmbot commented Jan 24, 2024 •

edited

Loading

bevin-hansson commented Jan 24, 2024

MaskRay commented Jan 24, 2024 •

edited

Loading

bevin-hansson left a comment

smithp35 left a comment

MaskRay commented Jan 24, 2024

ayermolo commented Jan 24, 2024

ayermolo commented Feb 29, 2024

MaskRay commented Feb 29, 2024 •

edited

Loading

ayermolo commented Feb 29, 2024

MaskRay commented Feb 29, 2024 •

edited

Loading

ayermolo commented Feb 29, 2024 •

edited

Loading

[ELF] Don't resolve relocations referencing SHN_ABS to tombstone in non-SHF_ALLOC sections #79238

[ELF] Don't resolve relocations referencing SHN_ABS to tombstone in non-SHF_ALLOC sections #79238

Conversation

MaskRay commented Jan 24, 2024 • edited Loading

llvmbot commented Jan 24, 2024 • edited Loading

bevin-hansson commented Jan 24, 2024

MaskRay commented Jan 24, 2024 • edited Loading

bevin-hansson left a comment

Choose a reason for hiding this comment

smithp35 left a comment

Choose a reason for hiding this comment

MaskRay commented Jan 24, 2024

ayermolo commented Jan 24, 2024

ayermolo commented Feb 29, 2024

MaskRay commented Feb 29, 2024 • edited Loading

ayermolo commented Feb 29, 2024

MaskRay commented Feb 29, 2024 • edited Loading

ayermolo commented Feb 29, 2024 • edited Loading

MaskRay commented Jan 24, 2024 •

edited

Loading

llvmbot commented Jan 24, 2024 •

edited

Loading

MaskRay commented Jan 24, 2024 •

edited

Loading

MaskRay commented Feb 29, 2024 •

edited

Loading

MaskRay commented Feb 29, 2024 •

edited

Loading

ayermolo commented Feb 29, 2024 •

edited

Loading