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

[George Rimar via Phabricator via llvm-commits]

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


================
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())));
----------------
jhenderson wrote:
> 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.
The problem is that  D83208 is not a NFC. It changes the message reported for `--addrsig` (adds a prefix with the context):

"warning: '[[FILE]]': unable to get symbol from section [index 2]: invalid symbol index (255)" -> "warning: '[[FILE]]': unable to read the name of symbol with index 255: unable to get symbol from section [index 2]: invalid symbol index (255)"

That is why I slightly prefer to keep them separate.


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

https://reviews.llvm.org/D83129