[PATCH] D65240: [XCOFF][AIX] Generate symbol table entries with llvm-readobj

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:152
+  support::ubig32_t IndexOfNextEntry;
+  char Pad[2];
+};
----------------
We should open a defect against the XCOFF object file docs, it list the pading as length 1 which is incorrect.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:462
+uint32_t
+XCOFFObjectFile::getSymbolIndex(const XCOFFSymbolEntry *SymbolEntPtr) const {
+  return SymbolEntPtr - SymbolTblPtr;
----------------
sfertile wrote:
> I think this is the wrong interface for this function.  Taking a pointer to an `XCOFFSymbolEntry`  ends up making us cast a bunch of symbol table entries to a type they clearly are not to get their Symbol index. Instead we should take a unitptr_t.
Looks good. One suggestion is to have some error checking similar to what we have in `checkSectionAddress` .


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:649
+  int16_t SectNum = getSectionNumber();
+  Expected<DataRefImpl> SI = OwningObjectPtr->getSectionByNum(SectNum);
+  if (!SI)
----------------
jasonliu wrote:
> sfertile wrote:
> > If we fail to find a section, then should a StorageMappingClass of `XMC_PR` be enough to determine the symbol is a function?
> In a single .o file, then maybe XMC_PR is enough to determine the symbol is a function. 
> But I'm not sure if that's the case for a executable/dynamic library where linker could regroup the functions in a different way.  
> I'm open to suggestions if there is a better way to do this.
Sorry, I don't have any helpful suggestions. I'm not familiar enough with XCOFF to know when/how it uses absolute symbols.  I think this current implementation is OK for now. It might lead to missing information in llvm-readobj but we can refine it as our understanding grows. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:125
+      XCOFFSymbolEntry::NAME_IN_STR_TBL_MAGIC)
+    return generateStringRef(CFileEntPtr->Name, XCOFF::NameSize);
+
----------------
We can remove the second argument to `generateStringRef` and use `XCOFF::NameSize` directly in the implementation, since all calls use the same value for the argument.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:647
+  const XCOFFCSectAuxEnt *CSectAuxEnt = getXCoffCSectAuxEnt();
+  // The symbol is label define.
+  if ((CSectAuxEnt->SymbolAlignmentAndType & SYM_TYPE_MASK) != XCOFF::XTY_LD)
----------------
`label define` --> `label definition`.


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:1
+# RUN: llvm-readobj --symbols %p/Inputs/aix_xcoff_xlc_test8.o | \
+# RUN: FileCheck --check-prefix=SYMBOL32 %s
----------------
Add the source used, compiler, and options used to genetrate the object file in a comment.

Also it seems that we have added several constructs that are not exercised in the test: Block Aux Entry, Dwarf Aux Entry, Function Aux Ent. If these aren't tested then remove them from this patch and we can add them in a subsequent patch.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:236
+  switch (SC) {
+  case XCOFF::C_EXT:
+  case XCOFF::C_WEAKEXT:
----------------
We are adding a bunch of cases which aren't tested. Would it be better to add these cases with an assert  and implement them in a future patch which also adds the missing test coverage? 


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

https://reviews.llvm.org/D65240