Skip to content

[BoundsChecking] Add guard= pass parameter #122575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 13, 2025
Merged

[BoundsChecking] Add guard= pass parameter #122575

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f879a22
[𝘀𝗽𝗿] initial version
vitalybuka Jan 11, 2025
8a11b0b
RT-GUARDRT fix
vitalybuka Jan 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions llvm/include/llvm/Transforms/Instrumentation/BoundsChecking.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@thurstond thurstond Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why allow a signed integer if it's intended to mirror SanitizerKind::SanitizerOrdinal, which is an enum based on uint64_t?

Copy link
Collaborator Author

@vitalybuka vitalybuka Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because this is the type acceptable by allow_ubsan_check

Copy link
Contributor

@thurstond thurstond Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
};

Copy link
Collaborator Author

@vitalybuka vitalybuka Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@thurstond thurstond Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for the explanation!

};

BoundsCheckingPass(Options Opts) : Opts(Opts) {}
Expand Down
16 changes: 12 additions & 4 deletions llvm/lib/Passes/PassBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
11 changes: 10 additions & 1 deletion llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -299,5 +306,7 @@ void BoundsCheckingPass::printPipeline(
}
if (Opts.Merge)
OS << ";merge";
if (Opts.GuardKind)
OS << ";guard=" << static_cast<int>(*Opts.GuardKind);
OS << ">";
}
51 changes: 51 additions & 0 deletions llvm/test/Instrumentation/BoundsChecking/runtimes.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]] = !{}
Expand All @@ -168,3 +215,7 @@ define void @f1(i64 %x) nounwind {
;.
; MINRTABORT-NOMERGE: [[META0]] = !{}
;.
; TR-GUARD: [[META0]] = !{}
;.
; RT-GUARDRT: [[META0]] = !{}
;.
Loading