[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/BinaryFormat/XCOFF.h:23
+enum {
+  NameSize = 8,
+  FileNamePadSize = 6
----------------
hubert.reinterpretcast wrote:
> Please commit the removal of `SectionNameSize` and `SymbolNameSize` separately from this patch. This conflicts with other Phabricator reviews in flight.
We should commit this as a NFC patch and then rebase this patch to reflect the change.


================
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) -
----------------
Why a pointer to a unitptr_t, shouldn't the argument just be a unitptr_t?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:473
+    report_fatal_error("Symbol table entry is outside of symbol table.");
+
+  if (Offset % SYM_TAB_ENTRY_SIZE != 0)
----------------
Should also check that the address isn't larger then the end of the symbol table.


================
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
----------------
sfertile wrote:
> 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.
Sorry I wasn't explicit before: the Run steps should be at the top of the file. Ideally the source and compile command would be at the end of the file.


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

https://reviews.llvm.org/D65240