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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

This patch seems to have gained a lot of functionality that sounds like it belongs in separate patches? The patch description and summary talk about reporting the symbol index, the qualname, and the symbol section. Other fields should be done in another patch, I'd think. There's no need for a single patch to implement all necessary functionality needed for complete symbol table printing.



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:230
+    if (!CsectAuxRefOrError)
+      consumeError(CsectAuxRefOrError.takeError());
+    else
----------------
`consumeError` is often a code smell. You should either 1) use `cantFail` instead, 2) add a TODO comment saying to report the message up the stack, 3) report the error somehow, or 4) add a comment explaining why the `consumeError` is justified here (e.g. "we don't care about invalid files for this purpose"). I suspect at this time 2) is the appropriate response.

Same comment applies for the other `consumeError` instances.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test:1
+; Test for the option --syms for the llvm-objdump for xcoff object file.
+; Test for the option --syms --symbol-description for xcoff object file.
----------------
We usually describe what the test //does// (and why where necessary - not applicable here) not what the test //is//: all the files in this directory are for testing llvm-objdump, so the comment can be simplified somewhat.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test:7
+
+;; Following IR are generated by following source code.
+;; bash> cat test.c
----------------



================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test:1
+; RUN: llc -mtriple powerpc-ibm-aix -verify-machineinstrs -mcpu=pwr4  \
+; RUN: -filetype=obj -o %t.o < %s
----------------
DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > Please add a brief comment explaining what this test is intended to test.
> > > > 
> > > > Also, this test won't succeed if the appropriate target is not configured for the person running the test. You need a REQUIRES statement.
> > > > 
> > > > I don't know much about llc, but I suspect it's likely that, based purely on the name, the `-verify-machineinstrs` option isn't needed?
> > > llc is cross compile, I think llc can compiler success in other platform , we do not need REQUIRES.
> > `llc` may be able to cross compile, but it requires to be built with the specified target enabled in LLVM_TARGETS_TO_BUILD, otherwise it doesn't know how to generate the machine code for the appropriate target.
> > 
> > Try regenerating your LLVM build tree with LLVM_TARGETS_TO_BUILD=X86 (and nothing else), and I bet you this test will fail, due to `llc` not knowing about the powerpc target in this case. You need the REQUIRES.
> > 
> > Also, don't forget my other comments in this test.
> there is a file 
> 
> llvm/test/tools/llvm-objdump/XCOFF/lit.local.cfg 
> 
> which has following content 
> 
> if not 'PowerPC' in config.root.targets:
>     config.unsupported = True
> 
> it protect the  test case in the llvm/test/tools/llvm-objdump/XCOFF not run for no PowerPC.
Thanks. Didn't realise that.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:72
+    return None;
+  uint32_t Idx = (uint32_t)CsectAuxEntOrErr.get().getSectionOrLength();
+  DataRefImpl DRI;
----------------
DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > 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.
> > If I hand-edited (or crafted with yaml2obj) an object file, could I end up with an invalid index that is larger than uint32_t, or is the field size only uint32_t?
> > 
> > Regardless, just because it's safe doesn't mean you need to do it. As far as I can tell, you could change `getSymbolByIndex` to take a `uint64_t` without impact, and you wouldn't need the cast.
> the field size only uint32_t
> 
> there is 
>  support::ubig32_t NumberOfSymTableEntries;  
> in both "struct XCOFFFileHeader64"  and "struct XCOFFFileHeader32"
> 
> if the  hand-edited (or crafted with yaml2obj) an object file , the high order of 32bit will be ignored. 
> 
> 
> 
Thanks. Please clang-format.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:25
+getXCOFFSymbolContainingSymbolRef(const object::XCOFFObjectFile *Obj,
+                                  const object::SymbolRef &Sym);
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > This is a problem throughout this header file (and its cpp) already, so please make a small commit to fix it: `SymbolRef` is designed to be a lightweight struct and passed by value, not by reference (just like `StringRef`).
> I think we should make a separate patch do deal the problem after the patch upload. 
I'm happy as long as it gets down.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109452