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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:284
+class XCOFFSymbolRef {
+  DataRefImpl SymEntDataRef;
+  const XCOFFObjectFile *const OwningObjectPtr;
----------------
I meant this line (re: `const`) as well.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:294
+  uint8_t getNumberOfAuxEntries() const;
+  const XCOFFCSectAuxEnt32 *getXCOFFCSectAuxEnt32() const;
+  uint16_t getType() const;
----------------
Might as well use `Csect` here while we are at this for this patch (see D65159).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:129
+  uint32_t Offset = CFileEntPtr->NameInStrTbl.Offset;
+  if (Offset < 4)
+    return StringRef(nullptr, 0);
----------------
Please common up some of the code between this and `getSymbolName`. We might be able to go with one template function implementation for both of these interfaces to use.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:467
+uint32_t XCOFFObjectFile::getSymbolIndex(uintptr_t SymbolEntPtr) const {
+#ifndef NDEBUG
+  if (SymbolEntPtr < reinterpret_cast<uintptr_t>(SymbolTblPtr))
----------------
`getSymbolIndex` is a strange place for the error checking to occur. We already got a `SymbolEntPtr`, and we might have used it already. We should move the error checking out to a separate function and call it where appropriate (perhaps at each place where we retrieve a handle to a symbol table entry, or perhaps only when jumping by an index).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:637
+
+const XCOFFCSectAuxEnt32 *XCOFFSymbolRef::getXCOFFCSectAuxEnt32() const {
+  assert(!OwningObjectPtr->is64Bit() &&
----------------
Global replace `CSect` with `Csect` in code (note: not the same for English).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:641
+  assert(hasCSectAuxEnt() && "No CSect Auxiliary Entry is found.");
+  uintptr_t AuxAddr = getWithOffset(
+      SymEntDataRef.p, SYM_TAB_ENTRY_SIZE * getNumberOfAuxEntries());
----------------
To explain the code, add a comment to indicate that the csect auxiliary entry is, for symbols that have them in XCOFF32, the last auxiliary entry.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:643
+      SymEntDataRef.p, SYM_TAB_ENTRY_SIZE * getNumberOfAuxEntries());
+  return reinterpret_cast<const XCOFFCSectAuxEnt32 *>(AuxAddr);
+}
----------------
Might make sense to call the error checking function here.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:655
+bool XCOFFSymbolRef::hasCSectAuxEnt() const {
+  XCOFF::StorageClass Sc = getStorageClass();
+  return (Sc == XCOFF::C_EXT || Sc == XCOFF::C_WEAKEXT ||
----------------
s/Sc/SC;


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:671
+  const XCOFFCSectAuxEnt32 *CSectAuxEnt = getXCOFFCSectAuxEnt32();
+  // The symbol is label definition.
+  if ((CSectAuxEnt->SymbolAlignmentAndType & SYM_TYPE_MASK) != XCOFF::XTY_LD)
----------------
Newline before the comment. Clarify the comment as saying that we are expecting function definitions to be label definitions.


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:1
+# This file tests the ability of llvm-readobj to generate symbol table for 
+# 32-bit XCOFF object file.
----------------
s/generate/display the/;
s/for/for a/;


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:16
+SYMBOL32-NEXT:     Section: N_DEBUG
+SYMBOL32-NEXT:     Source Language ID: 0
+SYMBOL32-NEXT:     CPU Version ID: 3
----------------
```
TB_C = 0,
TB_CPLUSPLUS = 9,
```


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:17
+SYMBOL32-NEXT:     Source Language ID: 0
+SYMBOL32-NEXT:     CPU Version ID: 3
+SYMBOL32-NEXT:     StorageClass: C_FILE (0x67)
----------------
```
TCPU_PPC64 = 2,
TCPU_COM = 3,
TCPU_970 = 19,
```


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:109
+SYMBOL32-NEXT:     NumberOfAuxEntries: 1
+SYMBOL32-NEXT:     CSect Auxiliary Entry {
+SYMBOL32-NEXT:       Index: 13
----------------
Global replace "CSect" with "CSECT" in English.


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:426
+#
+# > cat aix_xcoff_xlc_test8.c
+# 
----------------
The filename is `test8.c`.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:28
+    SymbolAlignmentMask = 0xF8,
+    SymbolTypeBits = 3
+  };
----------------
Given the way it is used, this should be something like `SymbolAlignmentBitOffset`.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:182
+    const XCOFFSectAuxEntForStat *AuxEntPtr) {
+  DictScope SymDs(W, "Sect Auxiliary Entry For Stat");
+  W.printNumber("Index",
----------------
Assert XCOFF32. This entry type does not exist for XCOFF64.


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

https://reviews.llvm.org/D65240