[llvm-objdump] Change symbol name/PLT decoding errors to warnings

[MaskRay]

Summary

If the referenced symbol of a J[U]MP_SLOT is invalid (e.g. symbol index 0), llvm-objdump -d will bail out:

error: 'a': st_name (0x326600) is past the end of the string table of size 0x7

where 0x326600 is the st_name field of the first entry past the end of .symtab

Change it to a warning to continue dumping. Noticed when trying an LLD patch.
X86/plt.test uses a prebuilt executable, so I pick ELF/AArch64/plt.test
which has a YAML input and can be easily modified.

[grimar]

llvm/test/tools/llvm-objdump/ELF/AArch64/plt.test

21

Probably it might be a bit better to be explicit here:

yaml2obj %s -D SYM=0 -o %t.warn

48

I'd do:

[[SYM=f1]]

I.e. then we have a valid YAML input by default and break it with -D SYM=0 for a particular test.

Otherwise, with [[SYM=<none>]] the input is initially broken, what prevents the possible futher extensions
(e.g. testing the second symbol) for this test without fixing values back to normal ones in each following
yaml2obj invocation.

llvm/tools/llvm-objdump/llvm-objdump.cpp

1406

Will it be better not to consume the error and to report something like:
"warning: unable to read the name of a symbol for PLT entry at ....: <error message with the reason>"?

[MaskRay]

Improve test

llvm/test/tools/llvm-objdump/ELF/AArch64/plt.test

48

Thanks. Did not know Symbol: 0 works..

llvm/tools/llvm-objdump/llvm-objdump.cpp

1406

The message is completely bogus (snowball effect after a decoding error): `st_name (0x326600) is past the end of the string tab
le of size 0x7`

[jhenderson]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1406

Sorry, I'm not sure I follow. Are you saying the PLT entries are bogus? In what context? Presumably that's not always the case when you get here, right?

[MaskRay]

Change the signature of getPltAddresses to return Optional<DataRefImpl>

Add another test

[MaskRay]

Fix a Mach-O test

[jhenderson]

llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp

571–574

This is either unrelated and/or untested, I think?

[grimar]

llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp

571–574

Previously

std::vector<std::pair<DataRefImpl, uint64_t>>
ELFObjectFileBase::getPltAddresses() const

had the following logic:

if (PltEntryIter != GotToPlt.end())
  Result.push_back(std::make_pair(
    Relocation.getSymbol()->getRawDataRefImpl(), PltEntryIter->second));

i.e Relocation.getSymbol() could return symbol_end() and
Addr.first could be broken here. Now Addr.first is Optional<> and can be None.
So I think this code is related, but indeed untested.

Perhaps we should just exit (like above code does):

if (!KV.second) {
  errs() << "Failed to add instruction at address "
         << format_hex(Instruction.VMAddress, 2)
         << ": Instruction at this address already exists.\n";
  exit(EXIT_FAILURE);
}

or return a error instead.

[MaskRay]

llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp

571–574

The existing code does consumeError(SymNameOrErr.takeError()); - an intention that it does not want to recover errors, so I think it is fine leaving it ignored. The maintainers may want to fix it, though, but it is out of the scope of this patch.

[grimar]

llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp

571–574

Sounds OK to me.

[jhenderson]

Looks good from my point of view.

[grimar]

LGTM too.

llvm/include/llvm/Object/ELFObjectFile.h

public:
   SubtargetFeatures getRISCVFeatures() const;
 
   void setARMSubArch(Triple &TheTriple) const override;
 
   virtual uint16_t getEType() const = 0;
 
   virtual uint16_t getEMachine() const = 0;
 
-  std::vector<std::pair<DataRefImpl, uint64_t>> getPltAddresses() const;
+  std::vector<std::pair<Optional<DataRefImpl>, uint64_t>>
+  getPltAddresses() const;
 };
 
 class ELFSectionRef : public SectionRef {
 public:
   ELFSectionRef(const SectionRef &B) : SectionRef(B) {
     assert(isa<ELFObjectFileBase>(SectionRef::getObject()));
   }
 

llvm/lib/Object/ELFObjectFile.cpp

if (Attr.hasValue()) {
     }
   }
   if (!isLittleEndian())
     Triple += "eb";
 
   TheTriple.setArchName(Triple);
 }
 
