[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
llvm-commits at lists.llvm.org
Tue Sep 21 00:31:54 PDT 2021
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
More information about the llvm-commits
mailing list