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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast requested changes to this revision.
hubert.reinterpretcast added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:136
+  XCOFF::CFileStringType Type;
+  uint8_t ReservedZero[2];
+  uint8_t AuxType;
----------------
`ReservedZeroes`?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:137
+  uint8_t ReservedZero[2];
+  uint8_t AuxType;
+};
----------------
Add at least a comment to indicate that this field is XCOFF64-specific.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:269
   uint32_t getNumberOfSymbolTableEntries64() const;
+  uint32_t getSymbolIndex(const uintptr_t SymEntPtr) const;
 
----------------
Stray top-level `const` on a parameter declaration not attached to a function definition.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:466
 
+uint32_t XCOFFObjectFile::getSymbolIndex(const uintptr_t SymbolEntPtr) const {
+  uint32_t Offset = reinterpret_cast<const char *>(SymbolEntPtr) -
----------------
Top-level `const` on parameter types are known to cause grief with certain build compilers.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:467
+uint32_t XCOFFObjectFile::getSymbolIndex(const uintptr_t SymbolEntPtr) const {
+  uint32_t Offset = reinterpret_cast<const char *>(SymbolEntPtr) -
+                    reinterpret_cast<const char *>(SymbolTblPtr);
----------------
Type error. The answer fits into 32-bits after being divided by 18, but not necessarily before that.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:467
+uint32_t XCOFFObjectFile::getSymbolIndex(const uintptr_t SymbolEntPtr) const {
+  uint32_t Offset = reinterpret_cast<const char *>(SymbolEntPtr) -
+                    reinterpret_cast<const char *>(SymbolTblPtr);
----------------
hubert.reinterpretcast wrote:
> Type error. The answer fits into 32-bits after being divided by 18, but not necessarily before that.
I suggest moving this to after the error checking.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:477
+                : getLogicalNumberOfSymbolTableEntries32();
+  if (SymbolEntPtr >
+      reinterpret_cast<uintptr_t>(reinterpret_cast<const char *>(SymbolTblPtr) +
----------------
`>=`. The calculation on the RHS points "one-past-the-end".


================
Comment at: llvm/test/tools/llvm-readobj/xcoff-symbols.test:21
+SYMBOL32-NEXT:       Type: XFT_FN (0x0)
+SYMBOL32-NEXT:       AuxType: 0x0
+SYMBOL32-NEXT:     }
----------------
There is no such field defined for XCOFF32.


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

https://reviews.llvm.org/D65240