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

Fri Oct 9 01:09:51 PDT 2020

jhenderson added a comment.

This patch is too large to review all in one go, and I'm not even looking at the logic detail, because I don't know anything about the file format and don't have time to read up, I'm afraid. You could probably reduce its size by not implementing many of the functions you do, since you don't use them initially. Those that you are requried to due to pure virtual interfaces are best off either left as empty stubs, I reckon, or should have unit tests that test the behaviour.

================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:1
+//===-- llvm/BinaryFormat/GOFF.h - GOFF definitions ---------------*- C++-*-===//
+//
----------------
You need to shorten this comment line by deleting the extra dashes to bring it within the column width (compare other header files to contrast how they work).

================
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.
+//
----------------
Looks like the comment here needs reflowing?

================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:40
+
+// Prefix byte on every record.  This indicates GOFF format.
+enum { PTVPrefix = 0x03 };
----------------

================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:173-174
+
+// RLDRelocationType is internal use only and these values not put
+// in GOFF format
+enum RLDRelocationType {
----------------
I'm not quite sure what this comment is saying. When it says "internal use only" do you mean not defined in any spec, and used only as a helper within the format processing? If so, the enum values should be styled after LLVM coding standard i.e. "UpperCamelCase". If not, you need to clarify what is meant by "internal use only".

================
Comment at: llvm/include/llvm/Object/GOFF.h:36
+
+  GOFFRelocationEntry(uint32_t REsdId_, uint32_t PEsdId_, uint64_t POffset_,
+                      GOFF::RLDReferenceType ReferenceType_,
----------------
Please follow LLVM coding standards and drop the trailing `_`. You don't need it.

================
Comment at: llvm/include/llvm/Object/GOFF.h:61
+
+  // Set PTV fields common to all records
+  void setRecordType(GOFF::RecordType RecordType) {
----------------
Here and throughout, please end your comments with a trailing full stop to conform to the LLVM coding standards.

================
Comment at: llvm/include/llvm/Object/GOFF.h:108
+
+  /// \brief Set two bytes of record, accounting for endianness
+  void set16(uint8_t ByteIndex, uint16_t Value) {
----------------
You state that this accounts for endianness. What happens if the host endianness is different to the target endianness? I suspect you're values will be in the wrong order then.

================
Comment at: llvm/include/llvm/Object/GOFF.h:112
+
+    for (int I = 0; I < 2; ++I) {
+      uint16_t Temp = (Value >> (8 * I)) & 0xFF;
----------------
As you're using this value for an operation in memory, you probably want it to be a `size_t`, rather than a signed integer. Same goes elsewhere below.

================
Comment at: llvm/include/llvm/Object/GOFF.h:128
+
+  // Get routine
+  static void getBits(const uint8_t *Bytes, uint8_t ByteIndex, uint8_t BitIndex,
----------------
This comment should be deleted - it provides no useful value.

================
Comment at: llvm/include/llvm/Object/GOFF.h:140
+
+  static void get8(const uint8_t *Bytes, uint8_t ByteIndex, uint8_t &Value) {
+    assert(ByteIndex < GOFF::RecordLength && "Byte index out of bounds!");
----------------
I've not looked too hard, but I wonder if some of this code could be removed by using the `DataExtractor` class? It provides operations for extracting values from a stream, given an index, and also handles endianness conversion.

================
Comment at: llvm/include/llvm/Object/GOFF.h:530-535
+  // As it stands we call this function with both of these iterators and they
+  // both just happen to be typedef'd to char*. If the underlying implementation
+  // ever changes then these overloads should be used.
+  // void setData(const SmallVectorImpl<char>::const_iterator &Data,
+  //             uint8_t Length);
+  // void setData(const StringRef::const_iterator &Data, uint8_t Length);
----------------
I'm not sure this is a useful comment. The implementation could change in all sorts of ways and break this function. I don't think it's useful to calll out two specific possibilities.

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:33
+
+class GOFFObjectFile : public ObjectFile {
+
----------------
This class is massive. Does it really need to be all implemented up front? For example, could you limit it to the bare minimum needed for the `ObjectFile` interface, and expand on it gradually in follow-up patches as you add things that need more information?

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:51
+
+protected:
+  // SymbolRef
----------------
Why are you using `protected` at all in this class? As far as I can see, there are no sub-classes, so `protected` is no different to `private`, and you should probably prefer the latter.

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:102
+
+  typedef struct {
+    //// == common output
----------------
Don't use `typedef struct`. This isn't C code - use `struct RelocationIteratorState { ... };`

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:103
+  typedef struct {
+    //// == common output
+    DataRefImpl Sec;         // section containing relocation
----------------
I'm not sure what's with the weird comment style. What does it add above normal comments?

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:124
+    bool DoingFuncSymb;
+  } RelocationIteratorState_t;
+
----------------
Don't add `_t` to type names.

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:135
+  // GOFF specific
+public:
+  static GOFF::RLDReferenceType getRLDReferenceType(uint64_t RelocationType);
----------------
Try to avoid repeatedly switching between `private`/`public` etc - put all the private stuff together and all the `public` stuff together. `public` stuff tends to be first, since that's the public API, although it's not that important.

================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:36
+
+GOFFObjectFile::GOFFObjectFile(MemoryBufferRef Object, std::error_code &EC)
+    : ObjectFile(Binary::ID_GOFF, Object),
----------------
Use the fallible constructor idiom here, as described in the [[ https://llvm.org/docs/ProgrammersManual.html#fallible-constructors | LLVM Programmer's Manual ]]. In particular, avoid using `std::error_code` in new code - prefer just using `Expected` and `Error`.

================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:47
+  SectionEntryImpl DummySection;
+  SectionList.emplace_back(DummySection); // dummy entry at index 0
+
----------------
Comments should start with an upper-case letter.

================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:49-50
+
+  const uint8_t *End = reinterpret_cast<const uint8_t *>(Data.getBufferEnd());
+  for (const uint8_t *I = base(); I < End; I += GOFF::RecordLength) {
+    uint8_t RecordType = (I[1] & 0xF0) >> 4;
----------------
As noted earlier - it might be better to use the `DataExtractor` and `Cursor` class to make parsing easier.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89071