[BoundsChecking] Add guard= pass parameter #122575
Merged
+74
−5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.4
|
@llvm/pr-subscribers-llvm-transforms Author: Vitaly Buka (vitalybuka) ChangesAnd use that as an argument for Other ubsan checks use SanitizerKind, So make it a parameter in LLVM. Full diff: https://github.com/llvm/llvm-project/pull/122575.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Instrumentation/BoundsChecking.h b/llvm/include/llvm/Transforms/Instrumentation/BoundsChecking.h
index 836fc907375d30..ab2dcee06551eb 100644
--- a/llvm/include/llvm/Transforms/Instrumentation/BoundsChecking.h
+++ b/llvm/include/llvm/Transforms/Instrumentation/BoundsChecking.h
@@ -29,6 +29,7 @@ class BoundsCheckingPass : public PassInfoMixin<BoundsCheckingPass> {
};
std::optional<Runtime> Rt; // Trap if empty.
bool Merge = false;
+ std::optional<int8_t> GuardKind; // `allow_ubsan_check` argument.
};
BoundsCheckingPass(Options Opts) : Opts(Opts) {}
diff --git a/llvm/lib/Passes/PassBuilder.cpp b/llvm/lib/Passes/PassBuilder.cpp
index aac44077400550..f923d5aabe0a0e 100644
--- a/llvm/lib/Passes/PassBuilder.cpp
+++ b/llvm/lib/Passes/PassBuilder.cpp
@@ -1315,10 +1315,18 @@ parseBoundsCheckingOptions(StringRef Params) {
} else if (ParamName == "merge") {
Options.Merge = true;
} else {
- return make_error<StringError>(
- formatv("invalid BoundsChecking pass parameter '{0}' ", ParamName)
- .str(),
- inconvertibleErrorCode());
+ StringRef ParamEQ;
+ StringRef Val;
+ std::tie(ParamEQ, Val) = ParamName.split('=');
+ int8_t Id = 0;
+ if (ParamEQ == "guard" && !Val.getAsInteger(0, Id)) {
+ Options.GuardKind = Id;
+ } else {
+ return make_error<StringError>(
+ formatv("invalid BoundsChecking pass parameter '{0}' ", ParamName)
+ .str(),
+ inconvertibleErrorCode());
+ }
}
}
return Options;
diff --git a/llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp b/llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
index 8004552250b471..609678f9979c63 100644
--- a/llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
+++ b/llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
@@ -214,8 +214,15 @@ static bool addBoundsChecking(Function &F, TargetLibraryInfo &TLI,
Or = getBoundsCheckCond(AI->getPointerOperand(), AI->getValOperand(),
DL, TLI, ObjSizeEval, IRB, SE);
}
- if (Or)
+ if (Or) {
+ if (Opts.GuardKind) {
+ llvm::Value *Allow = IRB.CreateIntrinsic(
+ IRB.getInt1Ty(), Intrinsic::allow_ubsan_check,
+ {llvm::ConstantInt::getSigned(IRB.getInt8Ty(), *Opts.GuardKind)});
+ Or = IRB.CreateAnd(Or, Allow);
+ }
TrapInfo.push_back(std::make_pair(&I, Or));
+ }
}
std::string Name;
@@ -299,5 +306,7 @@ void BoundsCheckingPass::printPipeline(
}
if (Opts.Merge)
OS << ";merge";
+ if (Opts.GuardKind)
+ OS << ";guard=" << static_cast<int>(*Opts.GuardKind);
OS << ">";
}
diff --git a/llvm/test/Instrumentation/BoundsChecking/runtimes.ll b/llvm/test/Instrumentation/BoundsChecking/runtimes.ll
index ccc7e93615fed2..055bd5544ac38d 100644
--- a/llvm/test/Instrumentation/BoundsChecking/runtimes.ll
+++ b/llvm/test/Instrumentation/BoundsChecking/runtimes.ll
@@ -9,6 +9,8 @@
; RUN: opt < %s -passes='bounds-checking<min-rt>' -S | FileCheck %s --check-prefixes=MINRT-NOMERGE
; RUN: opt < %s -passes='bounds-checking<min-rt-abort>' -S | FileCheck %s --check-prefixes=MINRTABORT-NOMERGE
;
+; RUN: opt < %s -passes='bounds-checking<trap;guard=3>' -S | FileCheck %s --check-prefixes=TR-GUARD
+; RUN: opt < %s -passes='bounds-checking<rt;guard=-5>' -S | FileCheck %s --check-prefixes=RT-GUARDRT
target datalayout = "e-p:64:64:64-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"
define void @f1(i64 %x) nounwind {
@@ -123,6 +125,42 @@ define void @f1(i64 %x) nounwind {
; MINRTABORT-NOMERGE: [[TRAP]]:
; MINRTABORT-NOMERGE-NEXT: call void @__ubsan_handle_local_out_of_bounds_minimal_abort() #[[ATTR2:[0-9]+]], !nosanitize [[META0]]
; MINRTABORT-NOMERGE-NEXT: unreachable, !nosanitize [[META0]]
+;
+; TR-GUARD-LABEL: define void @f1(
+; TR-GUARD-SAME: i64 [[X:%.*]]) #[[ATTR0:[0-9]+]] {
+; TR-GUARD-NEXT: [[TMP1:%.*]] = mul i64 16, [[X]]
+; TR-GUARD-NEXT: [[TMP2:%.*]] = alloca i128, i64 [[X]], align 8
+; TR-GUARD-NEXT: [[TMP3:%.*]] = sub i64 [[TMP1]], 0, !nosanitize [[META0:![0-9]+]]
+; TR-GUARD-NEXT: [[TMP4:%.*]] = icmp ult i64 [[TMP3]], 16, !nosanitize [[META0]]
+; TR-GUARD-NEXT: [[TMP5:%.*]] = or i1 false, [[TMP4]], !nosanitize [[META0]]
+; TR-GUARD-NEXT: [[TMP6:%.*]] = or i1 false, [[TMP5]], !nosanitize [[META0]]
+; TR-GUARD-NEXT: [[TMP7:%.*]] = call i1 @llvm.allow.ubsan.check(i8 3), !nosanitize [[META0]]
+; TR-GUARD-NEXT: [[TMP8:%.*]] = and i1 [[TMP6]], [[TMP7]], !nosanitize [[META0]]
+; TR-GUARD-NEXT: br i1 [[TMP8]], label %[[TRAP:.*]], label %[[BB9:.*]]
+; TR-GUARD: [[BB9]]:
+; TR-GUARD-NEXT: [[TMP10:%.*]] = load i128, ptr [[TMP2]], align 4
+; TR-GUARD-NEXT: ret void
+; TR-GUARD: [[TRAP]]:
+; TR-GUARD-NEXT: call void @llvm.ubsantrap(i8 3) #[[ATTR3:[0-9]+]], !nosanitize [[META0]]
+; TR-GUARD-NEXT: unreachable, !nosanitize [[META0]]
+;
+; RT-GUARDRT-LABEL: define void @f1(
+; RT-GUARDRT-SAME: i64 [[X:%.*]]) #[[ATTR0:[0-9]+]] {
+; RT-GUARDRT-NEXT: [[TMP1:%.*]] = mul i64 16, [[X]]
+; RT-GUARDRT-NEXT: [[TMP2:%.*]] = alloca i128, i64 [[X]], align 8
+; RT-GUARDRT-NEXT: [[TMP3:%.*]] = sub i64 [[TMP1]], 0, !nosanitize [[META0:![0-9]+]]
+; RT-GUARDRT-NEXT: [[TMP4:%.*]] = icmp ult i64 [[TMP3]], 16, !nosanitize [[META0]]
+; RT-GUARDRT-NEXT: [[TMP5:%.*]] = or i1 false, [[TMP4]], !nosanitize [[META0]]
+; RT-GUARDRT-NEXT: [[TMP6:%.*]] = or i1 false, [[TMP5]], !nosanitize [[META0]]
+; RT-GUARDRT-NEXT: [[TMP7:%.*]] = call i1 @llvm.allow.ubsan.check(i8 -5), !nosanitize [[META0]]
+; RT-GUARDRT-NEXT: [[TMP8:%.*]] = and i1 [[TMP6]], [[TMP7]], !nosanitize [[META0]]
+; RT-GUARDRT-NEXT: br i1 [[TMP8]], label %[[TRAP:.*]], label %[[BB9:.*]]
+; RT-GUARDRT: [[BB9]]:
+; RT-GUARDRT-NEXT: [[TMP10:%.*]] = load i128, ptr [[TMP2]], align 4
+; RT-GUARDRT-NEXT: ret void
+; RT-GUARDRT: [[TRAP]]:
+; RT-GUARDRT-NEXT: call void @__ubsan_handle_local_out_of_bounds() #[[ATTR2:[0-9]+]], !nosanitize [[META0]]
+; RT-GUARDRT-NEXT: br label %[[BB9]], !nosanitize [[META0]]
;
%1 = alloca i128, i64 %x
%3 = load i128, ptr %1, align 4
@@ -154,6 +192,15 @@ define void @f1(i64 %x) nounwind {
; MINRTABORT-NOMERGE: attributes #[[ATTR1:[0-9]+]] = { noreturn nounwind }
; MINRTABORT-NOMERGE: attributes #[[ATTR2]] = { nomerge noreturn nounwind }
;.
+; TR-GUARD: attributes #[[ATTR0]] = { nounwind }
+; TR-GUARD: attributes #[[ATTR1:[0-9]+]] = { nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: write) }
+; TR-GUARD: attributes #[[ATTR2:[0-9]+]] = { cold noreturn nounwind }
+; TR-GUARD: attributes #[[ATTR3]] = { nomerge noreturn nounwind }
+;.
+; RT-GUARDRT: attributes #[[ATTR0]] = { nounwind }
+; RT-GUARDRT: attributes #[[ATTR1:[0-9]+]] = { nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: write) }
+; RT-GUARDRT: attributes #[[ATTR2]] = { nomerge nounwind }
+;.
; TR: [[META0]] = !{}
;.
; RT: [[META0]] = !{}
@@ -168,3 +215,7 @@ define void @f1(i64 %x) nounwind {
;.
; MINRTABORT-NOMERGE: [[META0]] = !{}
;.
+; TR-GUARD: [[META0]] = !{}
+;.
+; RT-GUARDRT: [[META0]] = !{}
+;.
|
thurstond
reviewed
Jan 11, 2025
| @@ -29,6 +29,7 @@ class BoundsCheckingPass : public PassInfoMixin<BoundsCheckingPass> { | |||
| }; | |||
| std::optional<Runtime> Rt; // Trap if empty. | |||
| bool Merge = false; | |||
| std::optional<int8_t> GuardKind; // `allow_ubsan_check` argument. | |||
There was a problem hiding this comment.
Why allow a signed integer if it's intended to mirror SanitizerKind::SanitizerOrdinal, which is an enum based on uint64_t?
There was a problem hiding this comment.
because this is the type acceptable by allow_ubsan_check
There was a problem hiding this comment.
Should the allow_ubsan_check intrinsic be changed to use unsigned?
The other main usage in CodeGenFunction::EmitCheck is also using the signed type to store a non-negative number:
llvm::Value *Allow =
Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::allow_ubsan_check),
llvm::ConstantInt::get(CGM.Int8Ty, /* SanitizerHandler */ CheckHandler));
enum SanitizerHandler {
#define SANITIZER_CHECK(Enum, Name, Version) Enum,
LIST_SANITIZER_CHECKS
#undef SANITIZER_CHECK
};
There was a problem hiding this comment.
In llvm everything is signed, instructions on values can be signed/unsigned.
We can add more bits, but can do that when needed.
I copied type from ubsantrap() parameter, but at that time I didn't notice that ubsantrap
accepts "CheckHandler" which are not SanitizerKind.
Also uint64_t is unnecessary for SanitizerOrdinal, can be 0-127:
/// Number of array elements.
static constexpr unsigned kNumElem = 2;
/// Mask value initialized to 0.
uint64_t maskLoToHigh[kNumElem]{};
we should add static_assert there to modify ubsantrap if we need more bits.
Better with clang patch for code you mentioned.
There was a problem hiding this comment.
I see, thanks for the explanation!
Created using spr 1.3.4
thurstond
approved these changes
Jan 11, 2025
vitalybuka
deleted the
users/vitalybuka/spr/boundschecking-add-guard-pass-parameter
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/13989 Here is the relevant piece of the build log for the reference |
kazutakahirata
pushed a commit
to kazutakahirata/llvm-project
that referenced
this pull request
Jan 13, 2025
And use that as an argument for allow_ubsan_check when needed. Other ubsan checks use SanitizerKind, but those are known to the clang only. So make it a parameter in LLVM.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
And use that as an argument for
allow_ubsan_check when needed.
Other ubsan checks use SanitizerKind,
but those are known to the clang only.
So make it a parameter in LLVM.