[yaml2obj][GOFF] Fix writing GOFF header

[redstar]

The first byte in the header is reserved, but the write method
currently ignores that. Emit the missing zero byte, and update
the tests accordingly.

[llvmbot]

@llvm/pr-subscribers-objectyaml

Author: Kai Nacke (redstar)

Changes

The first byte in the header is reserved, but the write method
currently ignores that. Emit the missing zero byte, and update
the tests accordingly.

---

Full diff: https://github.com/llvm/llvm-project/pull/91383.diff

3 Files Affected:

• (modified) llvm/lib/ObjectYAML/GOFFEmitter.cpp (+2-1)
• (modified) llvm/test/tools/yaml2obj/GOFF/GOFF-header-end.yaml (+1-1)
• (modified) llvm/test/tools/yaml2obj/GOFF/GOFF-header-settings.yaml (+1-1)

diff --git a/llvm/lib/ObjectYAML/GOFFEmitter.cpp b/llvm/lib/ObjectYAML/GOFFEmitter.cpp
index 345904407e1d24..47550ac971980e 100644
--- a/llvm/lib/ObjectYAML/GOFFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/GOFFEmitter.cpp
@@ -219,7 +219,8 @@ void GOFFState::writeHeader(GOFFYAML::FileHeader &FileHdr) {
   }
 
   GW.makeNewRecord(GOFF::RT_HDR, GOFF::PayloadLength);
-  GW << binaryBe(FileHdr.TargetEnvironment)     // TargetEnvironment
+  GW << zeros(1)                                // Reserved
+     << binaryBe(FileHdr.TargetEnvironment)     // TargetEnvironment
      << binaryBe(FileHdr.TargetOperatingSystem) // TargetOperatingSystem
      << zeros(2)                                // Reserved
      << binaryBe(FileHdr.CCSID)                 // CCSID
diff --git a/llvm/test/tools/yaml2obj/GOFF/GOFF-header-end.yaml b/llvm/test/tools/yaml2obj/GOFF/GOFF-header-end.yaml
index a5e99c2da2c491..8fab5f6ddad409 100644
--- a/llvm/test/tools/yaml2obj/GOFF/GOFF-header-end.yaml
+++ b/llvm/test/tools/yaml2obj/GOFF/GOFF-header-end.yaml
@@ -4,7 +4,7 @@
 # CHECK:      03 f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 # CHECK-NEXT: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 # CHECK-NEXT: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
-# CHECK-NEXT: 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00 00
+# CHECK-NEXT: 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00 00
 # CHECK-NEXT: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 
 ## Verify GOFF Module end.
diff --git a/llvm/test/tools/yaml2obj/GOFF/GOFF-header-settings.yaml b/llvm/test/tools/yaml2obj/GOFF/GOFF-header-settings.yaml
index 1971c407199fbe..74a2a08f771322 100644
--- a/llvm/test/tools/yaml2obj/GOFF/GOFF-header-settings.yaml
+++ b/llvm/test/tools/yaml2obj/GOFF/GOFF-header-settings.yaml
@@ -4,7 +4,7 @@
 # CHECK:      03 f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 # CHECK-NEXT: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 # CHECK-NEXT: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
-# CHECK-NEXT: 00 00 01 00 03 00 00 00 00 00 00 00 00 00 00 00
+# CHECK-NEXT: 00 00 00 01 00 03 00 00 00 00 00 00 00 00 00 00
 # CHECK-NEXT: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
 
 ## Verify GOFF Module end.

[jh7370]

Sorry for the delay - I was off last week. Could you point me at the specification you're using for GOFF? I'm struggling to marry this code up with anything I can immediately find.

[redstar]

No worries, I had a couple of vacation days myself.

The main documentation entry is Generalized object file format (GOFF), and the description of the header record is Module header record. There is only the architecture level in the module header, the other fields are just proposals. I'm happy to remove those - they really slipped through. The problem here is that the architecture level field is off by one, that's how I noted the problem.

Reading through the comments on Yusra's PR I believe that the implementation still does not have the required flexibility. The GOFF file format is basically a sequence of records. There are certain requirements (like header comes first, a symbol must be defined before it is use, end records comes last), but this is not enforced through the physical structure of the file. With the current approach it is not possible to omit the end record, or have the end record defined first. I changed the implementation to be more flexible, and will put up the PR soon. I would include this change, and would like to remove the undocumented fields if this is ok for you. With the new approach, it is possible to write a single record, which simplifies testing, because it reduces the "hex desert" needed to be checked.

[jh7370]

No worries, I had a couple of vacation days myself.

The main documentation entry is Generalized object file format (GOFF), and the description of the header record is Module header record. There is only the architecture level in the module header, the other fields are just proposals. I'm happy to remove those - they really slipped through. The problem here is that the architecture level field is off by one, that's how I noted the problem.

Reading through the comments on Yusra's PR I believe that the implementation still does not have the required flexibility. The GOFF file format is basically a sequence of records. There are certain requirements (like header comes first, a symbol must be defined before it is use, end records comes last), but this is not enforced through the physical structure of the file. With the current approach it is not possible to omit the end record, or have the end record defined first. I changed the implementation to be more flexible, and will put up the PR soon. I would include this change, and would like to remove the undocumented fields if this is ok for you. With the new approach, it is possible to write a single record, which simplifies testing, because it reduces the "hex desert" needed to be checked.

Thanks for the suggestions, they all sound reasonable to me at this point.

[redstar]

See #93855.