[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/tools/llvm-readobj/XCOFFDumper.cpp:240
+  if (Obj.is64Bit())
+    report_fatal_error("64-bit support is unimplmented.");
+
----------------
s/unimplmented/unimplemented/;


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:270
+
+  W.printEnum("StorageClass", (uint8_t)SymbolEntPtr->StorageClass,
+              makeArrayRef(SymStorageClass));
----------------
Use `static_cast`.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:279
+  case XCOFF::C_FILE:
+    // If the symbol is C_FILE and has auxiliary entires.
+    if (SymbolName.equals(".file"))
----------------
s/entires/entries/;
Replace the period with an ellipsis. What is here is not a sentence.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:280
+    // If the symbol is C_FILE and has auxiliary entires.
+    if (SymbolName.equals(".file"))
+      for (int i = 1; i <= NumberOfAuxEntries; i++) {
----------------
Yes, the name //should// be ".file", but I don't think there is sufficient rationale to skip processing the auxiliary entries if the name isn't.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:290
+  case XCOFF::C_HIDEXT:
+    // If the symbol is Function.
+    if (XCOFFSymRef.isFunction() && NumberOfAuxEntries >= 2)
----------------
s/is Function/is for a function/;


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:294
+
+    // If more than 2,Instead of printing out error information,
+    // Print out the raw Auxiliary entry between 2 and NumberOfAuxEntries-1
----------------
Can we have some rationale here for not printing the first auxiliary entry if it is not also the last?
Also, please fix the spacing, capitalization, etc.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:297
+    for (int i = 2; i < NumberOfAuxEntries; i++) {
+      W.startLine() << "!Unexpected raw auxiliary entry data are print out:\n";
+      W.startLine() << format_bytes(ArrayRef<uint8_t>(
----------------
Remove the " are print out" part. It is clear that it is being printed out.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:299
+      W.startLine() << format_bytes(ArrayRef<uint8_t>(
+          (const uint8_t *)(SymbolEntPtr + i), sizeof(XCOFFSymbolEntry)));
+    }
----------------
`SYM_TAB_ENTRY_SIZE` might make more sense (since we are not really talking about an `XCOFFSymbolEntry` object).


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:299
+      W.startLine() << format_bytes(ArrayRef<uint8_t>(
+          (const uint8_t *)(SymbolEntPtr + i), sizeof(XCOFFSymbolEntry)));
+    }
----------------
hubert.reinterpretcast wrote:
> `SYM_TAB_ENTRY_SIZE` might make more sense (since we are not really talking about an `XCOFFSymbolEntry` object).
Use `reinterpret_cast`.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:310
+    StatAuxEntPtr =
+        reinterpret_cast<const XCOFFSectAuxEntForStat *>(SymbolEntPtr + 1);
+    printSectAuxEntForStat(StatAuxEntPtr);
----------------
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
```


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:321
+    for (int i = 1; i <= NumberOfAuxEntries; i++) {
+      W.startLine() << "!Unexpected raw auxiliary entry data are print out:\n";
+      W.startLine() << format_bytes(ArrayRef<uint8_t>(
----------------
See comments above.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:323
+      W.startLine() << format_bytes(ArrayRef<uint8_t>(
+          (const uint8_t *)(SymbolEntPtr + i), sizeof(XCOFFSymbolEntry)));
+    }
----------------
See comments above.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:327
+  }
+}
 void XCOFFDumper::printSymbols() {
----------------
Add a newline between the functions.


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

https://reviews.llvm.org/D65240