[llvm-objcopy][ELF] Preserve sh_link to .symtab when applicable

This change to llvm-objcopy preserves the ELF section sh_link to .symtab
so long as none of the symbol table indices have been changed.
Previously, any invocation of llvm-objcopy including a "no-op" would
clear any section sh_link to .symtab.

Differential Revision: https://reviews.llvm.org/D150859

Author: nga888

lld/test/ELF/icf-safe.s

@@ -2,13 +2,17 @@
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
 # RUN: llvm-objcopy %t1.o %t1copy.o
+# RUN: llvm-objcopy --localize-symbol=h1 %t1.o %t1changed.o
+# RUN: ld.lld -r %t1.o -o %t1reloc.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %S/Inputs/icf-safe.s -o %t2.o
 # RUN: ld.lld %t1.o %t2.o -o %t2 --icf=safe --print-icf-sections | FileCheck %s
+# RUN: ld.lld %t1copy.o %t2.o -o %t2 --icf=safe --print-icf-sections | FileCheck %s
 # RUN: ld.lld %t1.o %t2.o -o %t3 --icf=safe --print-icf-sections -shared | FileCheck --check-prefix=EXPORT %s
 # RUN: ld.lld %t1.o %t2.o -o %t3 --icf=safe --print-icf-sections --export-dynamic | FileCheck --check-prefix=EXPORT %s
 # RUN: ld.lld %t1.o %t2.o -o %t2 --icf=all --print-icf-sections | FileCheck --check-prefix=ALL %s
 # RUN: ld.lld %t1.o %t2.o -o %t2 --icf=all --print-icf-sections --export-dynamic | FileCheck --check-prefix=ALL-EXPORT %s
-# RUN: ld.lld %t1copy.o -o %t4 --icf=safe 2>&1 | FileCheck --check-prefix=OBJCOPY %s
+# RUN: ld.lld %t1changed.o -o %t4 --icf=safe 2>&1 | FileCheck --check-prefix=SH_LINK_0 %s
+# RUN: ld.lld %t1reloc.o -o %t4 --icf=safe 2>&1 | FileCheck --check-prefix=SH_LINK_0 %s
 
 # CHECK-NOT: selected section {{.*}}:(.text.f1)
 # CHECK: selected section {{.*}}:(.text.f3)
@@ -89,7 +93,7 @@
 # ALL-EXPORT:   removing identical section {{.*}}:(.text.non_addrsig2)
 # ALL-EXPORT-NOT: selected section
 
-# OBJCOPY: --icf=safe conservatively ignores SHT_LLVM_ADDRSIG [index [[#]]] with sh_link=0 (likely created using objcopy or ld -r)
+# SH_LINK_0: --icf=safe conservatively ignores SHT_LLVM_ADDRSIG [index [[#]]] with sh_link=0 (likely created using objcopy or ld -r)
 
 .section .text.f1,"ax",@progbits
 .globl f1

llvm/lib/ObjCopy/ELF/ELFObject.cpp

@@ -429,6 +429,13 @@ Error Section::accept(MutableSectionVisitor &Visitor) {
   return Visitor.visit(*this);
 }
 
