[llvm-readobj] [COFF] Print the symbol index for relocations

[mstorsjo]

Summary

There can be multiple local symbols with the same name (for e.g. comdat sections), and thus the symbol name itself isn't enough to disambiguate symbols.

[mstorsjo]

Neither of the tools that print relocations currently, llvm-readobj, llvm-objdump or yaml2obj, disambiguate between relocations pointing to different symbols with the same name.

[ruiu]

include/llvm/Object/COFF.h

438 ↗

(On Diff #179663)

Instead of storing an index as a member, can you compute it when you need it from the offset from the beginning of a symbol table?

[mstorsjo]

Btw, do you think the index should be printed in hex? That's what dumpbin -relocations does.

[mstorsjo]

include/llvm/Object/COFF.h

438 ↗

(On Diff #179663)

Yes, that'd be possible, but COFFSymbolRef doesn't have a reference to either the start of the symbol table or the surrounding COFFObjectFile, so it'd have to be a getIndexOf(COFFSymbolRef) member in COFFObjectFile in that case.

[mstorsjo]

Removed the extra member from COFFSymbol, added a method to COFFObjectFile for calculating it instead.

The question about printing hex vs decimal is still open for opinions.

[alexander-shaposhnikov]

tools/llvm-readobj/COFFDumper.cpp

1381 ↗

(On Diff #179726)

one minor concern: if I understand correctly if the symbol is not found (Symbol == Obj->symbol_end()) this code still will print SymbolIndex = 0. Maybe we can do smth similar to how SymbolName is handled ?

[mstorsjo]

tools/llvm-readobj/COFFDumper.cpp

1381 ↗

(On Diff #179726)

Sure, I guess -1 is the only sensible thing to print then. The index can technically be any uint32_t, so storing it in a local int64_t to allow for -1 should be safe then.

[mstorsjo]

Updated to print -1 for cases when the symbol isn't found.

[ruiu]

LGTM

llvm/trunk/include/llvm/Object/COFF.h

public:
   }
 
   std::error_code getSymbolName(COFFSymbolRef Symbol, StringRef &Res) const;
   std::error_code getSymbolName(const coff_symbol_generic *Symbol,
                                 StringRef &Res) const;
 
   ArrayRef<uint8_t> getSymbolAuxData(COFFSymbolRef Symbol) const;
 
+  uint32_t getSymbolIndex(COFFSymbolRef Symbol) const;
+
   size_t getSymbolTableEntrySize() const {
     if (COFFHeader)
       return sizeof(coff_symbol16);
     if (COFFBigObjHeader)
       return sizeof(coff_symbol32);
     llvm_unreachable("null symbol table pointer!");
   }
 

llvm/trunk/lib/Object/COFFObjectFile.cpp

#ifndef NDEBUG
 
     assert((Offset - getPointerToSymbolTable()) % SymbolSize == 0 &&
            "Aux Symbol data did not point to the beginning of a symbol");
 #endif
   }
   return makeArrayRef(Aux, Symbol.getNumberOfAuxSymbols() * SymbolSize);
 }
 
+uint32_t COFFObjectFile::getSymbolIndex(COFFSymbolRef Symbol) const {
+  uintptr_t Offset =
+      reinterpret_cast<uintptr_t>(Symbol.getRawPtr()) - getSymbolTable();
+  assert(Offset % getSymbolTableEntrySize() == 0 &&
+         "Symbol did not point to the beginning of a symbol");
+  size_t Index = Offset / getSymbolTableEntrySize();
+  assert(Index < getNumberOfSymbols());
+  return Index;
+}
+
 std::error_code COFFObjectFile::getSectionName(const coff_section *Sec,
                                                StringRef &Res) const {
   StringRef Name;
   if (Sec->Name[COFF::NameSize - 1] == 0)
     // Null terminated, let ::strlen figure out the length.
     Name = Sec->Name;
   else
     // Not null terminated, use all 8 bytes.

llvm/trunk/test/MC/COFF/cross-section-relative.s

 // READOBJ-NEXT:  }
 // READOBJ-NEXT:  ]
 // READOBJ-NEXT:  Relocations [
 // READOBJ-NEXT:  Section (5) .fix {
 // READOBJ-NEXT:    Relocation {
 // READOBJ-NEXT:      Offset: 0x0
 // READOBJ-NEXT:      Type: IMAGE_REL_AMD64_REL32 (4)
 // READOBJ-NEXT:      Symbol: g3
+// READOBJ-NEXT:      SymbolIndex: 12
 // READOBJ-NEXT:    }
 // READOBJ-NEXT:    Relocation {
 // READOBJ-NEXT:      Offset: 0x4
 // READOBJ-NEXT:      Type: IMAGE_REL_AMD64_ADDR32NB (3)
 // READOBJ-NEXT:      Symbol: g3
+// READOBJ-NEXT:      SymbolIndex: 12
 // READOBJ-NEXT:    }
 // READOBJ-NEXT:    Relocation {
 // READOBJ-NEXT:      Offset: 0x20
 // READOBJ-NEXT:      Type: IMAGE_REL_AMD64_REL32 (4)
 // READOBJ-NEXT:      Symbol: g3
+// READOBJ-NEXT:      SymbolIndex: 12
 // READOBJ-NEXT:    }
 // READOBJ-NEXT:    Relocation {
 // READOBJ-NEXT:      Offset: 0x28
 // READOBJ-NEXT:      Type: IMAGE_REL_AMD64_REL32 (4)
 // READOBJ-NEXT:      Symbol: foobar
+// READOBJ-NEXT:      SymbolIndex: 20
 // READOBJ-NEXT:    }
 // READOBJ-NEXT:  }
 // READOBJ-NEXT:]

llvm/trunk/test/Object/yaml2obj-readobj.test

 RUN: yaml2obj %p/Inputs/COFF/i386.yaml | llvm-readobj -file-headers -relocations -expand-relocs - | FileCheck %s --check-prefix COFF-I386
 RUN: yaml2obj -o %t %p/Inputs/COFF/i386.yaml
 RUN: llvm-readobj -file-headers -relocations -expand-relocs %t \
 RUN:   | FileCheck %s --check-prefix COFF-I386
 
 // COFF-I386:  Characteristics [ (0x200)
 // COFF-I386-NEXT:    IMAGE_FILE_DEBUG_STRIPPED (0x200)
 // COFF-I386-NEXT:  ]
 
 // COFF-I386:      Relocations [
 // COFF-I386-NEXT:   Section (1) .text {
 // COFF-I386-NEXT:     Relocation {
 // COFF-I386-NEXT:       Offset: 0xE
 // COFF-I386-NEXT:       Type: IMAGE_REL_I386_DIR32 (6)
 // COFF-I386-NEXT:       Symbol: L_.str
+// COFF-I386-NEXT:       SymbolIndex: 5
 // COFF-I386-NEXT:     }
 // COFF-I386-NEXT:     Relocation {
 // COFF-I386-NEXT:       Offset: 0x13
 // COFF-I386-NEXT:       Type: IMAGE_REL_I386_REL32 (20)
 // COFF-I386-NEXT:       Symbol: _puts
+// COFF-I386-NEXT:       SymbolIndex: 6
 // COFF-I386-NEXT:     }
 // COFF-I386-NEXT:     Relocation {
 // COFF-I386-NEXT:       Offset: 0x18
 // COFF-I386-NEXT:       Type: IMAGE_REL_I386_REL32 (20)
 // COFF-I386-NEXT:       Symbol: _SomeOtherFunction
+// COFF-I386-NEXT:       SymbolIndex: 7
 // COFF-I386-NEXT:     }
 // COFF-I386-NEXT:   }
 // COFF-I386-NEXT: ]

llvm/trunk/test/tools/llvm-readobj/relocations.test

 RUN:   | FileCheck %s -check-prefix MACHO-PPC64
 RUN: llvm-readobj -r -expand-relocs %p/Inputs/trivial.obj.macho-arm \
 RUN:   | FileCheck %s -check-prefix MACHO-ARM
 RUN: llvm-readobj -r --expand-relocs %p/Inputs/trivial.obj.wasm \
 RUN:   | FileCheck %s -check-prefix WASM
 
 COFF:      Relocations [
 COFF-NEXT:   Section (1) .text {
-COFF-NEXT:     0x4 IMAGE_REL_I386_DIR32 .data
-COFF-NEXT:     0x9 IMAGE_REL_I386_REL32 _puts
-COFF-NEXT:     0xE IMAGE_REL_I386_REL32 _SomeOtherFunction
+COFF-NEXT:     0x4 IMAGE_REL_I386_DIR32 .data (4)
+COFF-NEXT:     0x9 IMAGE_REL_I386_REL32 _puts (7)
+COFF-NEXT:     0xE IMAGE_REL_I386_REL32 _SomeOtherFunction (8)
 COFF-NEXT:   }
 COFF-NEXT: ]
 
 BAD-COFF-RELOCS:      Relocations [
 BAD-COFF-RELOCS-NEXT:   Section (1) sec {
 BAD-COFF-RELOCS-NEXT:     0xDEADBEEF IMAGE_REL_I386_ABSOLUTE -
 BAD-COFF-RELOCS-NEXT:   }
 BAD-COFF-RELOCS-NEXT: ]

llvm/trunk/tools/llvm-readobj/COFFDumper.cpp

 void COFFDumper::printRelocation(const SectionRef &Section,
                                  const RelocationRef &Reloc, uint64_t Bias) {
   uint64_t Offset = Reloc.getOffset() - Bias;
   uint64_t RelocType = Reloc.getType();
   SmallString<32> RelocName;
   StringRef SymbolName;
   Reloc.getTypeName(RelocName);
   symbol_iterator Symbol = Reloc.getSymbol();
+  int64_t SymbolIndex = -1;
   if (Symbol != Obj->symbol_end()) {
     Expected<StringRef> SymbolNameOrErr = Symbol->getName();
     error(errorToErrorCode(SymbolNameOrErr.takeError()));
     SymbolName = *SymbolNameOrErr;
+    SymbolIndex = Obj->getSymbolIndex(Obj->getCOFFSymbol(*Symbol));
   }
 
   if (opts::ExpandRelocs) {
     DictScope Group(W, "Relocation");
     W.printHex("Offset", Offset);
     W.printNumber("Type", RelocName, RelocType);
     W.printString("Symbol", SymbolName.empty() ? "-" : SymbolName);
+    W.printNumber("SymbolIndex", SymbolIndex);
   } else {
     raw_ostream& OS = W.startLine();
     OS << W.hex(Offset)
        << " " << RelocName
        << " " << (SymbolName.empty() ? "-" : SymbolName)
+       << " (" << SymbolIndex << ")"
        << "\n";
   }
 }
 
 void COFFDumper::printSymbols() {
   ListScope Group(W, "Symbols");
 
   for (const SymbolRef &Symbol : Obj->symbols())