[llvm-objcopy][MachO] Handle relocations where r_extern is 0

[alexander-shaposhnikov]

Summary

Fix handling of relocations with r_extern == 0.
If r_extern == 0 then r_symbolnum is an index of a section rather than a symbol index.

This diff is essentially an updated version of https://reviews.llvm.org/D71647 by @seiya,
the code has been slightly refactored, the test has been converted to YAML.
This change depends on https://reviews.llvm.org/D78898.

Test plan: make check-all

[smeenai]

LGTM from the Mach-O side.

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

221

Should this be a user-visible error instead? It can happen for a malformed input file, for example.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

221

yup, it should, i looked into this yesterday, so the interface of Reader needs some adjustments (to improve error handling). I'd prefer to add FIXME or TODO, this diff (I mean @seiya's) has been around for so long and it's really blocking me now in many ways, so would be good to commit it and refactor error handling on a separate diff.

[MaskRay]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

107

!R.Scattered && MachOObj.getPlainRelocationExternal(R.Info)

llvm/tools/llvm-objcopy/MachO/Object.cpp

63

*R.Symbol

[smeenai]

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

221

Sure, adding a TODO or FIXME and looking into this in a follow-up is good with me.

[alexander-shaposhnikov]

minor cleanup

[alexander-shaposhnikov]

cleanup

[alexander-shaposhnikov]

Fix typo.

[seiya]

Thank you for the fix, @alexshap!

llvm/test/tools/llvm-objcopy/MachO/relocations.test

+# RUN: yaml2obj %s -o %t
+
+## Show that llvm-objcopy copies relocation entries where r_extern = 0.
+# RUN: llvm-objcopy %t %t2
+# RUN: cmp %t %t2
+
+## Show that llvm-objcopy updates section indices properly.
+# RUN: llvm-objcopy --remove-section=__DATA,__foo %t %t3
+# RUN: llvm-objdump --macho --reloc %t3 | FileCheck %s
+
+# CHECK:      Relocation information (__DATA,__bar) 2 entries
+# CHECK-NEXT: address  pcrel length extern type    scattered symbolnum/value
+# CHECK-NEXT: 00000000 False quad   False  SUB     False     2 (__DATA,__bar)
+# CHECK-NEXT: 00000000 False quad   False  UNSIGND False     1 (__TEXT,__text)
+
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      312
+  flags:           0x00000000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         312
+    segname:         ''
+    vmaddr:          0
+    vmsize:          24
+    fileoff:         344
+    filesize:        24
+    maxprot:         7
+    initprot:        7
+    nsects:          3
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        size:            8
+        offset:          0x00000158
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         '0000000000000000'
+      - sectname:        __foo
+        segname:         __DATA
+        addr:            0x0000000000000008
+        size:            8
+        offset:          0x00000160
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         '0000000000000000'
+      - sectname:        __bar
+        segname:         __DATA
+        addr:            0x0000000000000010
+        size:            8
+        offset:          0x00000168
+        align:           0
+        reloff:          0x00000170
+        nreloc:          2
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         F0FFFFFFFFFFFFFF
+        relocations:
+          - address:         0x00000000
+            symbolnum:       3
+            pcrel:           false
+            length:          3
+            extern:          false
+            type:            5
+            scattered:       false
+            value:           0
+          - address:         0x00000000
+            symbolnum:       1
+            pcrel:           false
+            length:          3
+            extern:          false
+            type:            0
+            scattered:       false
+            value:           0
+...

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp

for (; reinterpret_cast<const void *>(Curr) < End; Curr++) {
     S.Relocations.reserve(S.NReloc);
     for (auto RI = MachOObj.section_rel_begin(SecRef->getRawDataRefImpl()),
               RE = MachOObj.section_rel_end(SecRef->getRawDataRefImpl());
          RI != RE; ++RI) {
       RelocationInfo R;
       R.Symbol = nullptr; // We'll fill this field later.
       R.Info = MachOObj.getRelocation(RI->getRawDataRefImpl());
       R.Scattered = MachOObj.isRelocationScattered(R.Info);
+      R.Extern = !R.Scattered && MachOObj.getPlainRelocationExternal(R.Info);
       S.Relocations.push_back(R);
     }
 
     assert(S.NReloc == S.Relocations.size() &&
            "Incorrect number of relocations");
   }
   return Sections;
 }
 
