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

[Kevin P. Neal via Phabricator via llvm-commits]

kpn added a comment.

You'll need to add much comprehensive testing like @jhenderson requested in the previous ticket.



================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:16
+// Version 1.2, May 1995. The GOFF64 specifics are based on GOFF-64 Object File
+// Format Version 1.5, Draft 2, May 1998 as well as OpenBSD header files.
+//
----------------
I went looking for this TIS document once. I never found it. And I have no idea what GOFF32 and GOFF64 is since all three amodes and rmodes can be combined in a single object. I'd drop the comments about TIS specification, and any other documents, if they aren't available on the public Internet.


================
Comment at: llvm/include/llvm/Object/GOFF.h:129
+
+  void setArchitectureLevel(uint32_t Level) { set<uint32_t>(48, Level); }
+};
----------------
Module properties go here, and continuation HDR records are allowed I'm pretty sure.


================
Comment at: llvm/include/llvm/Object/GOFF.h:143
+  /// 16 bit SIGNED length field.
+  static const uint16_t MaxNameLength = 32 * 1024;
+
----------------
Since this field is signed, isn't it 32*1024-1? The assert() checks seem to assume the value has the "-1" in it. Otherwise they'd check for "<" and not "<=".

Does any program that isn't a GOFF card dumping tool care if continuation records are used? Why keep this constant (ESDMaxNameLength) as part of the ESDRecord class? Plus, the names of these two constants are too similar. I'd drop "ESDMaxNameLength" and then give "MaxNameLength" the correct value. Or rename ESDMaxNameLength to be something along the lines of "MaxUncontinuedNameLength" if you want to keep both. It's confusing the way it is now.


================
Comment at: llvm/include/llvm/Object/GOFF.h:405
+
+class ContinuationRecord : public Record {
+public:
----------------
Why make ContinuationRecord appear to be a type of record like ESD, HDR, and END are types of record? I'd have a layer that deals with cards and presents to the layer above it all payload data from initial and continued records combined. Then the code handling the various types of record wouldn't have to know about continuation records.


================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:97
+  bool isSectionData(DataRefImpl Sec) const override { return false; }
+  bool isSectionBSS(DataRefImpl Sec) const override { return false; }
+  bool isSectionVirtual(DataRefImpl Sec) const override { return false; }
----------------
Wearing my MVS hat: What's BSS?


================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:112
+  // RelocationRef.
+  void moveRelocationNext(DataRefImpl &Rel) const override {};
+  uint64_t getRelocationOffset(DataRefImpl Rel) const override { return 0; }
----------------
Isn't this for RLD cards and thus should be in the commit that adds support for them?


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:62
+    if (IsContinuation)
+      continue;
+
----------------
This is what I mentioned above. It looks like this requires that each type of record know how to handle continuation records themselves. And I don't see any support for HDR or END continuation records. Continuation records really need to be handled uniformly and below this level.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98437