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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 6 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:284
+
+    // The symbol of section name is considerated as SymbolRef::ST_Other.
+    StringRef SecName;
----------------
jhenderson wrote:
> 
thanks


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:803
+    if (!CsectAuxRefOrError)
+      consumeError(CsectAuxRefOrError.takeError());
+    else {
----------------
jhenderson wrote:
> `consumeError` usage needs explanation/TODO etc, as before.
thanks


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test:21
 # LINES32:       00000000 <.text>:
-# LINES32:       ; main():
+# LINES32:       ; .main():
 # LINES32-NEXT:  ; /basic.c:1
----------------
jhenderson wrote:
> Mildly surprising there's a test change here. Why is there this change?
for we implement the getSymbolFlag() and getSymbolType() in the patch. after implement the getSymbolType() , the symbol ".main" is  marked as  function (it is correct to mark .main as function, the symbol "main" is function description in aix).  so the here change to .main(),   @Esme , can you help to confirm it ?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2042
+    if (O->isXCOFF()) {
+      XCOFFSymbolRef XCoffSymb =
+          dyn_cast<const XCOFFObjectFile>(O)->toSymbolRef(
----------------
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


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2063-2064
+
+        StringRef CName = unwrapOrError(SymRef.getValue().getName(), FileName,
+                                        ArchiveName, ArchitectureName);
+        std::string SymName(CName);
----------------
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;
  }

"




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109452