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

[Digger Lin via Phabricator via llvm-commits]

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


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:72
+    return None;
+  uint32_t Idx = (uint32_t)CsectAuxEntOrErr.get().getSectionOrLength();
+  DataRefImpl DRI;
----------------
jhenderson wrote:
> It seems that if a case is necessary here, there's a problem with the function you're calling. At a guess, it should just be two functions, one returning a section, the other an index, with errors or assertions reported if using the wrong one for the type.
> 
> If a case is really necessary, use `static_cast`.
there is a  description in the XCOFFObjectFile.h 
  // For getSectionOrLength(),
  // If the symbol type is XTY_SD or XTY_CM, the csect length.
  // If the symbol type is XTY_LD, the symbol table
  // index of the containing csect.
  // If the symbol type is XTY_ER, 0.
  uint64_t getSectionOrLength() const {
    return Entry32 ? getSectionOrLength32() : getSectionOrLength64();
  }

for the number of symbols in aix xcoff object file  should not large than the max of uint32_t. so cast to unit32_t is safe.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2048
     outs() << SectionName;
+    if (O->isXCOFF()) {
+      Optional<SymbolRef> SymRef = getXCOFFSymbolContainingSymbolRef(
----------------
hubert.reinterpretcast wrote:
> The containing csect presents as an annotation "within" the section field. It may be friendlier if the delimiters of the section field were more explicit:
> ```
> 00000000000000c0 l [.text (csect: (idx: 2) .foov[PR])] (idx: 3) .foov
> ```
> and without --symbol-description
> ```
> 00000000000000c0 l [.text (csect: .foov)] .foov
> ```
thanks


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2058
+        if (Demangle)
+          SymName = demangle(std::string(CName));
+
----------------
jhenderson wrote:
> 
thanks


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2070
 
   if (Common || O->isELF()) {
     uint64_t Val =
----------------
jhenderson wrote:
> hubert.reinterpretcast wrote:
> > @DiggerLin, at least some XCOFF symbols have size fields. This could be handled in a separate patch if that is your preference (in which case, please add a TODO comment to the code).
> > 
> > @jhenderson, at least for XCOFF, I would prefer to always have the field (even if empty). I am not convinced that having it present only for "common" symbols for non-ELF formats is good.
> At least for ELF format, we are tied by the need to match GNU objdump output as closely as practical. I've no issue it being different for other formats that don't have a GNU objdump implementation they are trying to mirror.
@hubert.reinterpretcast , the size we  mention here is section size?
in the https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format

XTY_SD  x_scnlen contains the csect length. 
XTY_CM x_scnlen contains the csect length.

so we want to display the size of the symbols of the XTY_SD symbols and XTY_CM symbols  or only XTY_CM symbols? 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2109
   if (Demangle)
-    outs() << ' ' << demangle(std::string(Name)) << '\n';
-  else
-    outs() << ' ' << Name << '\n';
+    SymName = demangle(std::string(Name));
+
----------------
jhenderson wrote:
> 
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109452