[PATCH] D89071: [SystemZ/z/OS] Add GOFFObjectFile class and details of GOFF file format

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:53
+
+enum ESDSymbolType {
+  ESD_ST_SectionDefinition = 0,
----------------
Does the GOFF spec specify the size of symbol types and other things like that? If so, I'd use the explicit size for these fields. For example, if symbol type is guaranteed to be a single byte, you might adopt my inline edit. The same goes for each other enum below.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:199-203
+  Expected<StringRef> NameOrErr = getSymbolName(Symbol.getRawDataRefImpl());
+  if (NameOrErr) {
+    return *NameOrErr;
+  }
+  return NameOrErr.takeError();
----------------
Actually, better yet, I think you can simplify this down to a single line as suggested in the edit.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:292-293
+    return createStringError(llvm::errc::invalid_argument,
+                             "ESD record %lu has invalid symbol type %02X",
+                             (unsigned long)EsdId, SymbolType);
+  }
----------------
Rather than casting `EsdId`, use the correct print format specifier.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:308-309
+          llvm::errc::invalid_argument,
+          "ESD record %lu has unknown Executable type %02X",
+          (unsigned long)EsdId, Executable);
+    }
----------------
Same as above.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:136
+    case GOFF::RT_LEN:
+      LLVM_DEBUG(dbgs() << "  --  LEN (GOFF record type) unhandled\n");
+      break;
----------------
jhenderson wrote:
> This sounds like it should be an error?
I think this comment was referring to the "unhandled" bits below. It's been marked as done, but I don't see any response. Could you clarify more why this isn't a hard error and instead such things are being ignored?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89071