[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 9 inline comments as done.
DiggerLin 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
----------------
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.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:72
+    return None;
+  uint32_t Idx = (uint32_t)CsectAuxEntOrErr.get().getSectionOrLength();
+  DataRefImpl DRI;
----------------
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. 





================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:25
+getXCOFFSymbolContainingSymbolRef(const object::XCOFFObjectFile *Obj,
+                                  const object::SymbolRef &Sym);
+
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109452