[StackSafetyAnalysis] Don't call getTruncateOrZeroExtend for pointers of different sizes #79804

Pierre-vh · 2024-01-29T10:32:36Z

Otherwise SCEV asserts Can't extend pointer!
I assume the proper fix would be to teach SCEV about addrspacecast but this is a blocking issue downstream so I'm making a quick fix.

Fixes SWDEV-442670

…tly-sized pointers Otherwise SCEV asserts. Fixes SWDEV-442670

llvmbot · 2024-01-29T10:33:08Z

@llvm/pr-subscribers-llvm-analysis

Author: Pierre van Houtryve (Pierre-vh)

Changes

Otherwise SCEV asserts Can't extend pointer!
I assume the proper fix would be to teach SCEV about addrspacecast but this is a blocking issue downstream so I'm making a quick fix.

Fixes SWDEV-442670

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

2 Files Affected:

(modified) llvm/lib/Analysis/StackSafetyAnalysis.cpp (+11)
(added) llvm/test/Analysis/StackSafetyAnalysis/extend-ptr.ll (+18)

diff --git a/llvm/lib/Analysis/StackSafetyAnalysis.cpp b/llvm/lib/Analysis/StackSafetyAnalysis.cpp
index 19991c1a7baee6..b06c31f1b1c331 100644
--- a/llvm/lib/Analysis/StackSafetyAnalysis.cpp
+++ b/llvm/lib/Analysis/StackSafetyAnalysis.cpp
@@ -273,6 +273,17 @@ ConstantRange StackSafetyLocalAnalysis::offsetFrom(Value *Addr, Value *Base) {
     return UnknownRange;
 
   auto *PtrTy = PointerType::getUnqual(SE.getContext());
+
+  // SCEV will crash if we try to extend/truncate between differently-sized
+  // pointer.
+  const auto CanExtend = [&](Type *Ty) {
+    return !Ty->isPointerTy() ||
+           DL.getTypeSizeInBits(Ty) == DL.getTypeSizeInBits(PtrTy);
+  };
+
+  if (!CanExtend(Addr->getType()) || !CanExtend(Base->getType()))
+    return UnknownRange;
+
   const SCEV *AddrExp = SE.getTruncateOrZeroExtend(SE.getSCEV(Addr), PtrTy);
   const SCEV *BaseExp = SE.getTruncateOrZeroExtend(SE.getSCEV(Base), PtrTy);
   const SCEV *Diff = SE.getMinusSCEV(AddrExp, BaseExp);
diff --git a/llvm/test/Analysis/StackSafetyAnalysis/extend-ptr.ll b/llvm/test/Analysis/StackSafetyAnalysis/extend-ptr.ll
new file mode 100644
index 00000000000000..bb9ae92c24fdda
--- /dev/null
+++ b/llvm/test/Analysis/StackSafetyAnalysis/extend-ptr.ll
@@ -0,0 +1,18 @@
+; RUN: opt %s -disable-output -S -passes="print<stack-safety-local>" 2>&1 | FileCheck %s
+
+; Datalayout from AMDGPU, p5 is 32 bits and p is 64.
+; We used to call SCEV getTruncateOrZeroExtend on %x.ascast/%x which caused an assertion failure.
+
+; CHECK:      @a dso_preemptable
+; CHECK-NEXT:   args uses:
+; CHECK-NEXT:     x[]: full-set
+; CHECK-NEXT:   allocas uses:
+
+target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8:9"
+
+define void @a(ptr addrspace(5) %x) {
+entry:
+  %x.ascast = addrspacecast ptr addrspace(5) %x to ptr
+  store i64 0, ptr %x.ascast, align 8
+  ret void
+}

arsenm · 2024-01-29T10:36:50Z

llvm/lib/Analysis/StackSafetyAnalysis.cpp

@@ -273,6 +273,17 @@ ConstantRange StackSafetyLocalAnalysis::offsetFrom(Value *Addr, Value *Base) {
    return UnknownRange;

  auto *PtrTy = PointerType::getUnqual(SE.getContext());


Didn't follow the context, but PointerType::getUnqual should never be used. Can this just use Addr->getType()?

I also found another use of getUnqual in this pass that still causes the assert to trip, so indeed seems like we should get rid of those instead. I'll try it

I tried it and the pass messes up somewhere else instead. It seems like the pass isn't written with handling address spaces with differently sized pointers at all. It even hardcodes the pointer size for AS 0 and uses it in a bunch of places.

The changes needed to the pass seems to be big, I don't mind looking at it sometime but this is really a blocker so we need a quick bandaid first.

Based on the context, I'm guessing another quick bandaid would be to replace the hardcoded PtrTy with the alloca address space pointer type

I'd expect any fix to this to at least remove all the getTruncateOrZeroExtend() calls on pointer SCEVS in this pass, as that fundamentally never makes sense.

I updated the diff so getTruncateOrZeroExtend is no longer called for pointers

arsenm · 2024-01-29T11:26:01Z

llvm/lib/Analysis/StackSafetyAnalysis.cpp

+
+  // FIXME: Pass does not deal with pointers from different address spaces that
+  // don't have the same size.
+  if (DL.getTypeSizeInBits(Addr->getType()) != PointerSize ||


Checking mismatched address space would also be better?

This pass works fine for same size AS I think, because getTruncateOrZeroExtend will not try to do anything as it just checks the type size and not the types themselves.

I guess the easiest thing then is to just reject any AS != 0 explicitly

I guess the easiest thing then is to just reject any AS != 0 explicitly

Yes. This pass is just a optimization.

nikic · 2024-01-29T14:26:39Z

llvm/lib/Analysis/StackSafetyAnalysis.cpp

+  Type *ValTy = Val->getType();
+
+  auto *PtrTy = PointerType::getUnqual(SE.getContext());
+  if (ValTy->isPointerTy()) {


Is this ever false? I don't think this code is working on anything but pointers.

yeah it only works on pointers, I added an assert and it doesn't trip

nikic

LGTM as a workaround. I looked over the pass a bit, and it's a real mess.

fmayer · 2024-01-29T22:05:47Z

From the presubmit bot, seems the assertion is not valid:

clang: /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-tdgkv-1/llvm-project/github-pull-requests/llvm/lib/Analysis/StackSafetyAnalysis.cpp:283: const SCEV *(anonymous namespace)::StackSafetyLocalAnalysis::getSCEVAsPointer(Value *): Assertion `ValTy->isPointerTy()' failed.

MaskRay · 2024-01-29T22:43:28Z

llvm/test/Analysis/StackSafetyAnalysis/extend-ptr.ll

+; CHECK-NEXT:   allocas uses:
+
+target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8:9"
+


Set target triple and specify ; REQUIRES: amdgpu-registered-target

Why? The datalayout string should work the same for all targets in this context (just running one IR analysis), no?

github-actions · 2024-01-30T08:02:57Z

✅ With the latest revision this PR passed the C/C++ code formatter.

nikic · 2024-01-30T09:50:09Z

I've added a regression test for the pointer assertion failure in 4c2422e. The pass is blindly following all instruction uses in

llvm-project/llvm/lib/Analysis/StackSafetyAnalysis.cpp

Lines 524 to 525 in 4c2422e

    
           if (Visited.insert(I).second) 
        
             WorkList.push_back(cast<const Instruction>(I));

, while it should only be following bitcast, GEP, select and phi and bailing out on everything else.

nikic · 2024-01-30T11:16:21Z

When trying to fix that it turns out that StackSafetyAnalysis assumes that all accesses it doesn't visit are "safe". For example, in

define void @indirect(ptr %p) {
entry:
  %a = alloca i8
  store ptr %a, ptr %p
  %a2 = load ptr, ptr %p
  store i64 0, ptr %a2
  ret void
}

the last store is considered "safe" even though it obviously isn't. Apparently the reason why this doesn't miscompile everything is that asan will only use stackAccessIsSafe() if findAllocaForValue() also succeeds.

fmayer · 2024-01-30T17:10:50Z

When trying to fix that it turns out that StackSafetyAnalysis assumes that all accesses it doesn't visit are "safe". For example, in
define void @indirect(ptr %p) {
entry:
  %a = alloca i8
  store ptr %a, ptr %p
  %a2 = load ptr, ptr %p
  store i64 0, ptr %a2
  ret void
}
the last store is considered "safe" even though it obviously isn't. Apparently the reason why this doesn't miscompile everything is that asan will only use stackAccessIsSafe() if findAllocaForValue() also succeeds.

Why is this surprising? stackAccessIsSafe talks about stack access, if findAllocaForValue doesn't succeed it isn't necessarily a stack access, so using stackAccessIsSafe doesn't make sense.

… of different sizes (llvm#79804) Otherwise SCEV asserts `Can't extend pointer!` Fixes SWDEV-442670 Change-Id: If1db5661c26760462acb7b05a1cd333a4eb01b88

[StackSafetyAnalysis] Don't call getTruncateOrZeroExtend for differen…

f3d5e4c

…tly-sized pointers Otherwise SCEV asserts. Fixes SWDEV-442670

Pierre-vh requested review from fmayer, arsenm and MaskRay January 29, 2024 10:32

llvmbot added the llvm:analysis label Jan 29, 2024

arsenm reviewed Jan 29, 2024

View reviewed changes

Fix both cases

d91dae9

Pierre-vh requested a review from arsenm January 29, 2024 11:16

arsenm reviewed Jan 29, 2024

View reviewed changes

don't call getTruncateOrZeroExtend on ptrs

8e2af9c

Pierre-vh requested review from arsenm and nikic January 29, 2024 13:02

nikic reviewed Jan 29, 2024

View reviewed changes

don't bother with non-pointer types

1d7a8dc

Pierre-vh requested a review from nikic January 29, 2024 15:54

nikic approved these changes Jan 29, 2024

View reviewed changes

MaskRay reviewed Jan 29, 2024

View reviewed changes

non-pointer types still need attention in the end

8e4fc3c

format

f90d22f

Pierre-vh merged commit 2fb3c9b into llvm:main Jan 30, 2024
4 checks passed

Pierre-vh deleted the asan-bug branch January 30, 2024 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[StackSafetyAnalysis] Don't call getTruncateOrZeroExtend for pointers of different sizes #79804

[StackSafetyAnalysis] Don't call getTruncateOrZeroExtend for pointers of different sizes #79804

Pierre-vh commented Jan 29, 2024

llvmbot commented Jan 29, 2024

arsenm Jan 29, 2024

Pierre-vh Jan 29, 2024

Pierre-vh Jan 29, 2024

arsenm Jan 29, 2024 •

edited

nikic Jan 29, 2024

Pierre-vh Jan 29, 2024

arsenm Jan 29, 2024

Pierre-vh Jan 29, 2024

vitalybuka Jan 30, 2024 •

edited

nikic Jan 29, 2024

Pierre-vh Jan 29, 2024

nikic left a comment

fmayer commented Jan 29, 2024

MaskRay Jan 29, 2024

Pierre-vh Jan 30, 2024

github-actions bot commented Jan 30, 2024 •

edited

nikic commented Jan 30, 2024 •

edited

nikic commented Jan 30, 2024 •

edited

fmayer commented Jan 30, 2024

		@@ -273,6 +273,17 @@ ConstantRange StackSafetyLocalAnalysis::offsetFrom(Value Addr, Value Base) {
		return UnknownRange;

		auto *PtrTy = PointerType::getUnqual(SE.getContext());

		; CHECK-NEXT: allocas uses:

		target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8:9"

[StackSafetyAnalysis] Don't call getTruncateOrZeroExtend for pointers of different sizes #79804

[StackSafetyAnalysis] Don't call getTruncateOrZeroExtend for pointers of different sizes #79804

Conversation

Pierre-vh commented Jan 29, 2024

llvmbot commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arsenm Jan 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitalybuka Jan 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

fmayer commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 30, 2024 • edited

nikic commented Jan 30, 2024 • edited

nikic commented Jan 30, 2024 • edited

fmayer commented Jan 30, 2024

arsenm Jan 29, 2024 •

edited

vitalybuka Jan 30, 2024 •

edited

github-actions bot commented Jan 30, 2024 •

edited

nikic commented Jan 30, 2024 •

edited

nikic commented Jan 30, 2024 •

edited