[PATCH] D109452: implement the --syms and using "symbol index and qualname" for --sym --symbol--description for llvm-objdump for xcoff

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2042
+    if (O->isXCOFF()) {
+      XCOFFSymbolRef XCoffSymb =
+          dyn_cast<const XCOFFObjectFile>(O)->toSymbolRef(
----------------
DiggerLin wrote:
> jhenderson wrote:
> > 1) Be consistent in your capitalization.
> > 2) If an abbreviation is needed, "Sym" is the usual abbreviation, not "Symb".
> > 
> > Consequently, I'd rename this variable "XCOFFSym" or, if possible "XCOFFSymbol", depending on whether the latter clashes with a type somewhere or not.
> thanks
Please make the same change throughout your changed code (there are several other places with the old variable name).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2063-2064
+
+        StringRef CName = unwrapOrError(SymRef.getValue().getName(), FileName,
+                                        ArchiveName, ArchitectureName);
+        std::string SymName(CName);
----------------
DiggerLin wrote:
> jhenderson wrote:
> > This will result in a hard error, and stopping of any further dumping of the file, if there's a problem getting the name. It seems to me like this should actually be reported as a warning, and just keep going, using some placeholder string instead (e.g. "<?>") for the invalid name.
> I think we need to deal with  the error as same  as section name in the code 
> 
> 
> "
>     if (!SegmentName.empty())
>       outs() << SegmentName << ",";
>     StringRef SectionName = unwrapOrError(Section->getName(), FileName);
>     outs() << SectionName;
>   }
> 
> "
> 
> 
I'd suggest the SectionName is in a similar boat - a failure there shouldn't prevent dumping the rest, and should instead probably be printed as a warning. You could do something similar to what I've suggested for the symbol. You can change the section name error handling in a different patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109452