[llvm-readobj][offload] Fix llvm-readobj --all on MachO

[nikic]

Currently running llvm-readobj --all on any MachO object asserts, because it implies --offload, and the --offload extraction includes an assert on the object format. Instead, we should be silently ignoring.

This regressed in #143342.

[llvmbot]

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

Author: Nikita Popov (nikic)

Changes

Currently running llvm-readobj --all on any MachO object asserts, because it implies --offload, and the --offload extraction includes an assert on the object format. Instead, we should be silently ignoring.

This regressed in #143342.

---

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

2 Files Affected:

• (modified) llvm/lib/Object/OffloadBundle.cpp (+3-1)
• (added) llvm/test/tools/llvm-readobj/MachO/all.test (+34)

diff --git a/llvm/lib/Object/OffloadBundle.cpp b/llvm/lib/Object/OffloadBundle.cpp
index 046cde8640b49..f006298c1f4b9 100644
--- a/llvm/lib/Object/OffloadBundle.cpp
+++ b/llvm/lib/Object/OffloadBundle.cpp
@@ -188,7 +188,9 @@ Error OffloadBundleFatBin::extractBundle(const ObjectFile &Source) {
 
 Error object::extractOffloadBundleFatBinary(
     const ObjectFile &Obj, SmallVectorImpl<OffloadBundleFatBin> &Bundles) {
-  assert((Obj.isELF() || Obj.isCOFF()) && "Invalid file type");
+  // Ignore unsupported object formats.
+  if (!Obj.isELF() && !Obj.isCOFF())
+    return Error::success();
 
   // Iterate through Sections until we find an offload_bundle section.
   for (SectionRef Sec : Obj.sections()) {
diff --git a/llvm/test/tools/llvm-readobj/MachO/all.test b/llvm/test/tools/llvm-readobj/MachO/all.test
new file mode 100644
index 0000000000000..8916d72614cc5
--- /dev/null
+++ b/llvm/test/tools/llvm-readobj/MachO/all.test
@@ -0,0 +1,34 @@
+# RUN: yaml2obj %s | llvm-readobj --all - | FileCheck %s
+
+# Make sure that --all at least doesn't crash.
+
+# CHECK: Format: Mach-O arm
+# CHECK: Arch: arm
+# CHECK: AddressSize: 32bit
+# CHECK: MachHeader {
+# CHECK:   Magic: Magic (0xFEEDFACE)
+# CHECK:   CpuType: Arm (0xC)
+# CHECK:   CpuSubType: CPU_SUBTYPE_ARM_V7 (0x9)
+# CHECK:   FileType: Relocatable (0x1)
+# CHECK:   NumOfLoadCommands: 0
+# CHECK:   SizeOfLoadCommands: 0
+# CHECK:   Flags [ (0x0)
+# CHECK:   ]
+# CHECK: }
+# CHECK: Sections [
+# CHECK: ]
+# CHECK: Relocations [
+# CHECK: ]
+# CHECK: UnwindInfo not implemented.
+# CHECK: Symbols [
+# CHECK: ]
+
+--- !mach-o
+FileHeader:
+  magic:      0xFEEDFACE
+  cputype:    0xC
+  cpusubtype: 0x9
+  filetype:   0x1
+  ncmds:      0
+  sizeofcmds: 0
+  flags:      0x0

[jh7370]

A part of me is wondering if this is the right place for the fix, but I'd actually prefer @david-salinas to make that decision. I could see a case that the --all option should only enable Offloading for the file formats that support it (either explicitly or via the ObjDumper class hierarchy in llvm-readobj's code).

Not directly related to this PR, but one thing is for certain: the option shouldn't be set by --all for GNU style output (i.e. llvm-readelf), because GNU readelf doesn't have the option. This is in keeping with things like Addrsig.

[jh7370]

Nit: newer llvm-readobj tests use ## for comments to help them stand out from lit/FileCheck directives.

[nikic]

A part of me is wondering if this is the right place for the fix, but I'd actually prefer @david-salinas to make that decision. I could see a case that the --all option should only enable Offloading for the file formats that support it (either explicitly or via the ObjDumper class hierarchy in llvm-readobj's code).

Even if it's not part of --all, surely --offloading shouldn't produce a crash either, even if the object format doesn't support offloading?

[jh7370]

True, in which case it should be either the fix you've got here or the class hierarchy rework (where offloading is only implemented for the supported platforms). As I don't maintain the offloading API, I think it's worth getting someone who does to make the decision.

[jhuber6]

I'm torn on whether or not --all should imply offloading, as it's not truly part of the object format. At a minimum we should just ignore this if it's MachO. Theoretically you could get this to work there, we'd just need someone to decide on a section name and correctly emit the linker magic that lets us iterate the linked & merged section. But there's really not any motivation to do so as I don't think there's a single person doing or caring about GPU compute on Apple systems.

[jhuber6]

I'm fine with this fix, but if people's would rather omit the offloading flag from this mode I'd be fine there as well.

[jh7370]

I'm fine with this fix, but if people's would rather omit the offloading flag from this mode I'd be fine there as well.

I think removing it from the --all mode for GNU output is the right thing to do (could you or @david-salinas or someone else with offloading ownership make that change, please?), but as @nikic has already highlighted, that's not enough to fix the issue, so if you're happy with this change, I am too. LGTM.

[nikic]

/cherry-pick 7abe5b7

[llvmbot]

/pull-request #176133