[llvm-objcopy] - Fix --compress-debug-sections when there are relocations.

[grimar]

Summary

When --compress-debug-sections is given,
llvm-objcopy removes the uncompressed sections and adds compressed to the section list.
This makes all the pointers to old sections to be outdated.

Currently, code already has logic for replacing the target sections of the relocation
sections. But we also have to update the relocations by themselves.

This fixes https://bugs.llvm.org/show_bug.cgi?id=40885.

[jhenderson]

test/tools/llvm-objcopy/ELF/compress-debug-sections-relocations.test

1 ↗

(On Diff #189300)

Could you reuse the Inputs/compress-debug-sections.yaml input file? Does that cover enough here, if updated with an actual symbol (see the update to that file in D58510)? I vaguely remember requesting that the relocation existed, so that it would test relocation behaviour, but it turns out the yaml was not quite correct.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

257–261

"update target" -> "update the target"

258

itself -> themselves.

Perhaps also worth mentioning how you are updating them (i.e. updating the symbol references).

tools/llvm-objcopy/ELF/Object.h

601

A more in-keeping with the llvm-objcopy Object design might be to provide a do-nothing virtual function for all sections "replaceSectionReferences" or similar, and then override it for section types that actually need some work, like relocation sections.

In particular, I could imagine some issues with things like group sections referencing debug data or similar.

[grimar]

• Addressed review comments.

test/tools/llvm-objcopy/ELF/compress-debug-sections-relocations.test

1 ↗

(On Diff #189300)

Done.

tools/llvm-objcopy/ELF/Object.h

601

Reasonable. Done.

[grimar]

test/tools/llvm-objcopy/ELF/Inputs/compress-debug-sections.yaml

20

I thought about adding one more section .debug_bar and do something like:

- Offset:          0x3
  Symbol:          .debug_bar
  Type:            R_X86_64_32

i.e. to show that if relocation section .rela.X has target X and relocation that references Y,
where X and Y are replaced debug section then we handle it correctly.

But I am not sure it worth doing that actually since the existent test covers the all code
added I think. I.e. it tests that if relocation section .rela.X has target X and relocation that references X,
where X is a replaced debug section then we handle it correctly.
Since both the code pieces that fixes section target and relocations themselves are different and both used,
maybe it is already enough here.

[jhenderson]

This LGTM, but I'd like to get a thumbs up from @jakehehrlich or @rupprecht to confirm that the replaceSectionReferences method makes sense to them too.

[jhenderson]

In D58960#1418376, @jhenderson wrote:

This LGTM, but I'd like to get a thumbs up from @jakehehrlich or @rupprecht to confirm that the replaceSectionReferences method makes sense to them too.

Oh, or @alexshap too (sorry Alex!).

[jakehehrlich]

This looks good to me but it is perhaps confusing that the replaceSectionReferences method is unimplemented for many types where it makes sense. Implementing it for all types makes --only-keep-debug possible for instance.

tools/llvm-objcopy/ELF/Object.h

284

Nice! This has actually been a blocker for correctly implementing --only-keep-debug anyhow.

[jhenderson]

It's probably worth adding a TODO or filing an enhancement bug for the other section types this makes sense for, e.g. symbol tables (to move symbols to the new section) or group sections (update referred-to symbol table/change group contents etc).

[grimar]

In D58960#1424252, @jhenderson wrote:

It's probably worth adding a TODO or filing an enhancement bug for the other section types this makes sense for, e.g. symbol tables (to move symbols to the new section) or group sections (update referred-to symbol table/change group contents etc).

I need a bit more time to investigate these scenarios. I'll try to take a look during this week.

[jhenderson]

In D58960#1424259, @grimar wrote:

In D58960#1424252, @jhenderson wrote:

It's probably worth adding a TODO or filing an enhancement bug for the other section types this makes sense for, e.g. symbol tables (to move symbols to the new section) or group sections (update referred-to symbol table/change group contents etc).

I need a bit more time to investigate these scenarios. I'll try to take a look during this week.

Great, cool. No need for that to block this landing though necessarily.

test/tools/llvm-objcopy/ELF/Inputs/compress-debug-sections.yaml

- Name:            .notdebug_foo
     Type:            SHT_PROGBITS
     Content:         0000000000000000
   - Name:            .rela.debug_foo
     Type:            SHT_RELA
     Info:            .debug_foo
     Relocations:
       - Offset:          0x1
         Symbol:          .debug_foo
         Type:            R_X86_64_32
+      - Offset:          0x2
+        Symbol:          .notdebug_foo
+        Type:            R_X86_64_32
+Symbols:
+  Global:
+    - Name:    .debug_foo
+      Type:    STT_SECTION
+      Section: .debug_foo
+    - Name:    .notdebug_foo
+      Type:    STT_SECTION
+      Section: .notdebug_foo
 ...

test/tools/llvm-objcopy/ELF/compress-debug-sections-zlib-gnu.test

 # CHECK-FLAGS-NEXT: Address:
 # CHECK-FLAGS-NEXT: Offset:
 # CHECK-FLAGS-NEXT: Size:
 # CHECK-FLAGS-NEXT: Link:
 # CHECK-FLAGS-NEXT: Info: 1
 
 # CHECK-FLAGS: Relocations [
 # CHECK-FLAGS-NEXT:   .rela.debug_foo {
-# CHECK-FLAGS-NEXT:     0x1 R_X86_64_32 - 0x0
+# CHECK-FLAGS-NEXT:     0x1 R_X86_64_32 .zdebug_foo 0x0
+# CHECK-FLAGS-NEXT:     0x2 R_X86_64_32 .notdebug_foo 0x0
 # CHECK-FLAGS-NEXT:   }
 # CHECK-FLAGS-NEXT: ]
 

test/tools/llvm-objcopy/ELF/compress-debug-sections-zlib.test

 # CHECK-FLAGS-NEXT: Address:
 # CHECK-FLAGS-NEXT: Offset:
 # CHECK-FLAGS-NEXT: Size:
 # CHECK-FLAGS-NEXT: Link:
 # CHECK-FLAGS-NEXT: Info: 1
 
 # CHECK-FLAGS: Relocations [
 # CHECK-FLAGS-NEXT:   .rela.debug_foo {
-# CHECK-FLAGS-NEXT:     0x1 R_X86_64_32 - 0x0
+# CHECK-FLAGS-NEXT:     0x1 R_X86_64_32 .debug_foo 0x0
+# CHECK-FLAGS-NEXT:     0x2 R_X86_64_32 .notdebug_foo 0x0
 # CHECK-FLAGS-NEXT:   }
 # CHECK-FLAGS-NEXT: ]
-

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

static bool isCompressable(const SectionBase &Section) {
   return !isCompressed(Section) && isDebugSection(Section) &&
          Section.Name != ".gdb_index";
 }
 
 static void replaceDebugSections(
     const CopyConfig &Config, Object &Obj, SectionPred &RemovePred,
     function_ref<bool(const SectionBase &)> shouldReplace,
     function_ref<SectionBase *(const SectionBase *)> addSection) {
+  // Build a list of the debug sections we are going to replace.
+  // We can't call `addSection` while iterating over sections,
+  // because it would mutate the sections array.
   SmallVector<SectionBase *, 13> ToReplace;
-  SmallVector<RelocationSection *, 13> RelocationSections;
-  for (auto &Sec : Obj.sections()) {
-    if (RelocationSection *R = dyn_cast<RelocationSection>(&Sec)) {
-      if (shouldReplace(*R->getSection()))
-        RelocationSections.push_back(R);
-      continue;
-    }
-
+  for (auto &Sec : Obj.sections())
     if (shouldReplace(Sec))
       ToReplace.push_back(&Sec);
-  }
-
-  for (SectionBase *S : ToReplace) {
-    SectionBase *NewSection = addSection(S);
 
-    for (RelocationSection *RS : RelocationSections) {
-      if (RS->getSection() == S)
-        RS->setSection(NewSection);
-    }
-  }
+  // Build a mapping from original section to a new one.
+  DenseMap<SectionBase *, SectionBase *> FromTo;
+  for (SectionBase *S : ToReplace)
+    FromTo[S] = addSection(S);
+
+  // Now we want to update the target sections of relocation
+  // sections. Also we will update the relocations themselves
+  // to update the symbol references.
+  for (auto &Sec : Obj.sections())
+    Sec.replaceSectionReferences(FromTo);
 
   RemovePred = [shouldReplace, RemovePred](const SectionBase &Sec) {
     return shouldReplace(Sec) || RemovePred(Sec);
   };
 }
 
 static bool isUnneededSymbol(const Symbol &Sym) {
   return !Sym.Referenced &&

tools/llvm-objcopy/ELF/Object.h

public:
   virtual void finalize();
   // Remove references to these sections. The list of sections must be sorted.
   virtual Error
   removeSectionReferences(function_ref<bool(const SectionBase *)> ToRemove);
   virtual Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove);
   virtual void accept(SectionVisitor &Visitor) const = 0;
   virtual void accept(MutableSectionVisitor &Visitor) = 0;
   virtual void markSymbols();
+  virtual void
+  replaceSectionReferences(const DenseMap<SectionBase *, SectionBase *> &);
 };
 
 class Segment {
 private:
   struct SectionCompare {
     bool operator()(const SectionBase *Lhs, const SectionBase *Rhs) const {
       // Some sections might have the same address if one of them is empty. To
       // fix this we can use the lexicographic ordering on ->Addr and the

 public:
   void addRelocation(Relocation Rel) { Relocations.push_back(Rel); }
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
   Error removeSectionReferences(
       function_ref<bool(const SectionBase *)> ToRemove) override;
   Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
   void markSymbols() override;
+  void replaceSectionReferences(
+      const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
 
   static bool classof(const SectionBase *S) {
     if (S->Flags & ELF::SHF_ALLOC)
       return false;
     return S->Type == ELF::SHT_REL || S->Type == ELF::SHT_RELA;
   }
 };
 

tools/llvm-objcopy/ELF/Object.cpp

 
 Error SectionBase::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
   return Error::success();
 }
 
 void SectionBase::initialize(SectionTableRef SecTable) {}
 void SectionBase::finalize() {}
 void SectionBase::markSymbols() {}
+void SectionBase::replaceSectionReferences(
+    const DenseMap<SectionBase *, SectionBase *> &) {}
 
 template <class ELFT> void ELFWriter<ELFT>::writeShdr(const SectionBase &Sec) {
   uint8_t *B = Buf.getBufferStart();
   B += Sec.HeaderOffset;
   Elf_Shdr &Shdr = *reinterpret_cast<Elf_Shdr *>(B);
   Shdr.sh_name = Sec.NameIndex;
   Shdr.sh_type = Sec.Type;
   Shdr.sh_flags = Sec.Flags;
Error RelocationSection::removeSymbols(
   return Error::success();
 }
 
 void RelocationSection::markSymbols() {
   for (const Relocation &Reloc : Relocations)
     Reloc.RelocSymbol->Referenced = true;
 }
 
+void RelocationSection::replaceSectionReferences(
+    const DenseMap<SectionBase *, SectionBase *> &FromTo) {
+  // Update the target section if it was replaced.
+  if (SectionBase *To = FromTo.lookup(SecToApplyRel))
+    SecToApplyRel = To;
+
+  // Change the sections where symbols are defined in if their
+  // original sections were replaced.
+  for (const Relocation &R : Relocations)
+    if (SectionBase *To = FromTo.lookup(R.RelocSymbol->DefinedIn))
+      R.RelocSymbol->DefinedIn = To;
+}
+
 void SectionWriter::visit(const DynamicRelocationSection &Sec) {
   llvm::copy(Sec.Contents,
             Out.getBufferStart() + Sec.Offset);
 }
 
 void DynamicRelocationSection::accept(SectionVisitor &Visitor) const {
   Visitor.visit(*this);
 }