[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.

The convention here is to wait a week or so before a ping, and the same between pings.



================
Comment at: llvm/include/llvm/Object/GOFF.h:54
+      // Check that this block is a Continuation.
+      assert(Record::isContinuation(Slice) && "Continuation bit must be set");
+
----------------
Doesn't this allow for malformed GOFF where a set of continued records isn't properly terminated? 

Also, because the last continued record can be recognized, it isn't necessary for this function to know how much data to expect. This implies that the different card types don't need to send down information on how many continued records are needed. They can get out of the continuation record business totally.


================
Comment at: llvm/include/llvm/Object/GOFF.h:247
+    assert(Length <= ESDMaxUncontinuedNameLength &&
+           "Data too long for ESD Record");
+
----------------
So how does one set a name that is too long for one record?

Why are we dealing with continuation records at this level still?


================
Comment at: llvm/include/llvm/Object/GOFF.h:419
+
+class ENDRecord : public Record {
+public:
----------------
END looks complete enough for now. I hope you'll flesh it out more in a later patch. Same with HDR cards.


================
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; }
----------------
yusra.syeda wrote:
> kpn wrote:
> > Wearing my MVS hat: What's BSS?
> https://en.wikipedia.org/wiki/.bss. There is no equivalent in GOFF, therefore the return value false
Yeah, I was kidding. 

GOFF supports ED symbols larger than zero that could have TXT cards but don't. Said ED is allowed to have LD symbols. At run time space in memory is allocated for the ED, the LD's work correctly, and the memory is filled with the fill byte (typically zero). Which is like how BSS works.

But since GOFF doesn't require that ED's with no TXT be grouped together, the Unix concept of BSS doesn't really apply. Thus returning false is correct. 


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:59
+
+    // Don't parse continuations records, only parse initial record
+    if (IsContinuation)
----------------
jhenderson wrote:
> 
Doesn't this allow for "rogue" continuation records? Records that are marked as continuation records but have no first record appear to be allowed and silently ignored.

Granted, this sort of bug should be rare in practice. It seems most likely with a new GOFF reader/writer. But, of course, that's exactly what we have here. So a check for the error here would be helpful.

Trying to debug bad GOFF where the only diagnostic is a binder abend is... not much fun. That's why I'd like to see easy checks for errors here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98437