[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
Mon Sep 20 01:18:22 PDT 2021
jhenderson added a comment.
Some mostly stylistic comments. Beyond that, I've got not much else to add. @hubert.reinterpretcast will need to review the actual XCOFF implementation and corresponding code coverage.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:280
+ if (SymNameOrError) {
+ // The "TOC" symbol is considerated as SymbolRef::ST_Other.
+ if (SymNameOrError.get() == "TOC")
----------------
"considerated" isn't a word. I assume you mean "considered", but I think "treated" would be better. Same applies before.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:284
+
+ // The symbol of section name is considerated as SymbolRef::ST_Other.
+ StringRef SecName;
----------------
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:803
+ if (!CsectAuxRefOrError)
+ consumeError(CsectAuxRefOrError.takeError());
+ else {
----------------
`consumeError` usage needs explanation/TODO etc, as before.
================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test:21
# LINES32: 00000000 <.text>:
-# LINES32: ; main():
+# LINES32: ; .main():
# LINES32-NEXT: ; /basic.c:1
----------------
Mildly surprising there's a test change here. Why is there this change?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2042
+ if (O->isXCOFF()) {
+ XCOFFSymbolRef XCoffSymb =
+ dyn_cast<const XCOFFObjectFile>(O)->toSymbolRef(
----------------
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.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2063-2064
+
+ StringRef CName = unwrapOrError(SymRef.getValue().getName(), FileName,
+ ArchiveName, ArchitectureName);
+ std::string SymName(CName);
----------------
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.
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