+void Section::restoreSymTabLink(SymbolTableSection &SymTab) {
+  if (HasSymTabLink) {
+    assert(LinkSection == nullptr);
+    LinkSection = &SymTab;
+  }
+}
+
 Error SectionWriter::visit(const OwnedDataSection &Sec) {
   llvm::copy(Sec.Data, Out.getBufferStart() + Sec.Offset);
   return Error::success();
@@ -680,8 +687,11 @@ bool Symbol::isCommon() const { return getShndx() == SHN_COMMON; }
 
 void SymbolTableSection::assignIndices() {
   uint32_t Index = 0;
-  for (auto &Sym : Symbols)
+  for (auto &Sym : Symbols) {
+    if (Sym->Index != Index)
+      IndicesChanged = true;
     Sym->Index = Index++;
+  }
 }
 
 void SymbolTableSection::addSymbol(Twine Name, uint8_t Bind, uint8_t Type,
@@ -741,7 +751,10 @@ Error SymbolTableSection::removeSymbols(
       std::remove_if(std::begin(Symbols) + 1, std::end(Symbols),
                      [ToRemove](const SymPtr &Sym) { return ToRemove(*Sym); }),
       std::end(Symbols));
+  auto PrevSize = Size;
   Size = Symbols.size() * EntrySize;
+  if (Size < PrevSize)
+    IndicesChanged = true;
   assignIndices();
   return Error::success();
 }
@@ -1106,8 +1119,10 @@ Error Section::initialize(SectionTableRef SecTable) {
 
   LinkSection = *Sec;
 
-  if (LinkSection->Type == ELF::SHT_SYMTAB)
+  if (LinkSection->Type == ELF::SHT_SYMTAB) {
+    HasSymTabLink = true;
     LinkSection = nullptr;
+  }
 
   return Error::success();
 }
@@ -2515,6 +2530,12 @@ template <class ELFT> Error ELFWriter<ELFT>::finalize() {
   if (Error E = removeUnneededSections(Obj))
     return E;
 
+  // If the .symtab indices have not been changed, restore the sh_link to
+  // .symtab for sections that were linked to .symtab.
+  if (Obj.SymbolTable && !Obj.SymbolTable->indicesChanged())
+    for (SectionBase &Sec : Obj.sections())
+      Sec.restoreSymTabLink(*Obj.SymbolTable);
+
   // We need to assign indexes before we perform layout because we need to know
   // if we need large indexes or not. We can assign indexes first and check as
   // we go to see if we will actully need large indexes.

llvm/lib/ObjCopy/ELF/ELFObject.h

@@ -432,6 +432,8 @@ class SectionBase {
   virtual bool hasContents() const { return false; }
   // Notify the section that it is subject to removal.
   virtual void onRemove();
+
+  virtual void restoreSymTabLink(SymbolTableSection &) {}
 };
 
 class Segment {
@@ -483,6 +485,7 @@ class Section : public SectionBase {
 
   ArrayRef<uint8_t> Contents;
   SectionBase *LinkSection = nullptr;
+  bool HasSymTabLink = false;
 
 public:
   explicit Section(ArrayRef<uint8_t> Data) : Contents(Data) {}
@@ -497,6 +500,7 @@ class Section : public SectionBase {
   bool hasContents() const override {
     return Type != ELF::SHT_NOBITS && Type != ELF::SHT_NULL;
   }
+  void restoreSymTabLink(SymbolTableSection &SymTab) override;
 };
 
 class OwnedDataSection : public SectionBase {
@@ -691,6 +695,7 @@ class SymbolTableSection : public SectionBase {
   std::vector<std::unique_ptr<Symbol>> Symbols;
   StringTableSection *SymbolNames = nullptr;
   SectionIndexSection *SectionIndexTable = nullptr;
+  bool IndicesChanged = false;
 
   using SymPtr = std::unique_ptr<Symbol>;
 
@@ -703,6 +708,7 @@ class SymbolTableSection : public SectionBase {
   void prepareForLayout();
   // An 'empty' symbol table still contains a null symbol.
   bool empty() const { return Symbols.size() == 1; }
+  bool indicesChanged() const { return IndicesChanged; }
   void setShndxTable(SectionIndexSection *ShndxTable) {
     SectionIndexTable = ShndxTable;
   }

llvm/test/tools/llvm-objcopy/ELF/symtab-link.test

@@ -1,9 +1,31 @@
 # RUN: yaml2obj %s -o %t
+## No-op copy.
 # RUN: llvm-objcopy %t %t2
 # RUN: llvm-readobj --sections %t2 | FileCheck %s
+## No-op strip.
 # RUN: cp %t %t3
 # RUN: llvm-strip --no-strip-all %t3
 # RUN: llvm-readobj --sections %t3 | FileCheck %s
+## Add symbol.
+# RUN: llvm-objcopy --add-symbol=another=.text:0,function %t %t4
+# RUN: llvm-readobj --sections %t4 | FileCheck %s
+
+## A section with sh_link referencing SHT_SYMTAB indicates that its content may
+## use the old symbol indices. If the symbol indices change, reset sh_link to 0
+## to inform tools like linkers that the sh_link has been invalidated.
+
+## Strip first symbol.
+# RUN: llvm-objcopy --strip-symbol bar %t %t5
+# RUN: llvm-readobj --sections %t5 | FileCheck %s --check-prefix=LINK-0
+## Strip last symbol.
+# RUN: llvm-objcopy --strip-symbol baz %t %t6
+# RUN: llvm-readobj --sections %t6 | FileCheck %s --check-prefix=LINK-0
+## Re-order symbols.
+# RUN: llvm-objcopy --localize-symbol baz %t %t7
+# RUN: llvm-readobj --sections %t7 | FileCheck %s --check-prefix=LINK-0
+## Remove .text section.
+# RUN: llvm-objcopy --remove-section=.text %t %t8
+# RUN: llvm-readobj --sections %t8 | FileCheck %s --check-prefix=LINK-0
 
 !ELF
 FileHeader:
@@ -12,11 +34,29 @@ FileHeader:
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x0000000000000010
+    Size:            32
   - Name:            .foo
     Link:            .symtab
     Type:            SHT_PROGBITS
     Flags:           [ ]
-Symbols: []
+Symbols:
+  - Name:     bar
+    Type:     STT_FUNC
+    Size:     8
+    Section:  .text
+    Value:    0x1000
+    Binding:  STB_GLOBAL
+  - Name:     baz
+    Type:     STT_FUNC
+    Size:     8
+    Section:  .text
+    Value:    0x1010
+    Binding:  STB_GLOBAL
 
 # CHECK:      Name: .foo
 # CHECK-NEXT: Type:
@@ -25,4 +65,17 @@ Symbols: []
 # CHECK-NEXT: Address:
 # CHECK-NEXT: Offset:
 # CHECK-NEXT: Size:
-# CHECK-NEXT: Link: 0
+# CHECK-NEXT: Link: [[#SYMTABIDX:]]
+
+# CHECK:      Index: [[#SYMTABIDX]]
+# CHECK-NEXT: Name: .symtab
+# CHECK-NEXT: Type: SHT_SYMTAB
+
+# LINK-0:      Name: .foo
+# LINK-0-NEXT: Type:
+# LINK-0-NEXT: Flags [ (0x0)
+# LINK-0-NEXT: ]
+# LINK-0-NEXT: Address:
+# LINK-0-NEXT: Offset:
+# LINK-0-NEXT: Size:
+# LINK-0-NEXT: Link: 0