[PATCH] D98437: [SystemZ][z/OS] Add GOFFObjectFile class support for HDR, ESD and END records

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:12
+//
+// GOFF specifics can be found in MVS Program Management: Advanced Facilities
+//
----------------
Nit: add trailing `.` to the sentence.


================
Comment at: llvm/include/llvm/Object/GOFF.h:35
+public:
+  Record() : Bytes(GOFF::RecordLength, (char)0x00) {
+    set<uint8_t>(0, GOFF::PTVPrefix);
----------------
Any particular reason you've used `(char)0x00` rather than simply `\0`?


================
Comment at: llvm/include/llvm/Object/GOFF.h:39-40
+
+  static void getContinuousData(const uint8_t *Record, uint16_t DataLength,
+                                int DataIndex, SmallString<256> &CompleteData) {
+    // First record.
----------------
This and several other methods within this file look like they belong in the .cpp file. We should avoid pointing non-trivial functions in the header.


================
Comment at: llvm/include/llvm/Object/GOFF.h:52
+         DataLength -= SliceLength, Slice += GOFF::PayloadLength) {
+      // Slice points to the begin of the new record.
+      // Check that this block is a Continuation.
----------------
"begin" -> either "beginning" or "start"

"begin" is a verb, not a noun.


================
Comment at: llvm/include/llvm/Object/GOFF.h:54-59
+      assert(Record::isContinuation(Slice) && "Continuation bit must be set");
+      if (DataLength <= 77)
+        // This is the last Continuation.
+        // Check that it is terminated correctly.
+        assert(!Record::isContinued(Slice) &&
+               "Continued bit should not be set");
----------------
I'm just trying to understand these assertions here. Could a malformed input potentially cause these assertions to trigger, or will such malformed data be detected earlier in the program and this code path avoided? If these asserts could fire with malformed data, then they should not be assertions, but proper error checks.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:276-279
+    return createStringError(llvm::errc::invalid_argument,
+                             "ESD record %" PRIu32
+                             " has invalid symbol type 0x%" PRIX8,
+                             EsdId, SymbolType);
----------------
Test case?


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:292-295
+      return createStringError(llvm::errc::invalid_argument,
+                               "ESD record %" PRIu32
+                               " has unknown Executable type 0x%02X",
+                               EsdId, Executable);
----------------
Test case?


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:335-336
+  }
+  return createStringError(llvm::errc::invalid_argument,
+                           "no symbol section found");
+}
----------------
Test case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98437