[llvm-objcopy] - Use replaceSectionReferences to update the sections for symbols in symbol table.

[grimar]

Summary

If the compression was used and we had a symbol not involved in relocation,
we never updated its section and it was silently removed from the output.

[grimar]

• Rewrote the comment.

[jhenderson]

Nice catch. Should we remove the loop in RelocationSection::replaceSectionReferences that updates the symbols? I think that's unnecessary now, if we are looping over all symbols in the symbol table.

[rupprecht]

lgtm modulo trying out jhenderson's suggestion

tools/llvm-objcopy/ELF/Object.h

533 ↗

(On Diff #191284)

missing parameter name

[grimar]

• Addressed review comments.

[grimar]

In D59542#1434828, @jhenderson wrote:

Should we remove the loop in RelocationSection::replaceSectionReferences that updates the symbols? I think that's unnecessary now, if we are looping over all symbols in the symbol table.

This should work I think, thanks! Done in the new diff.

[jhenderson]

LGTM.

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

- Name:            .rela.debug_foo
     Info:            .debug_foo
     Relocations:
       - Offset:          0x1
         Symbol:          .debug_foo
         Type:            R_X86_64_32
       - Offset:          0x2
         Symbol:          .notdebug_foo
         Type:            R_X86_64_32
+## This section should not be involved in relocations.
+  - Name:            .debug_bar
+    Type:            SHT_PROGBITS
+    Content:         0000000000000000
 Symbols:
   Global:
     - Name:    .debug_foo
       Type:    STT_SECTION
       Section: .debug_foo
     - Name:    .notdebug_foo
       Type:    STT_SECTION
       Section: .notdebug_foo
+    - Name:    .Linfo_string0
+      Section: .debug_bar
 ...

llvm/trunk/test/tools/llvm-objcopy/ELF/compress-debug-sections-symbols.test

+# REQUIRES: zlib
+
+# RUN: yaml2obj %p/Inputs/compress-debug-sections.yaml -o %t.o
+
+## Test that after the compression is done we do not remove the symbol
+## and it is placed into the right section.
+
+# RUN: llvm-objcopy --compress-debug-sections %t.o %t-compressed1.o
+# RUN: llvm-readobj -symbols %t-compressed1.o | FileCheck %s --check-prefixes=CHECK,ZLIB
+
+# RUN: llvm-objcopy --compress-debug-sections=zlib-gnu %t.o %t-compressed2.o
+# RUN: llvm-readobj -symbols %t-compressed2.o | FileCheck %s --check-prefixes=CHECK,ZLIBGNU
+
+# CHECK:        Name:    .Linfo_string0
+# CHECK-NEXT:   Value:   0x0
+# CHECK-NEXT:   Size:    0
+# CHECK-NEXT:   Binding: Global
+# CHECK-NEXT:   Type:    None
+# CHECK-NEXT:   Other:   0
+# ZLIB-NEXT:    Section: .debug_bar
+# ZLIBGNU-NEXT: Section: .zdebug_bar

llvm/trunk/tools/llvm-objcopy/ELF/Object.h

public:
 
   Error removeSectionReferences(
       function_ref<bool(const SectionBase *)> ToRemove) override;
   void initialize(SectionTableRef SecTable) override;
   void finalize() override;
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
   Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
+  void replaceSectionReferences(
+      const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
 
   static bool classof(const SectionBase *S) {
     return S->Type == ELF::SHT_SYMTAB;
   }
 };
 
 struct Relocation {
   Symbol *RelocSymbol = nullptr;

llvm/trunk/tools/llvm-objcopy/ELF/Object.cpp

Symbols.erase(
       std::remove_if(std::begin(Symbols) + 1, std::end(Symbols),
                      [ToRemove](const SymPtr &Sym) { return ToRemove(*Sym); }),
       std::end(Symbols));
   Size = Symbols.size() * EntrySize;
   assignIndices();
   return Error::success();
 }
 
+void SymbolTableSection::replaceSectionReferences(
+    const DenseMap<SectionBase *, SectionBase *> &FromTo) {
+  for (std::unique_ptr<Symbol> &Sym : Symbols)
+    if (SectionBase *To = FromTo.lookup(Sym->DefinedIn))
+      Sym->DefinedIn = To;
+}
+
 void SymbolTableSection::initialize(SectionTableRef SecTable) {
   Size = 0;
   setStrTab(SecTable.getSectionOfType<StringTableSection>(
       Link,
       "Symbol table has link index of " + Twine(Link) +
           " which is not a valid index",
       "Symbol table has link index of " + Twine(Link) +
           " which is not a string table"));
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 {