[PATCH] D98003: [obj2yaml] Implement parsing sections and auxiliary entries of XCOFF.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You'll need someone with XCOFF experience to review this too. Generally looks fine from my brief look though.



================
Comment at: llvm/test/tools/obj2yaml/XCOFF/aix.yaml:1
 # RUN: obj2yaml %S/Inputs/aix_xcoff.o | FileCheck %s
 # Test that we can parse the XCOFF object file correctly.
----------------
It would be nice if we could replace the canned binary input with a YAML input and then use yaml2obj to produce the YAML for consumption by obj2yaml at some point in the future. That'll allow us proper flexibility to test all code paths.


================
Comment at: llvm/test/tools/obj2yaml/XCOFF/aix.yaml:13
 
+# CHECK: Sections:
+# CHECK-NEXT:   - Name:            .text
----------------
Nit: add extra spaces to make things line up nicely.

Should this be a CHECK-NEXT though? If so, delete the blank line above it.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:37-39
+  if (Error E = dumpSymbols())
+    return E;
+  return Error::success();
----------------



================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:61
+  if (Obj.is64Bit())
+    report_fatal_error("64-bit XCOFF files not supported yet.");
+  else
----------------
Perhaps report this as a regular error using `createStringError()`? No need for `report_fatal_error` when we have an easy way to report the `Error` after all.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:62-63
+    report_fatal_error("64-bit XCOFF files not supported yet.");
+  else
+    return dumpSections(Obj.sections32());
+}
----------------
No need for the `else` here.


================
Comment at: llvm/tools/obj2yaml/xcoff2yaml.cpp:82
+      // Only .text and .data sections have raw data.
+      if (S.Flags != XCOFF::STYP_TEXT && S.Flags != XCOFF::STYP_DATA)
+        return createStringError(std::errc::invalid_argument,
----------------
This and the similar relocation check below - do you actually need it? If a user had an invalid object, where the section type was different to what it should be (e.g. a bug in an object producer), this check would prevent using obj2yaml, yet in my mind, the YAML could easily represent this state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98003