Add --offoading option to llvm-readobj

[david-salinas]

Utilize new extensions to LLVM Offloading API to
handle offloading fatbin Bundles.

The tool will output a list of available offload bundles
using URI syntax.

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-backend-amdgpu

Author: David Salinas (david-salinas)

Changes

Utilize new extensions to LLVM Offloading API to
handle offloading fatbin Bundles.

The tool will output a list of available offload bundles
using URI syntax.

Change-Id: Ib24a722be16f499ffb56bf46e4b82e90d8775040

---

Patch is 22.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143342.diff

7 Files Affected:

• (modified) llvm/docs/CommandGuide/llvm-readobj.rst (+4)
• (modified) llvm/include/llvm/Object/OffloadBundle.h (+1-1)
• (added) llvm/test/tools/llvm-readobj/ELF/AMDGPU/offloading.test (+42)
• (modified) llvm/tools/llvm-readobj/ObjDumper.cpp (+19)
• (modified) llvm/tools/llvm-readobj/ObjDumper.h (+2)
• (modified) llvm/tools/llvm-readobj/Opts.td (+3)
• (modified) llvm/tools/llvm-readobj/llvm-readobj.cpp (+5)

diff --git a/llvm/docs/CommandGuide/llvm-readobj.rst b/llvm/docs/CommandGuide/llvm-readobj.rst
index 8bd29eafbbfcf..faaddb4699f7d 100644
--- a/llvm/docs/CommandGuide/llvm-readobj.rst
+++ b/llvm/docs/CommandGuide/llvm-readobj.rst
@@ -104,6 +104,10 @@ file formats.
  Do not demangle symbol names in the output. This option is only for ELF and
  XCOFF file formats. The option is enabled by default.
 
+.. option:: --offloading
+
+ Display list of HIP Offload bundles using URI syntax.
+
 .. option:: --relocations, --relocs, -r
 
  Display the relocation entries in the file.
