[llvm][llvm-objdump] Fix fatbin handling on 32-bit systems

[DavidSpickett]

Which fixes a test failure seen on the bots, introduced by #140286.

[ RUN      ] OffloadingBundleTest.checkExtractOffloadBundleFatBinary
ObjectTests: ../llvm/llvm/include/llvm/ADT/StringRef.h:618: StringRef llvm::StringRef::drop_front(size_t) const: Assertion `size() >= N && "Dropping more elements than exist"' failed.
0 0x0a24a990 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/tcwg-buildbot/worker/clang-armv8-quick/stage1/unittests/Object/./ObjectTests+0x31a990)
1 0x0a248364 llvm::sys::RunSignalHandlers() (/home/tcwg-buildbot/worker/clang-armv8-quick/stage1/unittests/Object/./ObjectTests+0x318364)
2 0x0a24b410 SignalHandler(int, siginfo_t*, void*) Signals.cpp:0:0
3 0xf46ed6f0 __default_rt_sa_restorer ./signal/../sysdeps/unix/sysv/linux/arm/sigrestorer.S:80:0
4 0xf46ddb06 ./csu/../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47:0
5 0xf471d292 __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
6 0xf46ec840 gsignal ./signal/../sysdeps/posix/raise.c:27:6

Also reported on 32-bit x86.

I think the cause is the code was casting the result of StringRef.find into an int64_t. The failure value of find is StringRef::npos, which is defined as:
static constexpr size_t npos = ~size_t(0);

• size_t(0) is 32 bits of 0s
• the inverse of that is 32 bits of 1s
• Cast to int64_t needs to widen this, and it will preserve the original value in doing so, which is 0xffffffff.
• The result is 0x00000000ffffffff, which is >= 0, so we keep searching and try to go off the end of the file.

Or put another way, this equivalent function returns true when compiled for a 32-bit system:

bool fn() {
    size_t foo = ~size_t(0);
    int64_t foo64 = (int64_t)foo;
    return foo64 >= 0;
}

Using size_t throughout fixes the problem. Also I don't see a reason it needs to be a signed number, given that it always searches forward from the current offset.

[llvmbot]

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

Author: David Spickett (DavidSpickett)

Changes

Which fixes a test failure seen on the bots, introduced by #140286.

[ RUN      ] OffloadingBundleTest.checkExtractOffloadBundleFatBinary
ObjectTests: ../llvm/llvm/include/llvm/ADT/StringRef.h:618: StringRef llvm::StringRef::drop_front(size_t) const: Assertion `size() >= N && "Dropping more elements than exist"' failed.
0 0x0a24a990 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/tcwg-buildbot/worker/clang-armv8-quick/stage1/unittests/Object/./ObjectTests+0x31a990)
1 0x0a248364 llvm::sys::RunSignalHandlers() (/home/tcwg-buildbot/worker/clang-armv8-quick/stage1/unittests/Object/./ObjectTests+0x318364)
2 0x0a24b410 SignalHandler(int, siginfo_t*, void*) Signals.cpp:0:0
3 0xf46ed6f0 __default_rt_sa_restorer ./signal/../sysdeps/unix/sysv/linux/arm/sigrestorer.S:80:0
4 0xf46ddb06 ./csu/../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47:0
5 0xf471d292 __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
6 0xf46ec840 gsignal ./signal/../sysdeps/posix/raise.c:27:6

Also reported on 32-bit x86.

I think the cause is the code was casting the result of StringRef.find into an int64_t. The failure value of find is StringRef::npos, which is defined as:
static constexpr size_t npos = ~size_t(0);

I think what happens is size_t(0) is 32 bits of 0s, inverted to 32 bits of 1s. This is then cast to int64_t, which you might think would sign extend from the most significant bit, but if it did not and just added 0s, then the result would always be > 0. So I could be wrong about the exact cause here, but switching to size_t throughout fixes the issue.

Also I don't see a reason it needs to be a signed number, given that it always searches forward from the current offset.

---

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

1 Files Affected:

• (modified) llvm/lib/Object/OffloadBundle.cpp (+5-7)

diff --git a/llvm/lib/Object/OffloadBundle.cpp b/llvm/lib/Object/OffloadBundle.cpp
index 5f087a5c84e8d..239a3e2616ba5 100644
--- a/llvm/lib/Object/OffloadBundle.cpp
+++ b/llvm/lib/Object/OffloadBundle.cpp
@@ -38,12 +38,11 @@ Error extractOffloadBundle(MemoryBufferRef Contents, uint64_t SectionOffset,
                            StringRef FileName,
                            SmallVectorImpl<OffloadBundleFatBin> &Bundles) {
 
-  uint64_t Offset = 0;
-  int64_t NextbundleStart = 0;
+  size_t Offset = 0;
+  size_t NextbundleStart = 0;
 
   // There could be multiple offloading bundles stored at this section.
-  while (NextbundleStart >= 0) {
-
+  while (NextbundleStart != StringRef::npos) {
     std::unique_ptr<MemoryBuffer> Buffer =
         MemoryBuffer::getMemBuffer(Contents.getBuffer().drop_front(Offset), "",
                                    /*RequiresNullTerminator=*/false);
@@ -60,10 +59,9 @@ Error extractOffloadBundle(MemoryBufferRef Contents, uint64_t SectionOffset,
 
     // Find the next bundle by searching for the magic string
     StringRef Str = Buffer->getBuffer();
-    NextbundleStart =
-        (int64_t)Str.find(StringRef("__CLANG_OFFLOAD_BUNDLE__"), 24);
+    NextbundleStart = Str.find(StringRef("__CLANG_OFFLOAD_BUNDLE__"), 24);
 
-    if (NextbundleStart >= 0)
+    if (NextbundleStart != StringRef::npos)
       Offset += NextbundleStart;
   }
 

[DavidSpickett]

Still running the rest of the tests locally to confirm.

Also wondering if this was using 64 bit types because you expect files greater than 4GB. In which case we can keep it as it was but add a check for StringRef::npos before we do any conversions.

[mgorny]

I've started a test run too, it should finish in 20 minutes or so.

[DavidSpickett]

With this PR I had no test failures on an Armv8 32-bit machine.

[mgorny]

Thanks! I can confirm that it fixed the problem for me.

[david-salinas]

LGTM