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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added a comment.

Looks like the patch is getting close.



================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:164
+enum CFileCpuId : uint8_t {
+  TCPU_PPC64 = 2, ///< PowerPC common architecture 64 bit mode.
+  TCPU_COM = 3,   ///< POWER and PowerPC architecture common.
----------------
Use "64-bit".


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:175
+  // This function returns name from the string table.
+  Expected<StringRef> getNameFromStringTable(uint32_t Offset) const;
 
----------------
This function is sufficiently generic that we can just say that it returns string table entries (and name it appropriately).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:89
+#ifndef NDEBUG
+  checkSymbolEntryPointer(reinterpret_cast<uintptr_t>(SymEntPtr));
+#endif
----------------
`Ref.p` is a `uintptr_t`, so this can be moved to before the `viewAs`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:121
+  // This function is used by basic_symbol_iterator, which allows to
+  // point to the end of symbol table address.
+  if (reinterpret_cast<uintptr_t>(SymEntPtr) != getEndOfSymbolTableAddress())
----------------
Use "end-of-symbol-table".


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:131
   // The byte offset is relative to the start of the string table
   // or .debug section. A byte offset value of 0 is a null or zero-length symbol
   // name. A byte offset in the range 1 to 3 (inclusive) points into the length
----------------
Remove "or .debug section" as this function does not cover this case (at least not yet).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:141
 
   return make_error<GenericBinaryError>("Symbol Name parse failed",
                                         object_error::parse_failed);
----------------
This is more generically "Bad offset for string table entry".


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:162
+  if (SymEntPtr->NameInStrTbl.Magic != XCOFFSymbolEntry::NAME_IN_STR_TBL_MAGIC)
+    return generateStringRef(SymEntPtr->SymbolName);
+
----------------
`generateStringRef` got modified in an NFC patch I believe. The uses in this patch make it more obvious that the one-argument version of that function should not be named so generically. Perhaps `generateXCOFFFixedNameStringRef`?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:186
+  W.printNumber("SectionLength", AuxEntPtr->SectionLength);
+  W.printNumber("NumberOfRelocEnt", AuxEntPtr->NumberOfRelocEnt);
+  W.printNumber("NumberOfLineNum", AuxEntPtr->NumberOfLineNum);
----------------
jasonliu wrote:
> hubert.reinterpretcast wrote:
> > Overflow handling is not here, and there is no TODO.
> Discussed offline, the overflow handling is for section auxiliary header, not for symbol auxiliary entry. 
I've experimented with XLC, and confirmed that it does not do the overflow handling for these fields. It will populate these with the low bits of the logical value.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:163
+  if ((AuxEntPtr->SymbolAlignmentAndType & SymbolTypeMask) == XCOFF::XTY_LD)
+    W.printNumber("ContainingCSECTSymbolIndex", AuxEntPtr->SectionLen);
+  else
----------------
I think we should treat this as code.


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

https://reviews.llvm.org/D65240