-std::vector<std::pair<DataRefImpl, uint64_t>>
+std::vector<std::pair<Optional<DataRefImpl>, uint64_t>>
 ELFObjectFileBase::getPltAddresses() const {
   std::string Err;
   const auto Triple = makeTriple();
   const auto *T = TargetRegistry::lookupTarget(Triple.str(), Err);
   if (!T)
     return {};
   uint64_t JumpSlotReloc = 0;
   switch (Triple.getArch()) {
auto PltEntries = MIA->findPltEntries(Plt->getAddress(),
                                         arrayRefFromStringRef(*PltContents),
                                         GotPlt->getAddress(), Triple);
   // Build a map from GOT entry virtual address to PLT entry virtual address.
   DenseMap<uint64_t, uint64_t> GotToPlt;
   for (const auto &Entry : PltEntries)
     GotToPlt.insert(std::make_pair(Entry.second, Entry.first));
   // Find the relocations in the dynamic relocation table that point to
   // locations in the GOT for which we know the corresponding PLT entry.
-  std::vector<std::pair<DataRefImpl, uint64_t>> Result;
+  std::vector<std::pair<Optional<DataRefImpl>, uint64_t>> Result;
   for (const auto &Relocation : RelaPlt->relocations()) {
     if (Relocation.getType() != JumpSlotReloc)
       continue;
     auto PltEntryIter = GotToPlt.find(Relocation.getOffset());
-    if (PltEntryIter != GotToPlt.end())
-      Result.push_back(std::make_pair(
-          Relocation.getSymbol()->getRawDataRefImpl(), PltEntryIter->second));
+    if (PltEntryIter != GotToPlt.end()) {
+      symbol_iterator Sym = Relocation.getSymbol();
+      if (Sym == symbol_end())
+        Result.emplace_back(None, PltEntryIter->second);
+      else
+        Result.emplace_back(Sym->getRawDataRefImpl(), PltEntryIter->second);
+    }
   }
   return Result;
 }

llvm/test/tools/llvm-objdump/ELF/AArch64/plt.test

 # CHECK-BTI: Disassembly of section .plt:
 # CHECK-BTI: <f1@plt>:
 # CHECK-BTI-NEXT: bti   c
 # CHECK-BTI-NEXT: adrp  x16, {{.*}}
 # CHECK-BTI: <f2@plt>:
 # CHECK-BTI-NEXT: bti   c
 # CHECK-BTI-NEXT: adrp  x16, {{.*}}
 
+# RUN: yaml2obj -D SYM=0 %s -o %tindex
+# RUN: llvm-objdump -d %tindex 2>&1 | FileCheck %s --check-prefix=INVALID_INDEX -DFILE=%tindex
+
+# INVALID_INDEX: warning: '[[FILE]]': PLT entry at 0x210030 references an invalid symbol{{$}}
+# INVALID_INDEX: Disassembly of section .text:
+
+# RUN: yaml2obj -D ST_NAME=0x1234 %s -o %tst_name
+# RUN: llvm-objdump -d %tst_name 2>&1 | FileCheck %s --check-prefix=INVALID_ST_NAME -DFILE=%tst_name
+
+# INVALID_ST_NAME:      warning: '[[FILE]]': st_name (0x1234) is past the end of the string table of size 0x7
+# INVALID_ST_NAME-NEXT: warning: '[[FILE]]': PLT entry at 0x210030 references an invalid symbol{{$}}
+# INVALID_ST_NAME:      Disassembly of section .text:
+
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_AARCH64
 Sections:
   - Name:    .rela.plt
     Type:    SHT_RELA
     Flags:   [ SHF_ALLOC ]
     EntSize: 0x0000000000000018
     Info:    .got.plt
     Relocations:
       - Offset: 0x0000000000230018
-        Symbol: f1
+        Symbol: [[SYM=f1]]
         Type:   R_AARCH64_JUMP_SLOT
       - Offset: 0x0000000000230020
         Symbol: f2
         Type:   R_AARCH64_JUMP_SLOT
   - Name:    .text
     Type:    SHT_PROGBITS
     Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
     Address: 0x0000000000210000
     Content: 0C00009411000094C0035FD6
   - Name:    .plt
     Type:    SHT_PROGBITS
     Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
     Address: 0x0000000000210010
     Content: 5F2403D5F07BBFA910010090110A40F91042009120021FD61F2003D51F2003D55F2403D510010090110E40F9106200919F2103D520021FD65F2403D510010090111240F9108200919F2103D520021FD6
   - Name:    .got.plt
     Type:    SHT_PROGBITS
     Content: '000000000000000000000000000000000000000000000000100021000000000010002100000000001000210000000000'
 Symbols:
   - Name:    f1
+    StName:  [[ST_NAME=<none>]]
     Type:    STT_FUNC
     Binding: STB_GLOBAL
   - Name:    f2
     Type:    STT_FUNC
     Binding: STB_GLOBAL
 ...

llvm/test/tools/llvm-objdump/MachO/malformed-machos.test

 INVALID-SYMBOL-INDR: macho-invalid-symbol-indr': truncated or malformed object (bad n_value: 22 past the end of string table, for N_INDR symbol at index 1)
 
 RUN: not llvm-objdump --macho -d --arch all %p/Inputs/macho-invalid-symbol-indr-archive-universal 2>&1 | FileCheck --check-prefix INVALID-SYMBOL-INDR-ARCHIVE-UNIVERSAL %s
 INVALID-SYMBOL-INDR-ARCHIVE-UNIVERSAL: macho-invalid-symbol-indr-archive-universal(macho-invalid-symbol-indr) (for architecture i386): truncated or malformed object (bad n_value: 22 past the end of string table, for N_INDR symbol at index 1)
 
 RUN: not llvm-objdump --macho -d %p/Inputs/macho-invalid-symbol-strx 2>&1 | FileCheck --check-prefix INVALID-SYMBOL-STRX %s
 INVALID-SYMBOL-STRX: macho-invalid-symbol-strx': truncated or malformed object (bad string table index: 22 past the end of string table, for symbol at index 1)
 
-RUN: not llvm-objdump -d %p/Inputs/macho-invalid-symbol-strx 2>&1 | FileCheck --check-prefix INVALID-SYMBOL-STRX-NO-MACHO-FLAG %s
+RUN: llvm-objdump -d %p/Inputs/macho-invalid-symbol-strx 2>&1 | FileCheck --check-prefix INVALID-SYMBOL-STRX-NO-MACHO-FLAG %s
 INVALID-SYMBOL-STRX-NO-MACHO-FLAG: macho-invalid-symbol-strx': truncated or malformed object (bad string index: 22 for symbol at index 1)
 
 RUN: not llvm-objdump --macho -d --arch all %p/Inputs/macho-invalid-symbol-strx-universal 2>&1 | FileCheck --check-prefix INVALID-SYMBOL-STRX-UNIVERSAL %s
 INVALID-SYMBOL-STRX-UNIVERSAL: macho-invalid-symbol-strx-universal' (for architecture i386): truncated or malformed object (bad string table index: 22 past the end of string table, for symbol at index 1)
 
 RUN: not llvm-objdump --macho -d %p/Inputs/macho-invalid-symbol-lib_ordinal 2>&1 | FileCheck --check-prefix INVALID-SYMBOL-LIB_ORDINAL %s
 INVALID-SYMBOL-LIB_ORDINAL: macho-invalid-symbol-lib_ordinal': truncated or malformed object (bad library ordinal: 7 for symbol at index 2)
 
 RUN: not llvm-objdump --macho --objc-meta-data %p/Inputs/macho-invalid-bind-entry 2>&1 | FileCheck --check-prefix INVALID-BIND-ENTRY %s
 INVALID-BIND-ENTRY: macho-invalid-bind-entry': truncated or malformed object (for BIND_OPCODE_SET_DYLIB_ORDINAL_ULEB bad library ordinal: 83 (max 0) for opcode at: 0x0)
 
 RUN: llvm-objdump --macho -r --arch x86_64 %p/Inputs/macho-invalid-reloc-section-index 2>&1 | FileCheck --check-prefix INVALID-RELOC-SECTION-INDEX %s
 INVALID-RELOC-SECTION-INDEX: 00000021 False byte   False  UNSIGND False     8388613 (?,?)
 
 RUN: not llvm-objdump --bind -g --macho -r --rebase -s --section-headers -t --unwind-info %p/Inputs/macho-invalid-linker-command 2>&1 | FileCheck --check-prefix INVALID-LINKCMD %s
 INVALID-LINKCMD: truncated or malformed object (load command 4 LC_LINKER_OPTION string #2 is not NULL terminated)

llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp

else if (TrapOnFailFunctions.count(*SymNameOrErr) > 0) {
       if (!AddrOrErr)
         consumeError(AddrOrErr.takeError());
       else
         TrapOnFailFunctionAddresses.insert(*AddrOrErr);
     }
   }
   if (auto *ElfObject = dyn_cast<object::ELFObjectFileBase>(Object)) {
     for (const auto &Addr : ElfObject->getPltAddresses()) {
-      object::SymbolRef Sym(Addr.first, Object);
+      if (!Addr.first)
+        continue;
+      object::SymbolRef Sym(*Addr.first, Object);
       auto SymNameOrErr = Sym.getName();
       if (!SymNameOrErr)
         consumeError(SymNameOrErr.takeError());
       else if (TrapOnFailFunctions.count(*SymNameOrErr) > 0)
         TrapOnFailFunctionAddresses.insert(Addr.second);
     }
   }
   return Error::success();
 }