diff --git a/llvm/include/llvm/Object/OffloadBundle.h b/llvm/include/llvm/Object/OffloadBundle.h
index f4d5a1d878b8d..18be62b10c518 100644
--- a/llvm/include/llvm/Object/OffloadBundle.h
+++ b/llvm/include/llvm/Object/OffloadBundle.h
@@ -161,7 +161,7 @@ struct OffloadBundleURI {
     OffsetStr.getAsInteger(10, O);
     Str = Str.drop_front(OffsetStr.size());
 
-    if (Str.consume_front("&size="))
+    if (!Str.consume_front("&size="))
       return createStringError(object_error::parse_failed,
                                "Reading 'size' in URI");
 
diff --git a/llvm/test/tools/llvm-readobj/ELF/AMDGPU/offloading.test b/llvm/test/tools/llvm-readobj/ELF/AMDGPU/offloading.test
new file mode 100644
index 0000000000000..9656172f2941c
--- /dev/null
+++ b/llvm/test/tools/llvm-readobj/ELF/AMDGPU/offloading.test
@@ -0,0 +1,42 @@
+## Test that --offloading with a fatbin works correctly
+# REQUIRES: target={{x86_64-.*-linux.*}}
+# REQUIRES: amdgpu-registered-target
+
+# RUN: yaml2obj %s -o %t.elf
+# RUN: llvm-readobj --offloading %t.elf > %t.out 
+# RUN: FileCheck %s --input-file=%t.out -DFILE_NAME=%t.elf
+
+# CHECK:        host-x86_64-unknown-linux--     file://[[FILE_NAME]]#offset=8192&size=0
+# CHECK-NEXT:   hipv4-amdgcn-amd-amdhsa--gfx908 file://[[FILE_NAME]]#offset=8192&size=4048
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x2041B0
+ProgramHeaders:
+  - Type:            PT_PHDR
+    Flags:           [ PF_R ]
+    VAddr:           0x200040
+    Align:           0x8
+    Offset:          0x40
+  - Type:            PT_GNU_STACK
+    Flags:           [ PF_W, PF_R ]
+    Align:           0x0
+    Offset:          0x0
+Sections:
+  - Name:            .hip_fatbin
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x201000
+    AddressAlign:    0x1000
+    Content:         5F5F434C414E475F4F46464C4F41445F42554E444C455F5F0200000000000000001000000000000000000000000000001B00000000000000686F73742D7838365F36342D756E6B6E6F776E2D6C696E75782D2D0010000000000000D00F0000000000001F0000000000000068697076342D616D6467636E2D616D642D616D646873612D2D676678393038000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007F454C460201014003000000000000000300E0000100000000000000000000004000000000000000100C0000000000003005000040003800090040000F000D000600000004000000400000000000000040000000000000004000000000000000F801000000000000F80100000000000008000000000000000100000004000000000000000000000000000000000000000000000000000000C008000000000000C008000000000000001000000000000001000000050000000009000000000000001900000000000000190000000000006C000000000000006C00000000000000001000000000000001000000060000007009000000000000702900000000000070290000000000007000000000000000900600000000000000100000000000000100000006000000E009000000000000E039000000000000E039000000000000000000000000000001000000000000000010000000000000020000000600000070090000000000007029000000000000702900000000000070000000000000007000000000000000080000000000000052E574640400000070090000000000007029000000000000702900000000000070000000000000009006000000000000010000000000000051E57464060000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000400000004000000380200000000000038020000000000003802000000000000340500000000000034050000000000000400000000000000070000001D05000020000000414D44475055000083AE616D646873612E6B65726E656C7391DE0012AB2E616770725F636F756E7400A52E61726773DC001085AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65AA415F642E636F65726365A72E6F666673657400A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F62756666657285AE2E616464726573735F7370616365A6676C6F62616CA52E6E616D65AA425F642E636F65726365A72E6F666673657408A52E73697A6508AB2E76616C75655F6B696E64AD676C6F62616C5F62756666657284A52E6E616D65A14EA72E6F666673657410A52E73697A6508AB2E76616C75655F6B696E64A862795F76616C756583A72E6F666673657418A52E73697A6504AB2E76616C75655F6B696E64B468696464656E5F626C6F636B5F636F756E745F7883A72E6F66667365741CA52E73697A6504AB2E76616C75655F6B696E64B468696464656E5F626C6F636B5F636F756E745F7983A72E6F666673657420A52E73697A6504AB2E76616C75655F6B696E64B468696464656E5F626C6F636B5F636F756E745F7A83A72E6F666673657424A52E73697A6502AB2E76616C75655F6B696E64B368696464656E5F67726F75705F73697A655F7883A72E6F666673657426A52E73697A6502AB2E76616C75655F6B696E64B368696464656E5F67726F75705F73697A655F7983A72E6F666673657428A52E73697A6502AB2E76616C75655F6B696E64B368696464656E5F67726F75705F73697A655F7A83A72E6F66667365742AA52E73697A6502AB2E76616C75655F6B696E64B268696464656E5F72656D61696E6465725F7883A72E6F66667365742CA52E73697A6502AB2E76616C75655F6B696E64B268696464656E5F72656D61696E6465725F7983A72E6F66667365742EA52E73697A6502AB2E76616C75655F6B696E64B268696464656E5F72656D61696E6465725F7A83A72E6F666673657440A52E73697A6508AB2E76616C75655F6B696E64B668696464656E5F676C6F62616C5F6F66667365745F7883A72E6F666673657448A52E73697A6508AB2E76616C75655F6B696E64B668696464656E5F676C6F62616C5F6F66667365745F7983A72E6F666673657450A52E73697A6508AB2E76616C75655F6B696E64B668696464656E5F676C6F62616C5F6F66667365745F7A83A72E6F666673657458A52E73697A6502AB2E76616C75655F6B696E64B068696464656E5F677269645F64696D73B92E67726F75705F7365676D656E745F66697865645F73697A6500B62E6B65726E6172675F7365676D656E745F616C69676E08B52E6B65726E6172675F7365676D656E745F73697A65CD0118A92E6C616E6775616765A84F70656E434C2043B12E6C616E67756167655F76657273696F6E920200B82E6D61785F666C61745F776F726B67726F75705F73697A65CD0400A52E6E616D65B25F5A3973696D706C65416464506A504B6A6DBB2E707269766174655F7365676D656E745F66697865645F73697A6500AB2E736770725F636F756E740CB12E736770725F7370696C6C5F636F756E7400A72E73796D626F6CB55F5A3973696D706C65416464506A504B6A6D2E6B64B82E756E69666F726D5F776F726B5F67726F75705F73697A6501B32E757365735F64796E616D69635F737461636BC2AB2E766770725F636F756E7404B12E766770725F7370696C6C5F636F756E7400AF2E7761766566726F6E745F73697A6540AD616D646873612E746172676574B9616D6467636E2D616D642D616D646873612D2D676678393038AE616D646873612E76657273696F6E92010200000000000000000000000000000000000000000000000000000000000000010000001203070000190000000000006C000000000000001400000011030600800800000000000040000000000000002A00000011000A00E03900000000000001000000000000000100000001000000010000001A000000000008400000D20001000000360A4A7A5238A4D3F113F4DD04000000040000000200000001000000000000000300000000000000000000000000000000000000005F5A3973696D706C65416464506A504B6A6D005F5A3973696D706C65416464506A504B6A6D2E6B64005F5F6869705F637569645F623730363264386333326134613933330000000000000000000000000000000000000000000000000000000000000000000000180100000000000080100000000000000000000000000000000000000000000000000000000000004000AF008C000000090000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000C20102C02400000002000AC0000000008002027E7FC08CBF07FF0486FFFF0000060406920600006800008FD2820002000302067E0200043203030638008050DC02007F020102067E0000003203030238008050DC00007F03700F8CBF03050468008070DC00027F00000081BF00000000060000000000000070070000000000000B000000000000001800000000000000050000000000000020080000000000000A000000000000004600000000000000F5FEFF6F00000000D0070000000000000400000000000000F807000000000000000000000000000000000000000000004C696E6B65723A20414D44204C4C442031392E302E3000414D4420636C616E672076657273696F6E2031392E302E306769742028202032343231322063393630313665636534313337356462646438663037356266333762643666633333323230376233290000414D4420636C616E672076657273696F6E2031382E302E3067697420287373683A2F2F6765727269746769742F6C696768746E696E672F65632F6C6C766D2D70726F6A65637420616D642D6D61696E6C696E652D6F70656E20323431373620663935303039613166393032313232343865313036333964653837653635636163616338643961372900000000000000000000000000000000000000000000000000460000000002080070290000000000000000000000000000010000001203070000190000000000006C000000000000001400000011030600800800000000000040000000000000002A00000011000A00E0390000000000000100000000000000002E6E6F7465002E64796E73796D002E676E752E68617368002E68617368002E64796E737472002E726F64617461002E74657874002E64796E616D6963002E72656C726F5F70616464696E67002E627373002E636F6D6D656E74002E73796D746162002E7368737472746162002E73747274616200005F5A3973696D706C65416464506A504B6A6D005F5A3973696D706C65416464506A504B6A6D2E6B64005F5F6869705F637569645F62373036326438633332613461393333005F44594E414D494300000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000000070000000200000000000000380200000000000038020000000000003405000000000000000000000000000004000000000000000000000000000000070000000B00000002000000000000007007000000000000700700000000000060000000000000000500000001000000080000000000000018000000000000000F000000F6FFFF6F0200000000000000D007000000000000D007000000000000280000000000000002000000000000000800000000000000000000000000000019000000050000000200000000000000F807000000000000F80700000000000028000000000000000200000000000000040000000000000004000000000000001F000000030000000200000000000000200800000000000020080000000000004600000000000000000000000000000001000000000000000000000000000000270000000100000002000000000000008008000000000000800800000000000040000000000000000000000000000000400000000000000000000000000000002F000000010000000600000000000000001900000000000000090000000000006C00000000000000000000000000000000010000000000000000000000000000350000000600000003000000000000007029000000000000700900000000000070000000000000000500000000000000080000000000000010000000000000003E000000080000000300000000000000E029000000000000E00900000000000020060000000000000000000000000000010000000000000000000000000000004D000000080000000300000000000000E039000000000000E0090000000000000100000000000000000000000000000001000000000000000000000000000000520000000100000030000000000000000000000000000000E009000000000000F0000000000000000000000000000000010000000000000001000000000000005B0000000200000000000000000000000000000000000000D00A00000000000078000000000000000E0000000200000008000000000000001800000000000000630000000300000000000000000000000000000000000000480B00000000000075000000000000000000000000000000010000000000000000000000000000006D0000000300000000000000000000000000000000000000BD0B0000000000004F00000000000000000000000000000001000000000000000000000000000000
+  - Name:            .hipFatBinSegment
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x202FD0
+    AddressAlign:    0x8
+    Content:         '465049480100000000102000000000000000000000000000'
+...
diff --git a/llvm/tools/llvm-readobj/ObjDumper.cpp b/llvm/tools/llvm-readobj/ObjDumper.cpp
index d3c613ee823ba..326e6a41531aa 100644
--- a/llvm/tools/llvm-readobj/ObjDumper.cpp
+++ b/llvm/tools/llvm-readobj/ObjDumper.cpp
@@ -16,6 +16,8 @@
 #include "llvm/Object/Archive.h"
 #include "llvm/Object/Decompressor.h"
 #include "llvm/Object/ObjectFile.h"
+#include "llvm/Object/OffloadBinary.h"
+#include "llvm/Object/OffloadBundle.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -230,4 +232,21 @@ void ObjDumper::printSectionsAsHex(const object::ObjectFile &Obj,
   }
 }
 
