[MemoryBuiltins] Fix crash on large global variables

[steffenlarsen]

When analyzing global variables with very large sizes that exceed the addressable memory space for a given address space, the previous implementation would attempt to create an APInt with an unrepresentable value leading to crashes. This change introduces a conservative analysis failure condition checking if the size exceeds the maximum representable value for the given integer bit width, preventing these crashes.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Steffen Larsen (steffenlarsen)

Changes

When analyzing global variables with very large sizes that exceed the addressable memory space for a given address space, the previous implementation would attempt to create an APInt with an unrepresentable value leading to crashes. This change introduces a conservative analysis failure condition checking if the size exceeds the maximum representable value for the given integer bit width, preventing these crashes.

---

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

2 Files Affected:

• (modified) llvm/lib/Analysis/MemoryBuiltins.cpp (+2-1)
• (added) llvm/test/Transforms/DeadStoreElimination/large-global-var.ll (+21)

diff --git a/llvm/lib/Analysis/MemoryBuiltins.cpp b/llvm/lib/Analysis/MemoryBuiltins.cpp
index a329f9ae24c13..4410b3f0a38e9 100644
--- a/llvm/lib/Analysis/MemoryBuiltins.cpp
+++ b/llvm/lib/Analysis/MemoryBuiltins.cpp
@@ -1045,7 +1045,8 @@ OffsetSpan ObjectSizeOffsetVisitor::visitGlobalAlias(GlobalAlias &GA) {
 OffsetSpan ObjectSizeOffsetVisitor::visitGlobalVariable(GlobalVariable &GV) {
   if (!GV.getValueType()->isSized() || GV.hasExternalWeakLinkage() ||
       ((!GV.hasInitializer() || GV.isInterposable()) &&
-       Options.EvalMode != ObjectSizeOpts::Mode::Min))
+       Options.EvalMode != ObjectSizeOpts::Mode::Min) ||
+      !isUIntN(IntTyBits, GV.getGlobalSize(DL)))
     return ObjectSizeOffsetVisitor::unknown();
 
   APInt Size(IntTyBits, GV.getGlobalSize(DL));
diff --git a/llvm/test/Transforms/DeadStoreElimination/large-global-var.ll b/llvm/test/Transforms/DeadStoreElimination/large-global-var.ll
new file mode 100644
index 0000000000000..75f6c2274fa27
--- /dev/null
+++ b/llvm/test/Transforms/DeadStoreElimination/large-global-var.ll
@@ -0,0 +1,21 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt < %s -passes=dse -S | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i8:8-i32:32-i64:64"
+
+; The data-layout only allows 32-bit indexing, but the array is too large to be
+; fully accessible through that index-space.
+@G = internal global [39969271929 x i32] undef, align 4
+
+define void @func() {
+; CHECK-LABEL: define void @func() {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    store i32 0, ptr getelementptr (i8, ptr @G, i64 24), align 4
+; CHECK-NEXT:    store i32 0, ptr getelementptr (i8, ptr @G, i64 20), align 4
+; CHECK-NEXT:    ret void
+;
+entry:
+  store i32 0, ptr getelementptr (i8, ptr @G, i64 24), align 4
+  store i32 0, ptr getelementptr (i8, ptr @G, i64 20), align 4
+  ret void
+}

[github-actions]

✅ With the latest revision this PR passed the undef deprecator.

[nikic]

I think we should reject this in the IR verifier instead.

[steffenlarsen]

I think we should reject this in the IR verifier instead.

I'm not opposed to that idea. Unless there are objections, I'll investigate that option.

[boomanaiden154]

I agree that rejecting this in the verifier is a better approach. Otherwise we could just end up playing whack-a-mole with other bugs for something that isn't really supported.

[steffenlarsen]

@nikic & @boomanaiden154 - I locally made a small change in the verifier to diagnose global variables too large for addressing in the specified address space. However, it turns out there are tests relying on allowing this case.

  LLVM :: CodeGen/LoongArch/merge-base-offset-tlsle.ll
  LLVM :: CodeGen/LoongArch/merge-base-offset.ll
  LLVM :: Transforms/GlobalOpt/large-element-size.ll

On the side, I've opened a patch to avoid generating invalid sizes for AMDGPU in #178909.

[nikic]

@steffenlarsen It should be fine to adjust the tests, they're likely just coverage for previous instances of this problem.

[steffenlarsen]

Let's give it a try! I've opened #179625 as a replacement to this patch.