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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added a subscriber: DiggerLin.
jasonliu added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:22
+  FUNCTION_SYM = 0x20,
+  RELOC_OVERFLOW = 65535,
+  SYM_TAB_ENTRY_SIZE = 18,
----------------
hubert.reinterpretcast wrote:
> This is unused, and this should be something like `RELOC_LNNO_OVERFLOW` if we were to have it in this patch.
Yes, removed it from the patch. 

@DiggerLin 
Please note this change might be needed for the upcoming relocation patch. 


================
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))
----------------
hubert.reinterpretcast wrote:
> `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).
Hopefully, I covered all the cases that we needed to call the checker function. 


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:310
+    StatAuxEntPtr =
+        reinterpret_cast<const XCOFFSectAuxEntForStat *>(SymbolEntPtr + 1);
+    printSectAuxEntForStat(StatAuxEntPtr);
----------------
hubert.reinterpretcast wrote:
> This is unsafe. There is no statement I could find indicating that such an auxiliary entry is required, and `xlc` will generate such symbol table entries with no auxiliary entry.
> 
> ```
> [16]    m   0x00000094     .data     1  unamex                    _$STATIC
> [17]    a4  0x00000004       0    0     SD       RW    0    0
> [18]    m   0x00000094     .data     0  static                    x
> ```
We have 

```
if (NumberOfAuxEntries == 0)
    return;
```

before this switch statement. If there is no aux entry, we would exit already before getting here. 


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

https://reviews.llvm.org/D65240