[llvm] [polly] [InstCombine][asan] Don't speculate loads before `select ptr` (PR #100773)
Vitaly Buka via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 26 11:58:24 PDT 2024
https://github.com/vitalybuka updated https://github.com/llvm/llvm-project/pull/100773
>From ffe82da6043bd9f15abf4ab8075b1cf8f84fb2fa Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Fri, 26 Jul 2024 11:18:44 -0700
Subject: [PATCH 1/3] [NFC][Load] Find better place for
`mustSuppressSpeculation`
And extract `suppressSpeculativeLoadForSanitizers`.
Pull Request: https://github.com/llvm/llvm-project/pull/100794
---
llvm/include/llvm/Analysis/Loads.h | 7 +++++++
llvm/include/llvm/Analysis/ValueTracking.h | 7 -------
llvm/lib/Analysis/Loads.cpp | 13 +++++++++++++
llvm/lib/Analysis/ValueTracking.cpp | 11 -----------
4 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/llvm/include/llvm/Analysis/Loads.h b/llvm/include/llvm/Analysis/Loads.h
index 33e817828b754..38f86f77b4158 100644
--- a/llvm/include/llvm/Analysis/Loads.h
+++ b/llvm/include/llvm/Analysis/Loads.h
@@ -106,6 +106,13 @@ bool isSafeToLoadUnconditionally(Value *V, Type *Ty, Align Alignment,
const DominatorTree *DT = nullptr,
const TargetLibraryInfo *TLI = nullptr);
+/// Return true if speculation of the given load must be suppressed to avoid
+/// ordering or interfering with an active sanitizer. If not suppressed,
+/// dereferenceability and alignment must be proven separately. Note: This
+/// is only needed for raw reasoning; if you use the interface below
+/// (isSafeToSpeculativelyExecute), this is handled internally.
+bool mustSuppressSpeculation(const LoadInst &LI);
+
/// The default number of maximum instructions to scan in the block, used by
/// FindAvailableLoadedValue().
extern cl::opt<unsigned> DefMaxInstsToScan;
diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index 5ef6e43483906..96fa16970584d 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -792,13 +792,6 @@ bool onlyUsedByLifetimeMarkers(const Value *V);
/// droppable instructions.
bool onlyUsedByLifetimeMarkersOrDroppableInsts(const Value *V);
-/// Return true if speculation of the given load must be suppressed to avoid
-/// ordering or interfering with an active sanitizer. If not suppressed,
-/// dereferenceability and alignment must be proven separately. Note: This
-/// is only needed for raw reasoning; if you use the interface below
-/// (isSafeToSpeculativelyExecute), this is handled internally.
-bool mustSuppressSpeculation(const LoadInst &LI);
-
/// Return true if the instruction does not have any effects besides
/// calculating the result and does not have undefined behavior.
///
diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 61c6aa5e5a3eb..1704f0db4c599 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -345,6 +345,19 @@ bool llvm::isDereferenceableAndAlignedInLoop(LoadInst *LI, Loop *L,
HeaderFirstNonPHI, AC, &DT);
}
+static bool suppressSpeculativeLoadForSanitizers(const Instruction &CtxI) {
+ const Function &F = *CtxI.getFunction();
+ // Speculative load may create a race that did not exist in the source.
+ return F.hasFnAttribute(Attribute::SanitizeThread) ||
+ // Speculative load may load data from dirty regions.
+ F.hasFnAttribute(Attribute::SanitizeAddress) ||
+ F.hasFnAttribute(Attribute::SanitizeHWAddress);
+}
+
+bool llvm::mustSuppressSpeculation(const LoadInst &LI) {
+ return !LI.isUnordered() || suppressSpeculativeLoadForSanitizers(LI);
+}
+
/// Check if executing a load of this pointer value cannot trap.
///
/// If DT and ScanFrom are specified this method performs context-sensitive
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index bfd26fadd237b..497f6eafd22d8 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -6798,17 +6798,6 @@ bool llvm::onlyUsedByLifetimeMarkersOrDroppableInsts(const Value *V) {
V, /* AllowLifetime */ true, /* AllowDroppable */ true);
}
-bool llvm::mustSuppressSpeculation(const LoadInst &LI) {
- if (!LI.isUnordered())
- return true;
- const Function &F = *LI.getFunction();
- // Speculative load may create a race that did not exist in the source.
- return F.hasFnAttribute(Attribute::SanitizeThread) ||
- // Speculative load may load data from dirty regions.
- F.hasFnAttribute(Attribute::SanitizeAddress) ||
- F.hasFnAttribute(Attribute::SanitizeHWAddress);
-}
-
bool llvm::isSafeToSpeculativelyExecute(const Instruction *Inst,
const Instruction *CtxI,
AssumptionCache *AC,
>From 403561bd1ef32d20dc73a0e89328c9467d6f4a25 Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Fri, 26 Jul 2024 11:04:32 -0700
Subject: [PATCH 2/3] [NFC][Load] Make `ScanFrom` required parameters
In #100773 we will go conservative for sanitizers,
so it's better to pinpoint location consciously.
Pull Request: https://github.com/llvm/llvm-project/pull/100789
---
llvm/include/llvm/Analysis/Loads.h | 6 ++----
polly/lib/Analysis/ScopBuilder.cpp | 2 +-
polly/lib/Analysis/ScopDetection.cpp | 3 ++-
3 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/llvm/include/llvm/Analysis/Loads.h b/llvm/include/llvm/Analysis/Loads.h
index 38f86f77b4158..1f01ff7027fa9 100644
--- a/llvm/include/llvm/Analysis/Loads.h
+++ b/llvm/include/llvm/Analysis/Loads.h
@@ -69,8 +69,7 @@ bool isDereferenceableAndAlignedPointer(const Value *V, Align Alignment,
/// quick local scan of the basic block containing ScanFrom, to determine if
/// the address is already accessed.
bool isSafeToLoadUnconditionally(Value *V, Align Alignment, const APInt &Size,
- const DataLayout &DL,
- Instruction *ScanFrom = nullptr,
+ const DataLayout &DL, Instruction *ScanFrom,
AssumptionCache *AC = nullptr,
const DominatorTree *DT = nullptr,
const TargetLibraryInfo *TLI = nullptr);
@@ -100,8 +99,7 @@ bool isDereferenceableReadOnlyLoop(Loop *L, ScalarEvolution *SE,
/// quick local scan of the basic block containing ScanFrom, to determine if
/// the address is already accessed.
bool isSafeToLoadUnconditionally(Value *V, Type *Ty, Align Alignment,
- const DataLayout &DL,
- Instruction *ScanFrom = nullptr,
+ const DataLayout &DL, Instruction *ScanFrom,
AssumptionCache *AC = nullptr,
const DominatorTree *DT = nullptr,
const TargetLibraryInfo *TLI = nullptr);
diff --git a/polly/lib/Analysis/ScopBuilder.cpp b/polly/lib/Analysis/ScopBuilder.cpp
index d594823410f5f..0b9a1a916e1c1 100644
--- a/polly/lib/Analysis/ScopBuilder.cpp
+++ b/polly/lib/Analysis/ScopBuilder.cpp
@@ -2770,7 +2770,7 @@ isl::set ScopBuilder::getNonHoistableCtx(MemoryAccess *Access,
auto &DL = scop->getFunction().getDataLayout();
if (isSafeToLoadUnconditionally(LI->getPointerOperand(), LI->getType(),
- LI->getAlign(), DL)) {
+ LI->getAlign(), DL, nullptr)) {
SafeToLoad = isl::set::universe(AccessRelation.get_space().range());
} else if (BB != LI->getParent()) {
// Skip accesses in non-affine subregions as they might not be executed
diff --git a/polly/lib/Analysis/ScopDetection.cpp b/polly/lib/Analysis/ScopDetection.cpp
index eab7bd83e6a4e..79db3965de023 100644
--- a/polly/lib/Analysis/ScopDetection.cpp
+++ b/polly/lib/Analysis/ScopDetection.cpp
@@ -490,7 +490,8 @@ bool ScopDetection::onlyValidRequiredInvariantLoads(
for (auto NonAffineRegion : Context.NonAffineSubRegionSet) {
if (isSafeToLoadUnconditionally(Load->getPointerOperand(),
- Load->getType(), Load->getAlign(), DL))
+ Load->getType(), Load->getAlign(), DL,
+ nullptr))
continue;
if (NonAffineRegion->contains(Load) &&
>From b8cc3202637c414df642b2a6fb82fa84c62e86a0 Mon Sep 17 00:00:00 2001
From: Vitaly Buka <vitalybuka at google.com>
Date: Fri, 26 Jul 2024 09:18:32 -0700
Subject: [PATCH 3/3] [InstCombine][asan] Don't speculate loads before `select
ptr`
Even if memory is valid from `llvm` point of view,
e.g. local alloca, sanitizers have API for user
specific memory annotations.
This annotations can be used to track size of the
local object, e.g. inline vector like may prevent
accessed beyond the current vector size.
So valid programs should not access those parts of
alloca before checking preconditions.
Fixes #100639.
Pull Request: https://github.com/llvm/llvm-project/pull/100773
---
llvm/lib/Analysis/Loads.cpp | 3 +
.../Transforms/InstCombine/select-load.ll | 101 ++++++++++++++++++
2 files changed, 104 insertions(+)
create mode 100644 llvm/test/Transforms/InstCombine/select-load.ll
diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 1704f0db4c599..6b14473d1c843 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -375,6 +375,9 @@ bool llvm::isSafeToLoadUnconditionally(Value *V, Align Alignment, const APInt &S
AssumptionCache *AC,
const DominatorTree *DT,
const TargetLibraryInfo *TLI) {
+ if (ScanFrom && suppressSpeculativeLoadForSanitizers(*ScanFrom))
+ return false;
+
// If DT is not specified we can't make context-sensitive query
const Instruction* CtxI = DT ? ScanFrom : nullptr;
if (isDereferenceableAndAlignedPointer(V, Alignment, Size, DL, CtxI, AC, DT,
diff --git a/llvm/test/Transforms/InstCombine/select-load.ll b/llvm/test/Transforms/InstCombine/select-load.ll
new file mode 100644
index 0000000000000..36883423aea36
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/select-load.ll
@@ -0,0 +1,101 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -passes=instcombine -S < %s | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-grtev4-linux-gnu"
+
+define i32 @test_plain(i1 %f) {
+; CHECK-LABEL: @test_plain(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[A:%.*]] = alloca i32, align 8
+; CHECK-NEXT: [[B:%.*]] = alloca i32, align 8
+; CHECK-NEXT: [[A_VAL:%.*]] = load i32, ptr [[A]], align 8
+; CHECK-NEXT: [[B_VAL:%.*]] = load i32, ptr [[B]], align 8
+; CHECK-NEXT: [[L:%.*]] = select i1 [[F:%.*]], i32 [[A_VAL]], i32 [[B_VAL]]
+; CHECK-NEXT: ret i32 [[L]]
+;
+entry:
+ %a = alloca i32, align 8
+ %b = alloca i32, align 8
+ %sel = select i1 %f, ptr %a, ptr %b
+ %l = load i32, ptr %sel, align 8
+ ret i32 %l
+}
+
+; Don't speculate as the condition may control which memory is valid from
+; sanitizer perspective.
+define i32 @test_asan(i1 %f) sanitize_address {
+; CHECK-LABEL: @test_asan(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[A:%.*]] = alloca i32, align 8
+; CHECK-NEXT: [[B:%.*]] = alloca i32, align 8
+; CHECK-NEXT: [[SEL:%.*]] = select i1 [[F:%.*]], ptr [[A]], ptr [[B]]
+; CHECK-NEXT: [[L:%.*]] = load i32, ptr [[SEL]], align 8
+; CHECK-NEXT: ret i32 [[L]]
+;
+entry:
+ %a = alloca i32, align 8
+ %b = alloca i32, align 8
+ %sel = select i1 %f, ptr %a, ptr %b
+ %l = load i32, ptr %sel, align 8
+ ret i32 %l
+}
+
+
+; Don't speculate as the condition may control which memory is valid from
+; sanitizer perspective.
+define i32 @test_hwasan(i1 %f) sanitize_hwaddress {
+; CHECK-LABEL: @test_hwasan(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[A:%.*]] = alloca i32, align 8
+; CHECK-NEXT: [[B:%.*]] = alloca i32, align 8
+; CHECK-NEXT: [[SEL:%.*]] = select i1 [[F:%.*]], ptr [[A]], ptr [[B]]
+; CHECK-NEXT: [[L:%.*]] = load i32, ptr [[SEL]], align 8
+; CHECK-NEXT: ret i32 [[L]]
+;
+entry:
+ %a = alloca i32, align 8
+ %b = alloca i32, align 8
+ %sel = select i1 %f, ptr %a, ptr %b
+ %l = load i32, ptr %sel, align 8
+ ret i32 %l
+}
+
+; Don't speculate as the condition may control which memory is valid from
+; sanitizer perspective.
+define i32 @test_tsan(i1 %f) sanitize_thread {
+; CHECK-LABEL: @test_tsan(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[A:%.*]] = alloca i32, align 8
+; CHECK-NEXT: [[B:%.*]] = alloca i32, align 8
+; CHECK-NEXT: [[SEL:%.*]] = select i1 [[F:%.*]], ptr [[A]], ptr [[B]]
+; CHECK-NEXT: [[L:%.*]] = load i32, ptr [[SEL]], align 8
+; CHECK-NEXT: ret i32 [[L]]
+;
+entry:
+ %a = alloca i32, align 8
+ %b = alloca i32, align 8
+ %sel = select i1 %f, ptr %a, ptr %b
+ %l = load i32, ptr %sel, align 8
+ ret i32 %l
+}
+
+; Msan just propagates shadow, even if speculated load accesses uninitialized
+; value, instrumentation will select shadow of the desired value anyway.
+define i32 @test_msan(i1 %f) sanitize_memory {
+; CHECK-LABEL: @test_msan(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[A:%.*]] = alloca i32, align 8
+; CHECK-NEXT: [[B:%.*]] = alloca i32, align 8
+; CHECK-NEXT: [[A_VAL:%.*]] = load i32, ptr [[A]], align 8
+; CHECK-NEXT: [[B_VAL:%.*]] = load i32, ptr [[B]], align 8
+; CHECK-NEXT: [[L:%.*]] = select i1 [[F:%.*]], i32 [[A_VAL]], i32 [[B_VAL]]
+; CHECK-NEXT: ret i32 [[L]]
+;
+entry:
+ %a = alloca i32, align 8
+ %b = alloca i32, align 8
+ %sel = select i1 %f, ptr %a, ptr %b
+ %l = load i32, ptr %sel, align 8
+ ret i32 %l
+}
More information about the llvm-commits
mailing list