[Offloading] Extend OffloadBinary format to support multiple metadata entries #169425

YuriPlyakhin · 2025-11-24T23:42:03Z

No description provided.

… entries

github-actions · 2025-11-24T23:43:45Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions cpp,c,h -- clang/test/Driver/linker-wrapper-image.c llvm/include/llvm/Object/OffloadBinary.h llvm/include/llvm/ObjectYAML/OffloadYAML.h llvm/lib/Frontend/Offloading/OffloadWrapper.cpp llvm/lib/Object/OffloadBinary.cpp llvm/lib/ObjectYAML/OffloadEmitter.cpp llvm/lib/ObjectYAML/OffloadYAML.cpp llvm/tools/llvm-objdump/OffloadDump.cpp llvm/tools/llvm-offload-binary/llvm-offload-binary.cpp llvm/tools/obj2yaml/offload2yaml.cpp --diff_from_common_commit

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.

diff --git a/llvm/include/llvm/Object/OffloadBinary.h b/llvm/include/llvm/Object/OffloadBinary.h
index 84ebf51db..a63ea77d1 100644
--- a/llvm/include/llvm/Object/OffloadBinary.h
+++ b/llvm/include/llvm/Object/OffloadBinary.h
@@ -95,7 +95,8 @@ public:
   };
 
   using StringMap = MapVector<StringRef, StringRef>;
-  using entry_iterator = SmallVector<std::pair<const Entry *, StringMap>, 1>::const_iterator;
+  using entry_iterator =
+      SmallVector<std::pair<const Entry *, StringMap>, 1>::const_iterator;
   using entry_iterator_range = iterator_range<entry_iterator>;
   using string_iterator = MapVector<StringRef, StringRef>::const_iterator;
   using string_iterator_range = iterator_range<string_iterator>;
@@ -128,7 +129,9 @@ public:
     return Entries[0].first->TheImageKind;
   }
 
-  OffloadKind getOffloadKind() const { return Entries[0].first->TheOffloadKind; }
+  OffloadKind getOffloadKind() const {
+    return Entries[0].first->TheOffloadKind;
+  }
   uint32_t getVersion() const { return TheHeader->Version; }
   uint32_t getFlags() const { return Entries[0].first->Flags; }
   uint64_t getSize() const { return TheHeader->Size; }
@@ -137,7 +140,8 @@ public:
   StringRef getTriple() const { return getString("triple"); }
   StringRef getArch() const { return getString("arch"); }
   StringRef getImage() const {
-    return StringRef(&Buffer[Entries[0].first->ImageOffset], Entries[0].first->ImageSize);
+    return StringRef(&Buffer[Entries[0].first->ImageOffset],
+                     Entries[0].first->ImageSize);
   }
 
   // Iterator access to all entries in the binary
@@ -156,7 +160,9 @@ public:
   // Iterator over all the key and value pairs in the binary.
   string_iterator_range strings() const { return Entries[0].second; }
 
-  StringRef getString(StringRef Key) const { return Entries[0].second.lookup(Key); }
+  StringRef getString(StringRef Key) const {
+    return Entries[0].second.lookup(Key);
+  }
 
   static bool classof(const Binary *V) { return V->isOffloadFile(); }
 
@@ -172,8 +178,8 @@ private:
       StringMap Strings;
       for (uint64_t SI = 0, SE = TheEntry->NumStrings; SI != SE; ++SI) {
         StringRef Key = &Buffer[StringMapBegin[SI].KeyOffset];
-        StringRef Value = StringRef(
-            &Buffer[StringMapBegin[SI].ValueOffset], StringMapBegin[SI].ValueSize);
+        StringRef Value = StringRef(&Buffer[StringMapBegin[SI].ValueOffset],
+                                    StringMapBegin[SI].ValueSize);
         Strings.insert({Key, Value});
       }
       Entries.push_back(std::make_pair(TheEntry, std::move(Strings)));

github-actions · 2025-11-24T23:51:44Z

🐧 Linux x64 Test Results

96753 tests passed
1769 tests skipped

All tests passed but another part of the build failed. Click on a failure below to see the details.

unittests/Object/CMakeFiles/ObjectTests.dir/OffloadingTest.cpp.o

FAILED: unittests/Object/CMakeFiles/ObjectTests.dir/OffloadingTest.cpp.o
sccache /opt/llvm/bin/clang++ -DGTEST_HAS_RTTI=0 -DLLVM_BUILD_STATIC -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GLIBCXX_USE_CXX11_ABI=1 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/gha/actions-runner/_work/llvm-project/llvm-project/build/unittests/Object -I/home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/unittests/Object -I/home/gha/actions-runner/_work/llvm-project/llvm-project/build/include -I/home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/include -I/home/gha/actions-runner/_work/llvm-project/llvm-project/third-party/unittest/googletest/include -I/home/gha/actions-runner/_work/llvm-project/llvm-project/third-party/unittest/googlemock/include -gmlt -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wno-pass-failed -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -std=c++17  -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -Wno-suggest-override -MD -MT unittests/Object/CMakeFiles/ObjectTests.dir/OffloadingTest.cpp.o -MF unittests/Object/CMakeFiles/ObjectTests.dir/OffloadingTest.cpp.o.d -o unittests/Object/CMakeFiles/ObjectTests.dir/OffloadingTest.cpp.o -c /home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/unittests/Object/OffloadingTest.cpp
/home/gha/actions-runner/_work/llvm-project/llvm-project/llvm/unittests/Object/OffloadingTest.cpp:54:39: error: no member named 'getImageKind' in 'llvm::object::OffloadBinary'
54 |   ASSERT_EQ(Data.TheImageKind, Binary.getImageKind());
|                                ~~~~~~ ^
1 error generated.

