[llvm] Fix ObjectSizeOffsetVisitor behavior in exact mode upon negati…

[serge-sans-paille]

…ve offset

In Exact mode, the approximation of returning (0,0) is invalid. It only holds in min/max mode.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: None (serge-sans-paille)

Changes

…ve offset

In Exact mode, the approximation of returning (0,0) is invalid. It only holds in min/max mode.

---

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

2 Files Affected:

• (modified) llvm/lib/Analysis/MemoryBuiltins.cpp (+7-3)
• (added) llvm/test/Instrumentation/BoundsChecking/negative.ll (+45)

diff --git a/llvm/lib/Analysis/MemoryBuiltins.cpp b/llvm/lib/Analysis/MemoryBuiltins.cpp
index 6c0940c4c81ebe..79237d1edeb340 100644
--- a/llvm/lib/Analysis/MemoryBuiltins.cpp
+++ b/llvm/lib/Analysis/MemoryBuiltins.cpp
@@ -844,10 +844,14 @@ OffsetSpan ObjectSizeOffsetVisitor::computeImpl(Value *V) {
   }
 
   // We end up pointing on a location that's outside of the original object.
-  // This is UB, and we'd rather return an empty location then.
   if (ORT.knownBefore() && ORT.Before.isNegative()) {
-    ORT.Before = APInt::getZero(ORT.Before.getBitWidth());
-    ORT.After = APInt::getZero(ORT.Before.getBitWidth());
+    // This is UB, and we'd rather return an empty location then.
+    if (Options.EvalMode == ObjectSizeOpts::Mode::Min ||
+        Options.EvalMode == ObjectSizeOpts::Mode::Max) {
+      ORT.Before = APInt::getZero(ORT.Before.getBitWidth());
+      ORT.After = APInt::getZero(ORT.Before.getBitWidth());
+    }
+    // Otherwise it's fine, caller can handle negative offset.
   }
   return ORT;
 }
diff --git a/llvm/test/Instrumentation/BoundsChecking/negative.ll b/llvm/test/Instrumentation/BoundsChecking/negative.ll
new file mode 100644
index 00000000000000..d8fb117bd13af8
--- /dev/null
+++ b/llvm/test/Instrumentation/BoundsChecking/negative.ll
@@ -0,0 +1,45 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; Check that negative oob gep do not generate invalid check.
+; RUN: opt < %s -passes=bounds-checking -S | FileCheck %s
+target datalayout = "e-p:64:64:64-p1:16:16:16-p2:64:64:64:48-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+
+
+@str = global [100 x i8] zeroinitializer, align 1
+
+define i16 @main() {
+; CHECK-LABEL: @main(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_COND:%.*]]
+; CHECK:       for.cond:
+; CHECK-NEXT:    [[I_0:%.*]] = phi i8 [ 65, [[ENTRY:%.*]] ], [ [[INC:%.*]], [[TMP4:%.*]] ]
+; CHECK-NEXT:    [[EXITCOND_NOT:%.*]] = icmp eq i8 [[I_0]], 76
+; CHECK-NEXT:    br i1 [[EXITCOND_NOT]], label [[FOR_END:%.*]], label [[TMP4]]
+; CHECK:       for.inc:
+; CHECK-NEXT:    [[I_0_C:%.*]] = sext i8 [[I_0]] to i64
+; CHECK-NEXT:    [[TMP0:%.*]] = add i64 -65, [[I_0_C]]
+; CHECK-NEXT:    [[GEP:%.*]] = getelementptr i8, ptr getelementptr (i8, ptr @str, i8 -65), i8 [[I_0]]
+; CHECK-NEXT:    [[TMP1:%.*]] = sub i64 100, [[TMP0]]
+; CHECK-NEXT:    store i8 [[I_0]], ptr [[GEP]], align 1
+; CHECK-NEXT:    [[INC]] = add nuw nsw i8 [[I_0]], 1
+; CHECK-NEXT:    br label [[FOR_COND]]
+; CHECK:       for.end:
+; CHECK-NEXT:    ret i16 0
+;
+entry:
+  br label %for.cond
+
+for.cond:
+  %i.0 = phi i8 [ 65, %entry ], [ %inc, %for.inc ]
+  %exitcond.not = icmp eq i8 %i.0, 76
+  br i1 %exitcond.not, label %for.end, label %for.inc
+
+for.inc:                                          ; preds = %for.cond
+  %gep = getelementptr i8, ptr getelementptr (i8, ptr @str, i8 -65), i8 %i.0
+  store i8 %i.0, ptr %gep, align 1
+  %inc = add nuw nsw i8 %i.0, 1
+  br label %for.cond
+
+for.end:
+  ret i16 0
+}
+

[nathanchance]

Thanks, this resolves this issue I reported.

[nathanchance]

Hmmm, applying this on top of current main (3291372) and doing my full set of builds, I actually see a new crash:

llvm/lib/Analysis/MemoryBuiltins.cpp:569: APInt getSizeWithOverflow(const SizeOffsetAPInt &): Assertion `!Offset.isNegative() && "size for a pointer before the allocated object is ambiguous"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: clang-20 … io_uring/kbuf.c
1.      <eof> parser at end of file
2.      Code generation
3.      Running pass 'Function Pass Manager' on module '/home/nathan/cbl/src/clean/linux/io_uring/kbuf.c'.
4.      Running pass 'Split GEPs to a variadic base and a constant offset for better CSE' on function '@io_provide_buffers'
…

It’s quite late for me, so I will try to get a reduced reproducer as soon as I am able to tomorrow.

[nathanchance]

struct {
  int list;
} *io_add_buffers_bufs[4];
short io_add_buffers_buf_1, io_add_buffers_pbuf_1;
long io_add_buffers_pbuf_0;
void list_add_tail(int *);
int kmem_cache_alloc_bulk_noprof();
int io_add_buffers() {
  long addr = io_add_buffers_pbuf_0;
  int bid = io_add_buffers_pbuf_1,
      allocated = ({
        ({
          typeof(0) _res = kmem_cache_alloc_bulk_noprof();
          _res;
        });
      });
  while (allocated)
    list_add_tail(&io_add_buffers_bufs[--allocated]->list);
  addr = addr;
  io_add_buffers_buf_1 = bid;
  return 0;
}

$ clang --target=powerpc-linux-gnu -O2 -c -o /dev/null kbuf.i
clang: llvm/lib/Analysis/MemoryBuiltins.cpp:569: APInt getSizeWithOverflow(const SizeOffsetAPInt &): Assertion `!Offset.isNegative() && "size for a pointer before the allocated object is ambiguous"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: clang --target=powerpc-linux-gnu -O2 -c -o /dev/null kbuf.i
1.      <eof> parser at end of file
2.      Code generation
3.      Running pass 'Function Pass Manager' on module 'kbuf.i'.
4.      Running pass 'Split GEPs to a variadic base and a constant offset for better CSE' on function '@io_add_buffers'
…

[serge-sans-paille]

Got it! The assert no longer hold with the change, I've updated the patch, can you confirm it's all fine on your side?

[nathanchance]

The latest revision passed my initial smoke test build. My full matrix of build and boots is running now, I’ll report back if there are any problems but assume silence means all is well.

EDIT: Everything appears to be good, thanks again for the fix.