SymbolEntry SE =
              : constructSymbolEntry(StrTable, MachOObj.getSymbolTableEntry(
                                                   Symbol.getRawDataRefImpl())));
 
     O.SymTable.Symbols.push_back(std::make_unique<SymbolEntry>(SE));
   }
 }
 
 void MachOReader::setSymbolInRelocationInfo(Object &O) const {
+  std::vector<const Section *> Sections;
+  for (auto &LC : O.LoadCommands)
+    for (std::unique_ptr<Section> &Sec : LC.Sections)
+      Sections.push_back(Sec.get());
+
   for (LoadCommand &LC : O.LoadCommands)
     for (std::unique_ptr<Section> &Sec : LC.Sections)
       for (auto &Reloc : Sec->Relocations)
-        if (!Reloc.Scattered)
-          Reloc.Symbol = O.SymTable.getSymbolByIndex(
-              Reloc.getPlainRelocationSymbolNum(MachOObj.isLittleEndian()));
+        if (!Reloc.Scattered) {
+          const uint32_t SymbolNum =
+              Reloc.getPlainRelocationSymbolNum(MachOObj.isLittleEndian());
+          if (Reloc.Extern) {
+            Reloc.Symbol = O.SymTable.getSymbolByIndex(SymbolNum);
+          } else {
+            // FIXME: Refactor error handling in MachOReader and report an error
+            // if we encounter an invalid relocation.
+            assert(SymbolNum >= 1 && SymbolNum <= Sections.size() &&
+                   "Invalid section index.");
+            Reloc.Section = Sections[SymbolNum - 1];
+          }
+        }
 }
 
 void MachOReader::readRebaseInfo(Object &O) const {
   O.Rebases.Opcodes = MachOObj.getDyldInfoRebaseOpcodes();
 }
 
 void MachOReader::readBindInfo(Object &O) const {
   O.Binds.Opcodes = MachOObj.getDyldInfoBindOpcodes();

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

for (const std::unique_ptr<Section> &Sec : LC.Sections) {
       if (Sec->isVirtualSection())
         continue;
 
       assert(Sec->Offset && "Section offset can not be zero");
       assert((Sec->Size == Sec->Content.size()) && "Incorrect section size");
       memcpy(B.getBufferStart() + Sec->Offset, Sec->Content.data(),
              Sec->Content.size());
       for (size_t Index = 0; Index < Sec->Relocations.size(); ++Index) {
-        auto RelocInfo = Sec->Relocations[Index];
-        if (!RelocInfo.Scattered)
-          RelocInfo.setPlainRelocationSymbolNum(RelocInfo.Symbol->Index,
-                                                IsLittleEndian);
-
+        RelocationInfo RelocInfo = Sec->Relocations[Index];
+        if (!RelocInfo.Scattered) {
+          const uint32_t SymbolNum = RelocInfo.Extern
+                                         ? (*RelocInfo.Symbol)->Index
+                                         : (*RelocInfo.Section)->Index;
+          RelocInfo.setPlainRelocationSymbolNum(SymbolNum, IsLittleEndian);
+        }
         if (IsLittleEndian != sys::IsLittleEndianHost)
           MachO::swapStruct(
               reinterpret_cast<MachO::any_relocation_info &>(RelocInfo.Info));
         memcpy(B.getBufferStart() + Sec->RelOff +
                    Index * sizeof(MachO::any_relocation_info),
                &RelocInfo.Info, sizeof(RelocInfo.Info));
       }
     }

llvm/tools/llvm-objcopy/MachO/Object.h

 
 /// The location of the string table inside the binary is described by LC_SYMTAB
 /// load command.
 struct StringTable {
   std::vector<std::string> Strings;
 };
 
 struct RelocationInfo {
-  const SymbolEntry *Symbol;
+  // The referenced symbol entry. Set if !Scattered && Extern.
+  Optional<const SymbolEntry *> Symbol;
+  // The referenced section. Set if !Scattered && !Extern.
+  Optional<const Section *> Section;
   // True if Info is a scattered_relocation_info.
   bool Scattered;
+  // True if the r_symbolnum points to a section number (i.e. r_extern=0).
+  bool Extern;
   MachO::any_relocation_info Info;
 
   unsigned getPlainRelocationSymbolNum(bool IsLittleEndian) {
     if (IsLittleEndian)
       return Info.r_word1 & 0xffffff;
     return Info.r_word1 >> 8;
   }
 

llvm/tools/llvm-objcopy/MachO/Object.cpp

Error Object::removeSections(
   SmallPtrSet<const SymbolEntry *, 2> DeadSymbols;
   for (const std::unique_ptr<SymbolEntry> &Sym : SymTable.Symbols)
     if (IsDead(Sym))
       DeadSymbols.insert(Sym.get());
 
   for (const LoadCommand &LC : LoadCommands)
     for (const std::unique_ptr<Section> &Sec : LC.Sections)
       for (const RelocationInfo &R : Sec->Relocations)
-        if (R.Symbol && DeadSymbols.count(R.Symbol))
+        if (R.Symbol && *R.Symbol && DeadSymbols.count(*R.Symbol))
           return createStringError(std::errc::invalid_argument,
                                    "symbol '%s' defined in section with index "
                                    "'%u' cannot be removed because it is "
                                    "referenced by a relocation in section '%s'",
-                                   R.Symbol->Name.c_str(),
-                                   *(R.Symbol->section()),
+                                   (*R.Symbol)->Name.c_str(),
+                                   *((*R.Symbol)->section()),
                                    Sec->CanonicalName.c_str());
   SymTable.removeSymbols(IsDead);
   for (std::unique_ptr<SymbolEntry> &S : SymTable.Symbols)
     if (S->section())
       S->n_sect = OldIndexToSection[S->n_sect]->Index;
   return Error::success();
 }