[PATCH] D83129: [llvm-readobj] - Allow dumping partially corrupted SHT_LLVM_CALL_GRAPH_PROFILE sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, but see my inline comment (happy for you to commit the two patches separately or as one).



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6559-6566
+    if (Expected<std::string> NameOrErr =
+            this->dumper()->getStaticSymbolName(Index))
+      return *NameOrErr;
+    else
+      this->reportUniqueWarning(
+          createError("unable to read the name of symbol with index " +
+                      Twine(Index) + ": " + toString(NameOrErr.takeError())));
----------------
grimar wrote:
> grimar wrote:
> > jhenderson wrote:
> > > This seems like a pattern we're likely to have in several different parts of the ELFDumper. Is there any code we could share to avoid duplication? Maybe it just makes sense to change `getStaticSymbolName` to report the warning/return the `<?>` itself?
> > From what I see, the `getStaticSymbolName` is used in one more place:
> > 
> > ```
> > template <class ELFT>
> > void LLVMStyle<ELFT>::printAddrsig(const ELFFile<ELFT> *Obj) {
> > ...
> >   for (uint64_t Sym : *V) {
> >     Expected<std::string> NameOrErr = this->dumper()->getStaticSymbolName(Sym);
> >     if (NameOrErr) {
> >       W.printNumber("Sym", *NameOrErr, Sym);
> >       continue;
> >     }
> >     reportWarning(NameOrErr.takeError(), this->FileName);
> >     W.printNumber("Sym", "<?>", Sym);
> >   }
> > }
> > ```
> > 
> > And it looks like it should be reasonable and possible to do what you suggest. Follow-up?
> Follow-up: D83208
Normally, I'd say yes, follow-up, but in this case, D83208 looks small enough that it could be done as part of this, especially as most of that change is just undoing what some of this change does (in particular, the `GetSymName` lamdba replaces `getStaticSymbolName` in this change and then is just replaced back the other way again). I don't feel strongly about this though.


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

https://reviews.llvm.org/D83129