+// TODO: add proper error handling.
+void ObjDumper::printOffloading(const object::ObjectFile &Obj) {
+  // we can use an argument to let user select which offloading section they
+  // want to print. but for now, we're hardcoding ELF and "hip_fatbin".
+  assert((Obj.isELF() || Obj.isCOFF()) && "Invalid file type");
+
+  SmallVector<llvm::object::OffloadBundleFatBin> Bundles;
+  if (Error Err = llvm::object::extractOffloadBundleFatBinary(Obj, Bundles))
+    reportWarning(createError("Cannot extract Fatbin Binary from Object."),
+                  Obj.getFileName());
+
+  // Print out all the FatBin Bundles that are contained in this buffer.
+  for (const auto &[Index, Bundle] : llvm::enumerate(Bundles)) {
+    Bundle.printEntriesAsURI();
+  }
+}
+
 } // namesp...
[truncated]

[jh7370]

Suggested change

	## Test that --offloading with a fatbin works correctly
	# REQUIRES: target={{x86_64-.*-linux.*}}
	# REQUIRES: amdgpu-registered-target
	## Test that --offloading with a fatbin works correctly
	# REQUIRES: target={{x86_64-.*-linux.*}}
	# REQUIRES: amdgpu-registered-target

• Missing full stop at end of comment.
• Is the offloading bundling stuff target-specific? This test looks like it is otherwise target and platform agnostic and doesn't require these REQUIRES statements.