llvm/tools/llvm-objdump/llvm-objdump.cpp

for (const SectionRef &Section : Obj->sections()) {
     }
     if (*SecNameOrErr == ".plt")
       Plt = Section;
   }
   if (!Plt)
     return;
   if (auto *ElfObj = dyn_cast<ELFObjectFileBase>(Obj)) {
     for (auto PltEntry : ElfObj->getPltAddresses()) {
-      SymbolRef Symbol(PltEntry.first, ElfObj);
+      if (PltEntry.first) {
+        SymbolRef Symbol(*PltEntry.first, ElfObj);
         uint8_t SymbolType = getElfSymbolType(Obj, Symbol);
-
-      StringRef Name = unwrapOrError(Symbol.getName(), Obj->getFileName());
-      if (!Name.empty())
+        if (Expected<StringRef> NameOrErr = Symbol.getName()) {
+          if (!NameOrErr->empty())
             AllSymbols[*Plt].emplace_back(
-            PltEntry.second, Saver.save((Name + "@plt").str()), SymbolType);
+                PltEntry.second, Saver.save((*NameOrErr + "@plt").str()),
+                SymbolType);
+          continue;
+        } else {
+          // The warning has been reported in disassembleObject().
+          consumeError(NameOrErr.takeError());
+        }
+      }
+      reportWarning("PLT entry at 0x" + Twine::utohexstr(PltEntry.second) +
+                        " references an invalid symbol",
+                    Obj->getFileName());
     }
   }
 }
 
 // Normally the disassembly output will skip blocks of zeroes. This function
 // returns the number of zero bytes that can be skipped when dumping the
 // disassembly of the instructions in Buf.
 static size_t countSkippableZeroBytes(ArrayRef<uint8_t> Buf) {
static void disassembleObject(const Target *TheTarget, const ObjectFile *Obj,
 
   // Create a mapping from virtual address to symbol name.  This is used to
   // pretty print the symbols while disassembling.
   std::map<SectionRef, SectionSymbolsTy> AllSymbols;
   SectionSymbolsTy AbsoluteSymbols;
   const StringRef FileName = Obj->getFileName();
   const MachOObjectFile *MachO = dyn_cast<const MachOObjectFile>(Obj);
   for (const SymbolRef &Symbol : Obj->symbols()) {
-    StringRef Name = unwrapOrError(Symbol.getName(), FileName);
-    if (Name.empty() && !(Obj->isXCOFF() && SymbolDescription))
+    Expected<StringRef> NameOrErr = Symbol.getName();
+    if (!NameOrErr) {
+      reportWarning(toString(NameOrErr.takeError()), FileName);
+      continue;
+    }
+    if (NameOrErr->empty() && !(Obj->isXCOFF() && SymbolDescription))
       continue;
 
     if (Obj->isELF() && getElfSymbolType(Obj, Symbol) == ELF::STT_SECTION)
       continue;
 
     // Don't ask a Mach-O STAB symbol for its section unless you know that
     // STAB symbol's section field refers to a valid section index. Otherwise
     // the symbol may error trying to load a section that does not exist.