[PATCH] D111437: [SystemZ/z/OS] Implement GOFF writer for empty files

Thu Feb 17 00:57:02 PST 2022

jhenderson added a comment.

Basically stylistic-style comments and questions, as I don't know GOFF at all.

================
Comment at: llvm/include/llvm/MC/MCGOFFObjectWriter.h:34
+///
+/// \param MOTW - The target specific GOFF writer subclass.
+/// \param OS - The stream to write to.
----------------

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:68
+// The goff_ostream is responsible to write the data into the fixed physical
+// records of the format. A user of this class announces the begin of a new
+// logical record and the size of its content. While writing the content, the
----------------

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:74
+// writing data in big endian byte order.
+class goff_ostream : public raw_ostream {
+  /// The underlying raw_pwrite_stream.
----------------
My gut says we should be following standard LLVM naming scheme with this class name. The `raw_ostream` class uses the different style because it's intended to mirror the standard `std::ostream` class, but I don't think that's a justification to extend it to classes that make use of the stream.

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:92
+
+  /// Signals begin of new record.
+  bool NewLogicalRecord;
----------------
"begin" is a verb. You want "start" or "beginning".

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:113
+  /// Fill the last physical record of a logical record with zero bytes.
+  void fill_record();
+
----------------
Similar to my above comment, unless the function is an implementation of a virtual function in the basis class, I'd use standard LLVM `lowerCameCase` for the methods throughout this class.

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:138
+
+  // Support for endian specific data.
+  template <typename value_type> void write_be(value_type Value) {
----------------

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:141
+    Value = support::endian::byte_swap<value_type>(Value, support::big);
+    write((const char *)&Value, sizeof(value_type));
+  }
----------------
I think the general preference within LLVM is to use C++ static_cast and reinterpret_cast as appropriate, rather than the C-style cast.

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:176
+  if (Remains) {
+#ifndef NDEBUG
+    assert(Remains == Gap && "Wrong size of fill gap");
----------------
I don't think you need the NDEBUG check here? Isn't that within the assert macro?

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:216-218
+    if (Size) {
+      write_record_prefix(OS, CurrentType, RemainingSize);
+    }
----------------
No braces for single line if.

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:251
+void GOFFObjectWriter::writeHeader() {
+  OS.new_record(GOFF::RT_HDR, /* Size = */ 57);
+  OS.write_zeros(1);        // Reserved
----------------
clang-format understands this style for labelling arguments.

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:271
+
+  OS.new_record(GOFF::RT_END, /* Size = */ 13);
+  OS.write_be<uint8_t>(Flags(6, 2, Flags)); // Indicator flags
----------------

================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:274
+  OS.write_zeros(3);                           // Reserved
+  OS.write_be<uint32_t>(OS.logical_records()); // Record Count
+  OS.write_be<uint32_t>(ESDID);                // ESDID (of entry point)
----------------
kpn wrote:
> Kai wrote:
> > kpn wrote:
> > > Kai wrote:
> > > > kpn wrote:
> > > > > I have found it useful to use the Unix 'dd' command to slide up GOFF files when they cause the Binder to abend. This becomes much harder when this record count field is used. I'm not sure what the upside of this field is.
> > > > I need to check if the binder is happy without this field.
> > > > Both xlc and xlcang set the record count, so setting the field is consistent with these compilers.
> > > We've been putting zero for the record count for nearly 20 years. It's been fine all that time. If it stopped working I'd get a phone call pretty quickly. Last night's builds on V2R4 ran fine, for example.
> > Updated code to just write out the number of logical records for debug purposes.
> Thank you!
It might be worth a comment explaining why this field is always 0. Otherwirse a future contributor may not be aware of the reasoning and change it to be "correct".

================
Comment at: llvm/lib/MC/MCGOFFStreamer.cpp:28-29
+                                     bool RelaxAll) {
+  MCGOFFStreamer *S =
+      new MCGOFFStreamer(Context, std::move(MAB), std::move(OW), std::move(CE));
+  if (RelaxAll)
----------------
This seems like a memory leak?

================
Comment at: llvm/lib/Target/SystemZ/MCTargetDesc/SystemZELFObjectWriter.cpp:58
   switch (Kind) {
   case FK_Data_2:                return ELF::R_390_PC16;
   case FK_Data_4:                return ELF::R_390_PC32;
----------------
Whilst you're moving this file, I'd consider doing a complete clang-format of it too.

================
Comment at: llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmBackend.cpp:84
 public:
-  SystemZMCAsmBackend(uint8_t osABI)
-      : MCAsmBackend(support::big), OSABI(osABI) {}
+  SystemZMCAsmBackend()
+      : MCAsmBackend(support::big) {}
----------------
clang-format the modified code.

================
Comment at: llvm/test/MC/GOFF/empty-goff.s:1
+* RUN: llvm-mc <%s --triple s390x-ibm-zos --filetype=obj -o - | \
+* RUN:   od -Ax -tx1 -v | FileCheck --ignore-case %s
----------------
I'd find this syntax easier to follow, as it's a more common style among tests I've worked with.

================
Comment at: llvm/test/MC/GOFF/empty-goff.s:2
+* RUN: llvm-mc <%s --triple s390x-ibm-zos --filetype=obj -o - | \
+* RUN:   od -Ax -tx1 -v | FileCheck --ignore-case %s
+
----------------
Why do you need the `--ignore-case` option?

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

https://reviews.llvm.org/D111437