[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:129
+    support::ubig32_t Offset;
+    char NamdePad[XCOFF::FileNamePadSize];
+  } NameInStrTblType;
----------------
minor nit: Was this supposed to be `NamePad`?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:151
+  support::ubig32_t FilePointerToLineNum;
+  support::ubig32_t IndexofNextEntry;
+  char Pad[2];
----------------
minor nit: `IndexofNextEntry` --> `IndexOfNextEntry`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:462
+uint32_t
+XCOFFObjectFile::getSymbolIndex(const XCOFFSymbolEntry *SymbolEntPtr) const {
+  return SymbolEntPtr - SymbolTblPtr;
----------------
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.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:615
+      OwningObjectPtr->toSymbolEntry(SymEntDataRef);
+  assert(hasCSectAuxEnt());
+  return (const XCOFFCSectAuxEnt *)(SymEntPtr + getNumberOfAuxEntries());
----------------
minor nit: place the assert as the first statement in the function.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:616
+  assert(hasCSectAuxEnt());
+  return (const XCOFFCSectAuxEnt *)(SymEntPtr + getNumberOfAuxEntries());
+}
----------------
Use a reinterpret_cast instead of a c-style cast.

A though about the implementation of this function: 
We are converting the address in the DataRefImpl to an XCOFFSymbolEntry and doing the pointer arithmetic on that type and it works because all symbol table entries have the same size as this structiure. But why convert in the first place? Instead we can calculate the address using  `getWithOffset`. That code would be equally valid for 64-bit and 32-bit XCOFF objects.

```
assert(hasCSectAuxEnt());
uintptr_t AuxAddr = getWithOffset(SymEntDataRef.p, SYM_TAB_ENTRY_SIZE * getNumberOfAuxEntries());
return reiterpret_cast<const XCOFFCSectAuxEnt *>(AuxAddr);
```


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:649
+  int16_t SectNum = getSectionNumber();
+  Expected<DataRefImpl> SI = OwningObjectPtr->getSectionByNum(SectNum);
+  if (!SI)
----------------
If we fail to find a section, then should a StorageMappingClass of `XMC_PR` be enough to determine the symbol is a function?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:124
+
+template <typename T> const XCOFFSymbolEntry *ViewAsSymEnt(const T *t) {
+  return reinterpret_cast<const XCOFFSymbolEntry *>(t);
----------------
When we fix the interface to `getSymbolIndex` we can get rid of this function.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:131
+  DictScope SymDs(W, "File Auxiliary Entry");
+  W.printNumber("Index", Obj.getSymbolIndex(ViewAsSymEnt(AuxEntPtr)));
+  W.printString("Name", FileName);
----------------
This is the cast I find confusing. We have a pointer to a structure of type `XCOFFFileAuxEnt` and we have to cast it to something it is not to get the symbol table entries index.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65240