[david-salinas]

It is target specific. Only targets where a hip_fatbin offload section is generated. So AMDGPU targets. And, for now, just linux platforms. There is an issue on Windows where yam2obj doesn't properly generate the object file from this test. And you've noticed in the comment below, that ideally, we need proper support in yaml2obj for these offload bundles. This would be a fair amount of work and for now is just outside the scope of this patch.

[jh7370]

It is target specific. Only targets where a hip_fatbin offload section is generated. So AMDGPU targets.

Maybe I'm misunderstanding something, but I'm not convinced this should restrict under what configurations the test should run. yaml2obj can generate the section data regardless of what targets have been enabled in LLVM. Equally, llvm-readobj can read ELF object data without needing targets to be enabled, since the code lives in the Object library. Thus e.g. an x86 host could easily support this test without needing any particular configuration of LLVM such as the AMDGPU target being enabled. This means a REQUIRES target directive is inappropriate. This would be different if e.g. llvm-mc was being used to generate the input object, since llvm-mc requires targets to be enabled.

There is an issue on Windows where yam2obj doesn't properly generate the object file from this test.

I don't think target is the right thing to be using for this as this should be based on the system host, which may well be Windows targeting Linux. Specifically, you want something like UNSUPPORTED: system-windows. This will also allow the test to run on e.g. Mac systems. I'd even consider making it XFAIL if the intent is that it should work, but something unexpected is causing it to not work. That way, when the issue is fixed, the test will start XPASSing and the annotation can be removed, whereas with UNSUPPORTED, it might get forgotten about.

