[PATCH] D150613: [SystemZ][z/OS] Implement yaml2obj for GOFF Object File Format

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

kpn added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/GOFF.h:33
+/// number of bytes we allow ourselves to attach to a card is thus arbitrarily
+/// limited to 32K-1 bytes.
+constexpr uint16_t MaxDataLength = 32 * 1024 - 1;
----------------
Everybody0523 wrote:
> kpn wrote:
> > Has the spec been updated in the past three years? My z/OS 2.4 copy doesn't say that RLD records have an unsigned length field.
> > 
> > Also, if the spec is wrong about TXT cards then correcting the spec seems helpful. I don't know how difficult this is in practice.
> The spec isn't wrong per-say, this is essentially a work-around for a binder bug.
A rewording of the comment so it says there's a binder bug instead of saying, essentially, that the spec is wrong would be helpful.


================
Comment at: llvm/include/llvm/ObjectYAML/GOFFYAML.h:35
+enum {
+  ESD_Mask_ERST = 0x07,
+  ESD_Mask_RQW = 0x07,
----------------
This appears to be unused. I don't know what "ERST" stands for, either.


================
Comment at: llvm/include/llvm/ObjectYAML/GOFFYAML.h:36
+  ESD_Mask_ERST = 0x07,
+  ESD_Mask_RQW = 0x07,
+  ESD_Mask_TextStyle = 0xf0,
----------------
Perhaps a comment on what "RQW" means? It looks like it masks out bits that are currently reserved. 


================
Comment at: llvm/include/llvm/ObjectYAML/GOFFYAML.h:53
+  RLD_Same_Offset = 0x20,
+  RLD_EA_Present = 0x04,
+  RLD_Offset_Length = 0x02,
----------------
Is this undocumented?


================
Comment at: llvm/include/llvm/ObjectYAML/GOFFYAML.h:55
+  RLD_Offset_Length = 0x02,
+  RLD_Adressing_Mode_Sensitivity = 0x01,
+  RLD_FetchStore = 0x100,
----------------
Typo: Addressing


================
Comment at: llvm/include/llvm/ObjectYAML/GOFFYAML.h:56
+  RLD_Adressing_Mode_Sensitivity = 0x01,
+  RLD_FetchStore = 0x100,
+};
----------------
How does this work? The other flags are bits in the same byte. 


================
Comment at: llvm/lib/ObjectYAML/GOFFEmitter.cpp:250
+     << zeros(16 - LangProd.size())             // Fill bytes
+     << binaryBe(FileHdr.ArchitectureLevel);    // ArchitectureLevel
+  // The module propties are optional. Figure out if we need to write them
----------------
I couldn't find any documentation on any of this aside from the Architecture Level field. Same issue with the module properties field. So I can't say anything about the correctness of this code.


================
Comment at: llvm/lib/ObjectYAML/GOFFEmitter.cpp:309
+                         BIT(ESD_BA_ReadOnly, 4) | Sym.Executable))
+     << binaryBe(uint8_t(BIT(ESD_BA_NoPrime, 1) | Sym.BindingStrength))
+     << binaryBe(uint8_t(Sym.LoadingBehavior << 6 | BIT(ESD_BA_COMMON, 2) |
----------------
Shouldn't this be the duplicate symbol severity? What is ESD_BA_NoPrime?


================
Comment at: llvm/lib/ObjectYAML/GOFFYAML.cpp:222
+  BCase(RLD_Offset_Length);
+  BCase(RLD_Adressing_Mode_Sensitivity);
+  BCase(RLD_FetchStore);
----------------
Addressing. This is probably in other places as well.


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

https://reviews.llvm.org/D150613