[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 inline comments.


================
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:
> > 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.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test:2
+; RUN: llc -mtriple powerpc-ibm-aix -verify-machineinstrs -mcpu=pwr4  \
+; RUN: -filetype=obj -o %t.o < %s
+; RUN: llvm-objdump --syms %t.o | FileCheck --check-prefix=SYM %s
----------------
jhenderson wrote:
> When continuing run lines onto the next one, add spacing after the RUN: on the subsequent lines to indent the arguments a bit. This helps readability by tying it to the previous line.
This is marked as done, but has not been fixed. Please don't mark items as done until you upload a patch that addresses them. Same goes throughout.


================
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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109452