The annotation needs an accompanying comment too.

Missing full stop at end of comment.

This nit hasn't been addressed.

[jh7370]

This isn't really how yaml2obj is supposed to be used in tests - it's no better than opaque binaries. Alternative ideas:

• Implement yaml2obj support for offloading content in some form, like we have for many other sections. This probably requires the most work, but would allow for actual useful testing that is also human-readable. It also may be the only way to easily exercise error paths within the code.
• Generate the input object at test time using other commands.
• If neither of the above are options, at the very least please include a comment stating what was done to generate the input content.

[david-salinas]

Yup. :-) in writing this I ran into issues getting proper LIT tests, and realized that we really need to extend yaml2obj to generate offload bundles. I even had the test generate the input object (from C++/HIP source) as you suggested at one point. Maybe that's better than this? The yaml2obj on windows chokes on the Content data, which is why I had to add "linux" to the REQUIRES target.

[jh7370]

If you want comprehensive testing of the offloading code, you almost certainly need proper yaml2obj support, because without it, you'll find it hard to generate the edge cases that are often the most important reason for the tests, I find. It does require some time investment, but I find it's worth it, for the benefits in test maintainability that it grants. I'm not going to stand in the way of this PR going in without it, especially if you are planning on making the yaml2obj improvement at some point and will revisit this test at that point.

In the meantime, it's about test readability and maintainability. I'd use the approach that works best for these two, while also meeting other requirements like dependency and layering rules (e.g. no lld or clang usage in llvm tests). If this is the best approach, then so be it for now.

[jh7370]

Sorry, missed some comments with my first pass...

[jh7370]

Please don't force push to update a PR. Just use fixup commits, per LLVM policy (https://llvm.org/docs/GitHub.html#updating-pull-requests and the following sub-section). If you need to rebase, use a merge commit instead (it'll disappear during the squash & merge). Force pushes prevent me from easily seeing what has changed since the previous version I reviewed.

Also, I'd prefer it if you didn't resolve conversations in GitHub that I've started, as I use unresolved conversations to easily indicate to me whether I've reviewed and am happy with the requested change/resolution etc. I made a post on Discourse ages ago about this topic, if you want to read more context.

[jh7370]

It is target specific. Only targets where a hip_fatbin offload section is generated. So AMDGPU targets.

Maybe I'm misunderstanding something, but I'm not convinced this should restrict under what configurations the test should run. yaml2obj can generate the section data regardless of what targets have been enabled in LLVM. Equally, llvm-readobj can read ELF object data without needing targets to be enabled, since the code lives in the Object library. Thus e.g. an x86 host could easily support this test without needing any particular configuration of LLVM such as the AMDGPU target being enabled. This means a REQUIRES target directive is inappropriate. This would be different if e.g. llvm-mc was being used to generate the input object, since llvm-mc requires targets to be enabled.

There is an issue on Windows where yam2obj doesn't properly generate the object file from this test.

I don't think target is the right thing to be using for this as this should be based on the system host, which may well be Windows targeting Linux. Specifically, you want something like UNSUPPORTED: system-windows. This will also allow the test to run on e.g. Mac systems. I'd even consider making it XFAIL if the intent is that it should work, but something unexpected is causing it to not work. That way, when the issue is fixed, the test will start XPASSing and the annotation can be removed, whereas with UNSUPPORTED, it might get forgotten about.

The annotation needs an accompanying comment too.