If these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the infrastructure label.

jhuber6

I agree with this in principle, but

llvm/include/llvm/Object/OffloadBinary.h

jhuber6

I'm okay with these changes, now we just need the few changes to make sure that everything still works with the new version. Check all the uses of these fields and we probably want an obj2yaml support for multiple entries so we can test it. Realistically you just need to update the extractOffloadFiles to additionally extract files within the same header.

jhuber6 · 2025-11-25T20:09:11Z

llvm/include/llvm/Object/OffloadBinary.h

-    uint64_t EntrySize;   // Size of the metadata entry in bytes.
+    uint64_t Size;          // Size in bytes of this entire binary.
+    uint64_t EntriesOffset; // Offset in bytes to the start of entries block.
+    uint64_t EntriesCount;  // Number of metadata entries in the binary.


I'm okay with changing this, it wasn't really used for anything but a sanity check during parsing

YuriPlyakhin · 2025-11-25T20:14:30Z

I'm okay with these changes, now we just need the few changes ...

Thank you for quick feedback, Joseph, I'll continue updating this PR with the further changes.

…ry format.

YuriPlyakhin · 2025-11-26T02:57:13Z

FYI: I'm now updating OffloadBinary.cpp (OffloadBinary::create, OffloadBinary::write, etc..) to support new format.

jhuber6 · 2025-11-26T02:58:35Z

FYI: I'm now updating OffloadBinary.cpp (OffloadBinary::create, OffloadBinary::write, etc..) to support new format.

Yeah, we'll probably just want to make the interface take an array reference, possibly a helper that takes a single one and turns it into an array of one for convenience

jhuber6

I'm wondering if we should keep the accessor methods to all take a single image, so the extract code just returns an array of these. You'd need to make sure the reference stays alive but it might be simpler that way, I don't think the interface handles this well, like for checking the image kind.

jhuber6 · 2025-11-27T02:38:18Z

llvm/include/llvm/Object/OffloadBinary.h

+  OffloadKind getOffloadKind() const { return Entries[0].first->TheOffloadKind; }
  uint32_t getVersion() const { return TheHeader->Version; }
-  uint32_t getFlags() const { return TheEntry->Flags; }
+  uint32_t getFlags() const { return Entries[0].first->Flags; }


I don't think this has any users, we can and should change the interface

jhuber6 · 2025-11-27T02:38:48Z

llvm/include/llvm/Object/OffloadBinary.h

  StringRef getArch() const { return getString("arch"); }
  StringRef getImage() const {
-    return StringRef(&Buffer[TheEntry->ImageOffset], TheEntry->ImageSize);
+    return StringRef(&Buffer[Entries[0].first->ImageOffset], Entries[0].first->ImageSize);


Probably want an index, can initialize it to zero for backward compat.

jhuber6 · 2025-11-27T02:40:44Z

llvm/include/llvm/Object/OffloadBinary.h

-  MapVector<StringRef, StringRef> StringData;
+  /// Location of the metadata entries within the binary mapped to
+  /// the key-value string data.
+  SmallVector<std::pair<const Entry *, StringMap>, 1> Entries;


Can't decide if this should be two vectors that take the same index

jhuber6 · 2025-11-27T02:41:50Z

llvm/include/llvm/Object/OffloadBinary.h

+      const StringEntry *StringMapBegin = reinterpret_cast<const StringEntry *>(
+          &Buffer[TheEntry->StringOffset]);
+      StringMap Strings;
+      for (uint64_t SI = 0, SE = TheEntry->NumStrings; SI != SE; ++SI) {


Can we just take a reference to the StringData and keep the old handling?

[Offloading] Extend OffloadBinary format to support multiple metadata…

ce7ab76

… entries

jhuber6 self-requested a review November 25, 2025 00:06

jhuber6 reviewed Nov 25, 2025

View reviewed changes

llvm/include/llvm/Object/OffloadBinary.h Outdated Show resolved Hide resolved

llvm/include/llvm/Object/OffloadBinary.h Outdated Show resolved Hide resolved

updated offloadbinary per discussion

f66ae8c

jhuber6 reviewed Nov 25, 2025

View reviewed changes

Minimal changes to make build and lit tests pass with new OffloadBina…

98ad7e9

…ry format.

YuriPlyakhin added 2 commits November 27, 2025 02:33

Just a snapshot of work in progress.

a28aa79

one more minor change to make build pass for now

df65ae9

jhuber6 reviewed Nov 27, 2025

View reviewed changes

[Offloading] Extend OffloadBinary format to support multiple metadata entries #169425

Are you sure you want to change the base?

[Offloading] Extend OffloadBinary format to support multiple metadata entries #169425

Conversation

YuriPlyakhin commented Nov 24, 2025

Uh oh!

github-actions bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🐧 Linux x64 Test Results

Uh oh!

jhuber6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jhuber6 left a comment

Choose a reason for hiding this comment

Uh oh!

jhuber6 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

YuriPlyakhin commented Nov 25, 2025

Uh oh!

YuriPlyakhin commented Nov 26, 2025

Uh oh!

jhuber6 commented Nov 26, 2025

Uh oh!

jhuber6 left a comment

Choose a reason for hiding this comment

Uh oh!

jhuber6 Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

jhuber6 Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

jhuber6 Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

jhuber6 Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Nov 24, 2025 •

edited

Loading

github-actions bot commented Nov 24, 2025 •

edited

Loading