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

[George Rimar via Phabricator via llvm-commits]

grimar 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);
----------------
MaskRay wrote:
> 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.
Sounds OK to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85623