Missing full stop at end of comment.

This nit hasn't been addressed.

[jh7370]

Nit: don't start a function with a blank line.

[david-salinas]

ok

[jh7370]

You missed the spaces in my previous suggestion. These help show that the FileCheck line is a continuation of the previous line. Also, I forgot to remove --input-file since that's not needed now that you're passing the input from llvm-readobj directly into FileCheck.

Suggested change

	# RUN: FileCheck %s --input-file=%t.out -DFILE_NAME=%t.elf
	# RUN: FileCheck %s -DFILE_NAME=%t.elf

[david-salinas]

ok

[jh7370]

I doubt these ProgramHeaders are actually needed for this test.

[david-salinas]

yup. Not needed.

[jh7370]

We normally omit fields that aren't actually needed, to better highlight the important behaviour. I suspect these two are redundant (I'm less certain about Entry).

[david-salinas]

Seems to work with both removed. :-)

[jh7370]

Similar to the above comments, these fields look redundant.

[david-salinas]

We need to keep the AddressAlign

[jh7370]

Could you elaborate a bit more, please? Is this some requirement of the offloading parsing code?

[jh7370]

If you want comprehensive testing of the offloading code, you almost certainly need proper yaml2obj support, because without it, you'll find it hard to generate the edge cases that are often the most important reason for the tests, I find. It does require some time investment, but I find it's worth it, for the benefits in test maintainability that it grants. I'm not going to stand in the way of this PR going in without it, especially if you are planning on making the yaml2obj improvement at some point and will revisit this test at that point.

In the meantime, it's about test readability and maintainability. I'd use the approach that works best for these two, while also meeting other requirements like dependency and layering rules (e.g. no lld or clang usage in llvm tests). If this is the best approach, then so be it for now.

[david-salinas]

Please don't force push to update a PR. Just use fixup commits, per LLVM policy (https://llvm.org/docs/GitHub.html#updating-pull-requests and the following sub-section). If you need to rebase, use a merge commit instead (it'll disappear during the squash & merge). Force pushes prevent me from easily seeing what has changed since the previous version I reviewed.

Also, I'd prefer it if you didn't resolve conversations in GitHub that I've started, as I use unresolved conversations to easily indicate to me whether I've reviewed and am happy with the requested change/resolution etc. I made a post on Discourse ages ago about this topic, if you want to read more context.

OK. My concern is that if there is a lot of back and forth in a PR review, we could have lots of commits, and then we would end up with a merge commit instead of one clean commit. But I totally understand the issue from the reviewer's perspective. I have some of the issues addressed now, and I'll push a separate commit.

[jh7370]

Please don't force push to update a PR. Just use fixup commits, per LLVM policy (https://llvm.org/docs/GitHub.html#updating-pull-requests and the following sub-section). If you need to rebase, use a merge commit instead (it'll disappear during the squash & merge). Force pushes prevent me from easily seeing what has changed since the previous version I reviewed.
Also, I'd prefer it if you didn't resolve conversations in GitHub that I've started, as I use unresolved conversations to easily indicate to me whether I've reviewed and am happy with the requested change/resolution etc. I made a post on Discourse ages ago about this topic, if you want to read more context.

OK. My concern is that if there is a lot of back and forth in a PR review, we could have lots of commits, and then we would end up with a merge commit instead of one clean commit. But I totally understand the issue from the reviewer's perspective. I have some of the issues addressed now, and I'll push a separate commit.

The normal process for merging a PR is a Squash & Merge, which causes all the fixups and merges to be folded into one commit in main. Was that what you were worried about?

[david-salinas]

Please don't force push to update a PR. Just use fixup commits, per LLVM policy (https://llvm.org/docs/GitHub.html#updating-pull-requests and the following sub-section). If you need to rebase, use a merge commit instead (it'll disappear during the squash & merge). Force pushes prevent me from easily seeing what has changed since the previous version I reviewed.
Also, I'd prefer it if you didn't resolve conversations in GitHub that I've started, as I use unresolved conversations to easily indicate to me whether I've reviewed and am happy with the requested change/resolution etc. I made a post on Discourse ages ago about this topic, if you want to read more context.

