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

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

kpn added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:16-17
+// 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.
+//
----------------
jhenderson wrote:
> Looks like the comment here needs reflowing?
I can't find any document with Google that describes a GOFF-specific TIS, and Google also has trouble with "GOFF64" and "GOFF-64". Those version numbers I guess refer to that missing document.

Can you instead reference the exact book from IBM's "z/OS Internet Library" that describes GOFF? Include the full name, the edition number, and the year please. Then it's easy to look up the GOFF spec.


================
Comment at: llvm/include/llvm/BinaryFormat/GOFFAda.def:9
+//
+// This file centralizes the ADA definitions used by GOFF and XPLINK.
+//
----------------
Please expand "ADA" here at the top. This appears to be for "Extended Attributes"?


================
Comment at: llvm/include/llvm/Object/GOFF.h:137
+    setArchitectureLevel(1);
+  }
+
----------------
Missing module properties field. 


================
Comment at: llvm/include/llvm/Object/GOFF.h:156
+
+  void setDataLength(uint16_t Length) { set<uint16_t>(22, Length); }
+
----------------
I don't see any support for "Text Encoding". These fields are at bytes 16-21. Maybe a comment if you don't plan on ever implementing it?


================
Comment at: llvm/include/llvm/Object/GOFF.h:223
+  // This intentionally uses the same field as above.
+  void setERSymbolType(GOFF::ESDERSymbolType Type) { setBits(41, 5, 3, Type); }
+
----------------
Is this right? Bit 41.3 in "Program Management" is the "Removable Class Flag", which matches the code above this. Bit 41.4-6 are marked reserved, and bit 41.7 is unnamed but if set means "Reserve 16 bytes at beginning of class. MRG class ED records only." 

So it looks like the code is writing to the wrong part of byte 41. The code that reads from that byte also appears incorrect. Shouldn't it be (41, 4, 3)?


================
Comment at: llvm/include/llvm/Object/GOFF.h:227
+
+  void setAdaEsdId(uint32_t EsdId) { set<uint32_t>(44, EsdId); }
+
----------------
This is the "Associated data ID". It's confusing having "Ada" and both "ADA" but I don't think they're related?


================
Comment at: llvm/include/llvm/Object/GOFF.h:266
+
+  void setIndirectReference(bool Indirect) {
+    uint8_t Value = Indirect ? 1 : 0;
----------------
No COMMON flag?


================
Comment at: llvm/include/llvm/Object/GOFF.h:487
+    set<uint32_t>(Offset + 16, Ent.POffset);
+  }
+
----------------
I assume RLD continuation records and relocation compression are coming later.


================
Comment at: llvm/include/llvm/Object/GOFF.h:530
+
+  void setRecordCount(uint32_t RecordCount) { set<uint32_t>(8, RecordCount); }
+};
----------------
For completeness this is good. But please don't ever use it. 

When the Binder abends I can't tell you how useful it is to use the Unix "dd" command to slice up GOFF until the abend goes away. That's how I've had to shoot down a number of bugs in emitting GOFF. But that technique doesn't work if the END card's record count field is used. 


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:262
+    Expected<StringRef> Name = getSymbolName(Symb);
+    if (Name && !Name->equals(" ")) { // blank name is local
+      Flags |= SymbolRef::SF_Global;
----------------
jhenderson wrote:
> Do you mean specifically " " means local? What about "", "  " (i.e. 0, 2 spaces) etc?
It's " " that's special. My employeer's compiler uses the same symbol name because the Binder translates it into a private symbol name that uses characters. In this way multiple private symbols can be disambiguated in a listing after a link.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89071