[Verifier] Make verifier fail when global variable size exceeds address space size - #179625
Conversation
…ss space size When a global variable has a size that exceeds the size of the address space it resides in, the verifier should fail as the variable can neither be materialized nor fully accessed. This patch adds a check to the verifier to enforce it. Signed-off-by: Steffen Holst Larsen <HolstLarsen.Steffen@amd.com>
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-backend-loongarch Author: Steffen Larsen (steffenlarsen) ChangesWhen a global variable has a size that exceeds the size of the address space it resides in, the verifier should fail as the variable can neither be materialized nor fully accessed. This patch adds a check to the verifier to enforce it. Full diff: https://github.com/llvm/llvm-project/pull/179625.diff 5 Files Affected:
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 01d32d4a1825f..8d2d36c813207 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -955,6 +955,16 @@ void Verifier::visitGlobalVariable(const GlobalVariable &GV) {
"Global @" + GV.getName() + " has illegal target extension type",
GVType);
+ // Check that the the address space can hold all bits of the type, recognized
+ // by an access in the address space being able to reach all bytes of the
+ // type.
+ Check(GVType->isScalableTy() || !GVType->isSized() ||
+ GV.getGlobalSize(DL) == 0 ||
+ isUIntN(DL.getAddressSizeInBits(GV.getAddressSpace()),
+ GV.getGlobalSize(DL) - 1),
+ "Global variable is too large to fit into the address space", &GV,
+ GVType);
+
if (!GV.hasInitializer()) {
visitGlobalValue(GV);
return;
diff --git a/llvm/test/CodeGen/LoongArch/merge-base-offset-tlsle.ll b/llvm/test/CodeGen/LoongArch/merge-base-offset-tlsle.ll
index 97d33379913e5..6605d5961aa6a 100644
--- a/llvm/test/CodeGen/LoongArch/merge-base-offset-tlsle.ll
+++ b/llvm/test/CodeGen/LoongArch/merge-base-offset-tlsle.ll
@@ -661,7 +661,7 @@ if.end:
ret void
}
-@g_a64 = dso_local thread_local(localexec) global [614750729487779976 x i64] zeroinitializer, align 8
+@g_a64 = dso_local thread_local(localexec) global [536870912 x i64] zeroinitializer, align 8
define dso_local ptr @tlsle_load_addr_offset_1() nounwind {
; LA32-LABEL: tlsle_load_addr_offset_1:
diff --git a/llvm/test/CodeGen/LoongArch/merge-base-offset.ll b/llvm/test/CodeGen/LoongArch/merge-base-offset.ll
index ea0fe3d0b0178..4cda6a708a80c 100644
--- a/llvm/test/CodeGen/LoongArch/merge-base-offset.ll
+++ b/llvm/test/CodeGen/LoongArch/merge-base-offset.ll
@@ -959,7 +959,7 @@ label:
ret ptr %0
}
-@g_a64 = dso_local global [614750729487779976 x i64] zeroinitializer, align 8
+@g_a64 = dso_local global [536870912 x i64] zeroinitializer, align 8
define dso_local ptr @load_addr_offset_1() nounwind {
; LA32-LABEL: load_addr_offset_1:
diff --git a/llvm/test/Transforms/GlobalOpt/large-element-size.ll b/llvm/test/Transforms/GlobalOpt/large-element-size.ll
index 914112af313a5..14a3cd035d26f 100644
--- a/llvm/test/Transforms/GlobalOpt/large-element-size.ll
+++ b/llvm/test/Transforms/GlobalOpt/large-element-size.ll
@@ -6,7 +6,7 @@ target datalayout = "p:32:32"
%struct.t.1 = type { %struct.u.0, %struct.u.0, %struct.u.0, %struct.u.0, i32, i32, i32, i32 }
%struct.u.0 = type { i32, i32, i32, i8 }
-@s = external global [700 x [24000 x %struct.s.2]], align 1
+@s = external global [700 x [2400 x %struct.s.2]], align 1
@p = global ptr getelementptr (i8, ptr @s, i64 2247483647), align 1
; CHECK: @p = local_unnamed_addr global ptr getelementptr (i8, ptr @s, i32 -2047483649), align 1
diff --git a/llvm/test/Verifier/global-var-too-big.ll b/llvm/test/Verifier/global-var-too-big.ll
new file mode 100644
index 0000000000000..478b093eb0bae
--- /dev/null
+++ b/llvm/test/Verifier/global-var-too-big.ll
@@ -0,0 +1,21 @@
+; RUN: not opt -S -passes=verify < %s 2>&1 | FileCheck %s
+
+target datalayout = "e-m:e-p1:16:16-p2:32:32-p3:64:64-i8:8-i32:32-i64:64"
+
+; Too large for 16-bit address space.
+@G1 = internal addrspace(1) global [65537 x i8] zeroinitializer, align 4
+
+; Too large for 32-bit address space.
+@G2 = internal addrspace(2) global [2147483649 x i16] zeroinitializer, align 4
+
+; Fit within the address spaces
+@G3 = internal addrspace(1) global [65536 x i8] zeroinitializer, align 4
+@G4 = internal addrspace(2) global [2147483648 x i16] zeroinitializer, align 4
+
+; CHECK: Global variable is too large to fit into the address space
+; CHECK-NEXT: ptr addrspace(1) @G1
+; CHECK-NEXT: [65537 x i8]
+; CHECK: Global variable is too large to fit into the address space
+; CHECK-NEXT: ptr addrspace(2) @G2
+; CHECK-NEXT: [2147483649 x i16]
+; CHECK-NOT: Global variable is too large to fit into the address space
|
|
Tag @nikic & @boomanaiden154 from #178391 for awareness. ⭐ |
| Check(GVType->isScalableTy() || !GVType->isSized() || | ||
| GV.getGlobalSize(DL) == 0 || | ||
| isUIntN(DL.getAddressSizeInBits(GV.getAddressSpace()), | ||
| GV.getGlobalSize(DL) - 1), |
There was a problem hiding this comment.
Strictly speaking objects can only take up half the address space. But even if we don't enforce that as an IR well-formedness requirement, I think we should at least be dropping the -1 here, otherwise we're still going to be dealing with overflow issues.
| isUIntN(DL.getAddressSizeInBits(GV.getAddressSpace()), | ||
| GV.getGlobalSize(DL) - 1), | ||
| "Global variable is too large to fit into the address space", &GV, | ||
| GVType); |
There was a problem hiding this comment.
Probably worth noting that this doesn't catch cases that overflow during the size calculation, but that's something we can handle separately...
Signed-off-by: Steffen Holst Larsen <HolstLarsen.Steffen@amd.com>
Signed-off-by: Steffen Holst Larsen <HolstLarsen.Steffen@amd.com>
|
@steffenlarsen Hello Steffen, we are hitting this assertion when building the embedded Swift stdlib: Claude came back with
Does this sound reasonable to you? |
|
Thanks for making me aware of this, @AnthonyLatsis ! Since it is just for metadata purposes, I believe it should be safe enough to let global variables like this through this check. I've proposed this in #212733. |
…ress-space size check Commit 9501114 ("[Verifier] Make verifier fail when global variable size exceeds address space size", llvm#179625) added a check that a global's size fits within its address space, reasoning that an access must be able to reach all bytes of the global. That invariant does not hold for globals whose bytes are located and consumed by section rather than addressed as ordinary data. Clang's embedded PCH/module container emits exactly such a global: the "__clang_ast" AST blob (frequently >64KB) pinned to the "__clangast" section and extracted from the object file by section name, never addressed at run time. It has internal linkage and no users, yet the new check rejected it when building embedded Swift modules for a small-address-space target (avr-none-none-elf, 16-bit data space): Global variable is too large to fit into the address space ptr @__clang_ast [136164 x i8] Skip the check for a global that is section-anchored and has no uses: with no accesses, none can exceed the address space, so the invariant is vacuous. Globals that are actually referenced remain fully checked. Assisted-by: Claude Code
|
@steffenlarsen Thank you for fixing this! Would you mind cherry-picking to |
When a global variable has a size that exceeds the size of the address space it resides in, the verifier should fail as the variable can neither be materialized nor fully accessed. This patch adds a check to the verifier to enforce it.