OK. My concern is that if there is a lot of back and forth in a PR review, we could have lots of commits, and then we would end up with a merge commit instead of one clean commit. But I totally understand the issue from the reviewer's perspective. I have some of the issues addressed now, and I'll push a separate commit.

The normal process for merging a PR is a Squash & Merge, which causes all the fixups and merges to be folded into one commit in main. Was that what you were worried about?

Actually I was worried the commits wouldn't be squashed and the log would be filled with a bunch of commits. But I'm not sure why I thought we wouldn't just do a Squash. :-) But I think this works better; multiple commits addressing comments, and then when all is good, a Squash Merge. :-)

[david-salinas]

Response to the comment about Testing for Err, or reportWarning above (#143342 (comment)). for some reason github is not letting me "reply" to this comment. So I'm going to reply here.

I totally agree that we should have error condition checking tests. I'm a bit limited in what error conditions I can check, because of the limitation in yaml2obj. Like I can't really test when the metadata in the Bundle header is wrong/missing (e.g. number of Entries doesn't match actual number of entries, or size of an entry isn't a valid value, etc). But I can test for big errors, like the offload bundle section doesn't contain any Magic string, or the API just fails entirely to read the offload section. I'll tests for that in a new patch shortly.

[jh7370]

Response to the comment about Testing for Err, or reportWarning above (#143342 (comment)). for some reason github is not letting me "reply" to this comment. So I'm going to reply here.

I totally agree that we should have error condition checking tests. I'm a bit limited in what error conditions I can check, because of the limitation in yaml2obj. Like I can't really test when the metadata in the Bundle header is wrong/missing (e.g. number of Entries doesn't match actual number of entries, or size of an entry isn't a valid value, etc). But I can test for big errors, like the offload bundle section doesn't contain any Magic string, or the API just fails entirely to read the offload section. I'll tests for that in a new patch shortly.

I suspect the best way to do this would be to enhance yaml2obj further (possibly after adding basic offloading section support of course), to allow modifying fields. There are good examples in the ELF yaml2obj code of fields that can be used for testing these edge cases, for example setting the sh_offset field of a section header to a value, but having the data somewhere else.

[arsenm]

Why? It's probably not true but needs commenting what the issue is if any

[david-salinas]

I'm using the yaml2obj util to generate an object with a clang offload bundle with proper binary file layout (https://clang.llvm.org/docs/ClangOffloadBundler.html#bundled-binary-file-layout). But the yaml2obj util when built on Windows cannot handle "Content" field this large - even though I generated that from a HIP simple add source. Ideally we should be able more easily specify the metadata for the bundled binary in yaml, but that will require some work to the util/tool. I plan on doing this in a future PR. So even though we can generate an ELF Object on windows, the yaml2obj util has a bug on Windows that prevents this test from getting past the first RUN command.

[arsenm]

!windows would be better than only linux

[jh7370]

As stated in an earlier comment, I have tested this on Windows and the yaml2obj code works absolutely fine for me. The test passes without any of the REQUIRES/UNSUPPORTED commands.

[david-salinas]

A previous CI check failed on the yaml2obj command, on windows. I'll remove the limitation and see if it still fails.

[arsenm]

error messages should start with lowercase

[david-salinas]

This is an existing error message, and it is used/checked in other tests.

[jh7370]

Some nits left, but basically there. Sorry for the delay in review.

[jh7370]

Nit: since EC usually stands for "error code" and you're using an Error not std::error_code, I'd change the name to Err or similar.

[jh7370]

Please delete these lines as previously requested. The code under test should be target agnostic (in terms of what targets are enabled in LLVM).

[jh7370]

Same comment.

[jh7370]

Nit: this list is the wrong list to add this option, as it's the ELF-specific ones (according to the comment). It should be the list above (and in alphabetical order).

[jh7370]

Nit: These options are set in alphabetical order.

[jh7370]

Coming back to an earlier comment that seems to have got missed: why is this AddressAlign needed?