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

[Yusra Syeda via Phabricator via llvm-commits]

yusra.syeda marked 5 inline comments as done.
yusra.syeda added a comment.

In D98437#2672051 <https://reviews.llvm.org/D98437#2672051>, @kpn wrote:

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

Thanks for letting me know, I wasn't aware of this.



================
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");
+
----------------
kpn wrote:
> 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.
It is possible that the last continuation is not completely filled with data - this is why I pass how much data to expect to this function. I did add a check to ensure the last continuation is terminated properly.


================
Comment at: llvm/include/llvm/Object/GOFF.h:247
+    assert(Length <= ESDMaxUncontinuedNameLength &&
+           "Data too long for ESD Record");
+
----------------
kpn wrote:
> 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?
This function is not needed for this patch. I removed it and will update the functionality for the future patches. 


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:59
+
+    // Don't parse continuations records, only parse initial record
+    if (IsContinuation)
----------------
kpn wrote:
> 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.
I've added the error checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98437