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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp:571-573
+      if (!Addr.first)
+        continue;
+      object::SymbolRef Sym(*Addr.first, Object);
----------------
grimar wrote:
> jhenderson wrote:
> > This is either unrelated and/or untested, I think?
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85623/new/

https://reviews.llvm.org/D85623