[ELF] -r --compress-debug-sections: update implicit addends for .rel.debug_* referencing STT_SECTION symbols #66804

MaskRay · 2023-09-19T19:06:27Z

https://reviews.llvm.org/D48929 updated addends for non-SHF_ALLOC sections
relocated by REL for -r links, but the patch did not update the addends when
--compress-debug-sections={zlib,zstd} is used (#66738).

https://reviews.llvm.org/D116946 handled tombstone values in debug
sections in relocatable links. As a side effect, both
relocateNonAllocForRelocatable (using sec->relocations) and
relocatenonNonAlloc (using raw REL/RELA) may run.

Actually, we can adjust the condition in relocatenonAlloc to completely replace
relocateNonAllocForRelocatable. This patch implements this idea and fixes #66738.

As relocateNonAlloc processes the raw relocations like copyRelocations() does,
the condition if (config->relocatable && type != target.noneRel) in copyRelocations
(commit 08d6a3f, modified by https://reviews.llvm.org/D62052)
can be made specific to SHF_ALLOC sections.

As a side effect, we can now report diagnostics for PC-relative relocations for
-r. This is a less useful diagnostic that is not worth too much code. As
ClangBuiltLinux/linux#1937 has violations, just
suppress the warning for -r. Tested by commit 561b98f.

llvmbot · 2023-09-19T19:07:32Z

@llvm/pr-subscribers-lld-elf

@llvm/pr-subscribers-lld

Changes

https://reviews.llvm.org/D48929 updated addends for non-SHF_ALLOC sections
relocated by REL, but the patch did not handle the addends when
--compress-debug-sections={zlib,zstd} is used.

https://reviews.llvm.org/D116946 handled tombstone values in debug
sections in relocatable links. As a side effect, both
relocateNonAllocForRelocatable (using sec->relocations) and
relocatenonNonAlloc (using raw REL/RELA) may run.

Actually, we can now update addends in relocatenonAlloc, eliminating
relocateNonAllocForRelocatable. This patch implements this idea and fixes #66738:

Adjust if (config->relocatable && type != target.noneRel) in copyRelocations
(commit 08d6a3f, modified by https://reviews.llvm.org/D62052)
to restrict to SHF_ALLOC sections.

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

2 Files Affected:

(modified) lld/ELF/InputSection.cpp (+7-22)
(modified) lld/test/ELF/relocatable-section-symbol.s (+5-7)

diff --git a/lld/ELF/InputSection.cpp b/lld/ELF/InputSection.cpp
index f97ca96bf4a852e..cfd9cba77a4ad96 100644
--- a/lld/ELF/InputSection.cpp
+++ b/lld/ELF/InputSection.cpp
@@ -463,7 +463,10 @@ void InputSection::copyRelocations(uint8_t *buf,
 
       if (RelTy::IsRela)
         p->r_addend = sym.getVA(addend) - section->getOutputSection()->addr;
-      else if (config->relocatable && type != target.noneRel)
+      // See the comment in writeSections. For SHF_ALLOC sections relocated by
+      // REL relocations, utilize the non-REL/RELA loop.
+      else if (config->relocatable && (sec->flags & SHF_ALLOC) &&
+               type != target.noneRel)
         sec->addReloc({R_ABS, type, rel.offset, addend, &sym});
     } else if (config->emachine == EM_PPC && type == R_PPC_PLTREL24 &&
                p->r_addend >= 0x8000 && sec->file->ppc32Got2) {
@@ -954,8 +957,9 @@ void InputSection::relocateNonAlloc(uint8_t *buf, ArrayRef<RelTy> rels) {
       }
     }
 
-    // For a relocatable link, only tombstone values are applied.
-    if (config->relocatable)
+    // For a relocatable link, only tombstone values are applied when relocated
+    // by REL relocations.
+    if (RelTy::IsRela && config->relocatable)
       continue;
 
     if (expr == R_SIZE) {
@@ -994,23 +998,6 @@ void InputSection::relocateNonAlloc(uint8_t *buf, ArrayRef<RelTy> rels) {
   }
 }
 
-// This is used when '-r' is given.
-// For REL targets, InputSection::copyRelocations() may store artificial
-// relocations aimed to update addends. They are handled in relocateAlloc()
-// for allocatable sections, and this function does the same for
-// non-allocatable sections, such as sections with debug information.
-static void relocateNonAllocForRelocatable(InputSection *sec, uint8_t *buf) {
-  const unsigned bits = config->is64 ? 64 : 32;
-
-  for (const Relocation &rel : sec->relocs()) {
-    // InputSection::copyRelocations() adds only R_ABS relocations.
-    assert(rel.expr == R_ABS);
-    uint8_t *bufLoc = buf + rel.offset;
-    uint64_t targetVA = SignExtend64(rel.sym->getVA(rel.addend), bits);
-    target->relocate(bufLoc, rel, targetVA);
-  }
-}
-
 template <class ELFT>
 void InputSectionBase::relocate(uint8_t *buf, uint8_t *bufEnd) {
   if ((flags & SHF_EXECINSTR) && LLVM_UNLIKELY(getFile<ELFT>()->splitStack))
@@ -1022,8 +1009,6 @@ void InputSectionBase::relocate(uint8_t *buf, uint8_t *bufEnd) {
   }
 
   auto *sec = cast<InputSection>(this);
-  if (config->relocatable)
-    relocateNonAllocForRelocatable(sec, buf);
   // For a relocatable link, also call relocateNonAlloc() to rewrite applicable
   // locations with tombstone values.
   const RelsOrRelas<ELFT> rels = sec->template relsOrRelas<ELFT>();
diff --git a/lld/test/ELF/relocatable-section-symbol.s b/lld/test/ELF/relocatable-section-symbol.s
index 9d8892236304b22..2ee1e7692cf2c59 100644
--- a/lld/test/ELF/relocatable-section-symbol.s
+++ b/lld/test/ELF/relocatable-section-symbol.s
@@ -30,10 +30,11 @@
 
 # RUN: llvm-mc -filetype=obj -triple=i686 %s -o %t1.o
 # RUN: ld.lld -r -o %t1 %t1.o %t1.o
-# RUN: llvm-readelf -r -x .data -x .bar -x .debug_line %t1 | FileCheck %s --check-prefixes=REL,REL0
+# RUN: llvm-readelf -r -x .data -x .bar -x .debug_line %t1 | FileCheck %s --check-prefix=REL
+## https://github.com/llvm/llvm-project/issues/66738 Update implicit addends for -r and --compress-debug-sections
 # RUN: ld.lld -r --compress-debug-sections=zlib -o %t1.zlib %t1.o %t1.o
 # RUN: llvm-objcopy --decompress-debug-sections %t1.zlib %t1.zlib.de
-# RUN: llvm-readelf -r -x .data -x .bar -x .debug_line %t1.zlib.de | FileCheck %s --check-prefixes=REL,REL1
+# RUN: llvm-readelf -r -x .data -x .bar -x .debug_line %t1.zlib.de | FileCheck %s --check-prefix=REL
 
 # REL:         Offset   Info   Type                Sym. Value  Symbol's Name
 # REL-NEXT:  00000000  {{.*}} R_386_32               00000000   .text
@@ -55,11 +56,8 @@
 # REL-NEXT:  0x00000000 01000000 05000000                   ........
 # REL:       Hex dump of section '.bar':
 # REL-NEXT:  0x00000000 01000000 00000000 02000000 04000000 ................
-# REL0:      Hex dump of section '.debug_line':
-# REL0-NEXT: 0x00000000 01000000 00000000 02000000 04000000 ................
-## FIXME: https://github.com/llvm/llvm-project/issues/66738 The implicit addends for the second input section are wrong.
-# REL1:      Hex dump of section '.debug_line':
-# REL1-NEXT: 0x00000000 01000000 00000000 01000000 00000000 ................
+# REL:       Hex dump of section '.debug_line':
+# REL-NEXT:  0x00000000 01000000 00000000 02000000 04000000 ................
 
 .long 42
 .data

nickdesaulniers · 2023-09-19T20:00:23Z

Thanks for the patch! It resolves the build issue I was observing that lead to the report in #66738.

One thing I've noticed is that now the build succeeds, but we have a stream of warnings from lld that look like:

ld.lld: warning: vmlinux.a(drivers/i2c/busses/i2c-i801.o):(.discard.retpoline_safe+0x120): has non-ABS relocation R_386_PC32 against symbol ''

(maybe 200 of these warnings are printed).

Let me do some testing to see if it's related to a specific kernel configuration or what. It's not clear to me yet if there's more than one bug here, or if this PR is improving one thing while regressing another.

I'll also do further testing on 32b arm targets that we were also observing errors with.

nickdesaulniers · 2023-09-19T20:10:00Z

Forgetting compressed debug info for a minute, those LLD warnings are reproducible with:

$ cd linux
$ make LLVM=1 ARCH=i386 -j128 -s defconfig all

The warnings are not observed at the commit before your change. Seems like a regression?

nickdesaulniers · 2023-09-19T20:17:17Z

No issues observed for ARCH=arm with your change; only ARCH=i386.

nickdesaulniers

Fixes builds with compressed debug info, but introduces warnings for builds without any debug info (compressed or not) for 32b x86.

MaskRay · 2023-09-19T22:05:54Z

Forgetting compressed debug info for a minute, those LLD warnings are reproducible with:
$ cd linux
$ make LLVM=1 ARCH=i386 -j128 -s defconfig all
The warnings are not observed at the commit before your change. Seems like a regression?

Thanks for testing this patch on the Linux kernel. However, I think the problem is in the Linux kernel, specifically, arch/x86/include/asm/nospec-branch.h. ClangBuiltLinux/linux#1937

nickdesaulniers

Ok, thanks for bisecting to the culprit in the kernel; I think that's something we can pursue getting fixed up in the kernel sources.

Consider backporting this to 17.0.1 so that we can have compressed debug info on 32b targets sooner.

Thanks for the quick fix and diagnoses.

EDIT: d'oh misclicked "request changes" meant to approve

MaskRay · 2023-09-20T04:20:14Z

Add a special case in InputSection::relocateNonAlloc to work around ClangBuiltLinux/linux#1937

arichardson

LGTM

lld/ELF/InputSection.cpp

smithp35

To make sure I understand the change:

relocateNonAlloc now does all the work that relocateNonAllocForRelocateble so we can remove it.
we now no longer need to copy relocations from non SHF_ALLOC sections as relocateNonAlloc processes the raw relocations like copyRelocations() does.

If the above is correct, I got a bit confused by the comment, but otherwise LGTM.

smithp35 · 2023-09-20T14:45:45Z

lld/ELF/InputSection.cpp

@@ -463,7 +463,10 @@ void InputSection::copyRelocations(uint8_t *buf,

      if (RelTy::IsRela)
        p->r_addend = sym.getVA(addend) - section->getOutputSection()->addr;
-      else if (config->relocatable && type != target.noneRel)
+      // See the comment in writeSections. For SHF_ALLOC sections relocated by


I guess the comment is:

// In -r or --emit-relocs mode, write the relocation sections first as in // ELf_Rel targets we might find out that we need to modify the relocated // section while doing it.

I not sure what the non-REL/RELA loop is referring to? I'm assuming this parses as (non-REL)/RELA and not non-(REL/RELA).

In writeSections there is a write relocations (REL or RELA), write non-relocations (!REL && !RELA) followed by an optional dynamic relocations check.

Would it be possible to clarify?

Thanks for the comment. Clarified in d63198be55293facbd5dd9ed929a32a710be56ad

Thanks for the update LGTM.

MaskRay · 2023-09-20T17:47:16Z

I improved relocatable-local-sym.s in commit a317afa. I need to adjust the condition to restrict to STT_SECTION (keeping just one STT_SECTION, originally from 08d6a3f)

@@ -954,8 +960,10 @@ void InputSection::relocateNonAlloc(uint8_t *buf, ArrayRef<RelTy> rels) {
       }
     }

-    // For a relocatable link, only tombstone values are applied.
-    if (config->relocatable)
+    // For a relocatable link, content relocated by RELA remains unchanged and
+    // we can stop here, while content relocated by REL needs updating for
+    // computed implicit addends.
+    if (config->relocatable && (RelTy::IsRela || sym.type != STT_SECTION))
       continue;

lld/ELF/InputSection.cpp

…debug_* referencing STT_SECTION symbols (llvm#66804) https://reviews.llvm.org/D48929 updated addends for non-SHF_ALLOC sections relocated by REL for -r links, but the patch did not update the addends when --compress-debug-sections={zlib,zstd} is used (llvm#66738). https://reviews.llvm.org/D116946 handled tombstone values in debug sections in relocatable links. As a side effect, both relocateNonAllocForRelocatable (using `sec->relocations`) and relocatenonNonAlloc (using raw REL/RELA) may run. Actually, we can adjust the condition in relocatenonAlloc to completely replace relocateNonAllocForRelocatable. This patch implements this idea and fixes llvm#66738. As relocateNonAlloc processes the raw relocations like copyRelocations() does, the condition `if (config->relocatable && type != target.noneRel)` in `copyRelocations` (commit 08d6a3f, modified by https://reviews.llvm.org/D62052) can be made specific to SHF_ALLOC sections. As a side effect, we can now report diagnostics for PC-relative relocations for -r. This is a less useful diagnostic that is not worth too much code. As ClangBuiltLinux/linux#1937 has violations, just suppress the warning for -r. Tested by commit 561b98f.

…debug_* referencing STT_SECTION symbols (#66804) https://reviews.llvm.org/D48929 updated addends for non-SHF_ALLOC sections relocated by REL for -r links, but the patch did not update the addends when --compress-debug-sections={zlib,zstd} is used (#66738). https://reviews.llvm.org/D116946 handled tombstone values in debug sections in relocatable links. As a side effect, both relocateNonAllocForRelocatable (using `sec->relocations`) and relocatenonNonAlloc (using raw REL/RELA) may run. Actually, we can adjust the condition in relocatenonAlloc to completely replace relocateNonAllocForRelocatable. This patch implements this idea and fixes #66738. As relocateNonAlloc processes the raw relocations like copyRelocations() does, the condition `if (config->relocatable && type != target.noneRel)` in `copyRelocations` (commit 08d6a3f, modified by https://reviews.llvm.org/D62052) can be made specific to SHF_ALLOC sections. As a side effect, we can now report diagnostics for PC-relative relocations for -r. This is a less useful diagnostic that is not worth too much code. As ClangBuiltLinux/linux#1937 has violations, just suppress the warning for -r. Tested by commit 561b98f.

MaskRay · 2023-09-20T21:52:12Z

Closed by f5b42ea

--compress-sections <section-glib>=[none|zlib|zstd] is similar to --compress-debug-sections but applies to broader sections without the SHF_ALLOC flag. lld will report an error if a SHF_ALLOC section is matched. An interesting use case is to compress `.strtab`/`.symtab`, which consume a significant portion of the file size (15.1% for a release build of Clang). An older revision is available at https://reviews.llvm.org/D154641 . This patch focuses on non-allocated sections for safety. Moving `maybeCompress` as D154641 does not handle STT_SECTION symbols for `-r --compress-debug-sections=zlib` (see `relocatable-section-symbol.s` from #66804). Since different output sections may use different compression algorithms, we need CompressedData::type to generalize config->compressDebugSections. GNU ld feature request: https://sourceware.org/bugzilla/show_bug.cgi?id=27452 Link: https://discourse.llvm.org/t/rfc-compress-arbitrary-sections-with-ld-lld-compress-sections/71674 Pull Request: #84855

MaskRay requested review from smithp35, arichardson and nickdesaulniers September 19, 2023 19:06

llvmbot added lld lld:ELF labels Sep 19, 2023

MaskRay mentioned this pull request Sep 19, 2023

lld/ELF -r: implicit addends in .rel.debug_* are incorrect if --compress-debug-sections={zlib,zstd} #66738

Closed

MaskRay requested a review from igorkudrin September 19, 2023 19:08

nickdesaulniers requested changes Sep 19, 2023

View reviewed changes

MaskRay mentioned this pull request Sep 19, 2023

ld.lld: warning: vmlinux.a(drivers/i2c/busses/i2c-i801.o):(.discard.retpoline_safe+0x120): has non-ABS relocation R_386_PC32 against symbol '' ClangBuiltLinux/linux#1937

Closed

nickdesaulniers requested changes Sep 19, 2023

View reviewed changes

nickdesaulniers approved these changes Sep 19, 2023

View reviewed changes

MaskRay force-pushed the lld/rel branch from a215a59 to de76a42 Compare September 20, 2023 04:19

arichardson approved these changes Sep 20, 2023

View reviewed changes

lld/ELF/InputSection.cpp Show resolved Hide resolved

smithp35 reviewed Sep 20, 2023

View reviewed changes

MaskRay force-pushed the lld/rel branch 3 times, most recently from c4265cc to 8462388 Compare September 20, 2023 17:28

MaskRay force-pushed the lld/rel branch from 8462388 to ca454af Compare September 20, 2023 17:45

arichardson reviewed Sep 20, 2023

View reviewed changes

lld/ELF/InputSection.cpp Outdated Show resolved Hide resolved

arichardson approved these changes Sep 20, 2023

View reviewed changes

MaskRay force-pushed the lld/rel branch from ca454af to 7ffdb74 Compare September 20, 2023 21:33

MaskRay force-pushed the lld/rel branch from 7ffdb74 to 0fb808c Compare September 20, 2023 21:43

MaskRay force-pushed the lld/rel branch from 0fb808c to f93c401 Compare September 20, 2023 21:48

MaskRay changed the title ~~[ELF] -r --compress-debug-sections: update addends for .debug_* sections relocated by REL~~ [ELF] -r --compress-debug-sections: update implicit addends for .rel.debug_* referencing STT_SECTION symbols (#66804) Sep 20, 2023

MaskRay changed the title ~~[ELF] -r --compress-debug-sections: update implicit addends for .rel.debug_* referencing STT_SECTION symbols (#66804)~~ [ELF] -r --compress-debug-sections: update implicit addends for .rel.debug_* referencing STT_SECTION symbols Sep 20, 2023

MaskRay closed this Sep 20, 2023

MaskRay deleted the lld/rel branch September 20, 2023 21:52

kstoimenov mentioned this pull request Sep 22, 2023

Add memcpm test kstoimenov/llvm-project#13

Closed

MaskRay mentioned this pull request Mar 12, 2024

[ELF] Add --compress-section to compress matched non-SHF_ALLOC sections #84855

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ELF] -r --compress-debug-sections: update implicit addends for .rel.debug_* referencing STT_SECTION symbols #66804

[ELF] -r --compress-debug-sections: update implicit addends for .rel.debug_* referencing STT_SECTION symbols #66804

MaskRay commented Sep 19, 2023 •

edited

llvmbot commented Sep 19, 2023 •

edited

nickdesaulniers commented Sep 19, 2023

nickdesaulniers commented Sep 19, 2023

nickdesaulniers commented Sep 19, 2023

nickdesaulniers left a comment

MaskRay commented Sep 19, 2023 •

edited

nickdesaulniers left a comment •

edited

MaskRay commented Sep 20, 2023

arichardson left a comment

smithp35 left a comment

smithp35 Sep 20, 2023

MaskRay Sep 20, 2023

smithp35 Sep 20, 2023

MaskRay commented Sep 20, 2023

MaskRay commented Sep 20, 2023

[ELF] -r --compress-debug-sections: update implicit addends for .rel.debug_* referencing STT_SECTION symbols #66804

[ELF] -r --compress-debug-sections: update implicit addends for .rel.debug_* referencing STT_SECTION symbols #66804

Conversation

MaskRay commented Sep 19, 2023 • edited

llvmbot commented Sep 19, 2023 • edited

nickdesaulniers commented Sep 19, 2023

nickdesaulniers commented Sep 19, 2023

nickdesaulniers commented Sep 19, 2023

nickdesaulniers left a comment

Choose a reason for hiding this comment

MaskRay commented Sep 19, 2023 • edited

nickdesaulniers left a comment • edited

Choose a reason for hiding this comment

MaskRay commented Sep 20, 2023

arichardson left a comment

Choose a reason for hiding this comment

smithp35 left a comment

Choose a reason for hiding this comment

smithp35 Sep 20, 2023

Choose a reason for hiding this comment

MaskRay Sep 20, 2023

Choose a reason for hiding this comment

smithp35 Sep 20, 2023

Choose a reason for hiding this comment

MaskRay commented Sep 20, 2023

MaskRay commented Sep 20, 2023

MaskRay commented Sep 19, 2023 •

edited

llvmbot commented Sep 19, 2023 •

edited

MaskRay commented Sep 19, 2023 •

edited

nickdesaulniers left a comment •

edited