[PATCH] D85774: [XCOFF][AIX] Enable tooling support for 64 bit symbol table parsing

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:822
+  if (auto Err = NameOrErr.takeError())
+    return Err;
 
----------------
I believe this requires `std::move(Error);`, as you're returning an `Expected`, not an `Error`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:826-827
+    return createStringError(object_error::parse_failed,
+                             "this csect symbol contains no auxiliary entry: " +
+                                 *NameOrErr);
+  }
----------------
I think it would make more sense to insert the name in the middle of the message to make it a bit more concise. Something like:
"csect symbol `name` contains no auxiliary entry"


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:858
+                           "a csect auxiliary entry is not found for symbol " +
+                               *NameOrErr);
+}
----------------
I'd suggest quoting somehow the symbol name, so that any whitespace or similar that happens to end up in the name (rare, but possible to write using assembly, at least for other platforms) is easily understood to be part of the name.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:391-392
+    if (!ErrOrCsectAuxRef) {
+      handleAllErrors(ErrOrCsectAuxRef.takeError(), [this](ErrorInfoBase &EI) {
+        report_fatal_error(EI.message());
+      });
----------------
Use `reportError` or `reportWarning` so that the error is reported in a clean manner and consistent with other llvm-readobj varieties, and not `report_fatal_error` which looks like a crash. General rule of thumb: try to avoid using `report_fatal_error`, especially in tool code where it is easy to report errors properly.

In llvm-readobj for ELF, we try to avoid even using `reportError` where possible, as that stops the tool from continuing dumping, which can be problematic occasionally. We prefer `reportWarning` (or more specifically the local `reportUniqueWarning` which avoids reporting the same warning multiple times) and bailing out of the current routine. Take a look at ELFDumper.cpp for examples.


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

https://reviews.llvm.org/D85774