[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

41

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

1408

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

41

Thanks. Did not know Symbol: 0 works..

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

1408

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

1408

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–573 ↗

(On Diff #284780)

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

[grimar]

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

571–573 ↗

(On Diff #284780)

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–573 ↗

(On Diff #284780)

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–573 ↗

(On Diff #284780)

Sounds OK to me.

[jhenderson]

Looks good from my point of view.

[grimar]

LGTM too.

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 %t.warn
+# RUN: llvm-objdump -d %t.warn 2>&1 | FileCheck %s --check-prefix=INVALID -DFILE=%t.warn
+
+# INVALID: warning: '[[FILE]]': PLT entry at 0x210030 references an invalid symbol
+# INVALID: 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

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

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);
       uint8_t SymbolType = getElfSymbolType(Obj, Symbol);
-
-      StringRef Name = unwrapOrError(Symbol.getName(), Obj->getFileName());
-      if (!Name.empty())
-        AllSymbols[*Plt].emplace_back(
-            PltEntry.second, Saver.save((Name + "@plt").str()), SymbolType);
+      if (Expected<StringRef> NameOrErr = Symbol.getName()) {
+        if (!NameOrErr->empty())
+          AllSymbols[*Plt].emplace_back(PltEntry.second,
+                                        Saver.save((*NameOrErr + "@plt").str()),
+                                        SymbolType);
+      } else {
+        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) {