[Sanitizer] add signed-integer-wrap sanitizer #80089
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-compiler-rt-sanitizer Author: Justin Stitt (JustinStitt) ChangesReasoning The kernel enables In short, we need a way to instrument signed arithmetic even when the overflow behavior is technically defined due to * it seems division and modulo arithmetic is still instrumented with Other Notes
cc: @nickdesaulniers @kees @nathanchance @bwendling Patch is 28.27 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/80089.diff 5 Files Affected:
diff --git a/clang/include/clang/Basic/Sanitizers.def b/clang/include/clang/Basic/Sanitizers.def
index c2137e3f61f64..b987b26f93c39 100644
--- a/clang/include/clang/Basic/Sanitizers.def
+++ b/clang/include/clang/Basic/Sanitizers.def
@@ -104,6 +104,7 @@ SANITIZER("shift-base", ShiftBase)
SANITIZER("shift-exponent", ShiftExponent)
SANITIZER_GROUP("shift", Shift, ShiftBase | ShiftExponent)
SANITIZER("signed-integer-overflow", SignedIntegerOverflow)
+SANITIZER("signed-integer-wrap", SignedIntegerWrap)
SANITIZER("unreachable", Unreachable)
SANITIZER("vla-bound", VLABound)
SANITIZER("vptr", Vptr)
@@ -144,7 +145,7 @@ SANITIZER_GROUP("undefined", Undefined,
IntegerDivideByZero | NonnullAttribute | Null | ObjectSize |
PointerOverflow | Return | ReturnsNonnullAttribute | Shift |
SignedIntegerOverflow | Unreachable | VLABound | Function |
- Vptr)
+ Vptr | SignedIntegerWrap)
// -fsanitize=undefined-trap is an alias for -fsanitize=undefined.
SANITIZER_GROUP("undefined-trap", UndefinedTrap, Undefined)
@@ -179,7 +180,7 @@ SANITIZER_GROUP("implicit-conversion", ImplicitConversion,
SANITIZER_GROUP("integer", Integer,
ImplicitConversion | IntegerDivideByZero | Shift |
SignedIntegerOverflow | UnsignedIntegerOverflow |
- UnsignedShiftBase)
+ UnsignedShiftBase | SignedIntegerWrap)
SANITIZER("local-bounds", LocalBounds)
SANITIZER_GROUP("bounds", Bounds, ArrayBounds | LocalBounds)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 5502f685f6474..74e016fe4899f 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -723,6 +723,11 @@ class ScalarExprEmitter
if (Ops.Ty->isSignedIntegerOrEnumerationType()) {
switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
case LangOptions::SOB_Defined:
+ if (CGF.SanOpts.has(SanitizerKind::SignedIntegerWrap)) {
+ if (CanElideOverflowCheck(CGF.getContext(), Ops))
+ return Builder.CreateNSWMul(Ops.LHS, Ops.RHS, "mul");
+ return EmitOverflowCheckedBinOp(Ops);
+ }
return Builder.CreateMul(Ops.LHS, Ops.RHS, "mul");
case LangOptions::SOB_Undefined:
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
@@ -2517,6 +2522,12 @@ llvm::Value *ScalarExprEmitter::EmitIncDecConsiderOverflowBehavior(
StringRef Name = IsInc ? "inc" : "dec";
switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
case LangOptions::SOB_Defined:
+ if (CGF.SanOpts.has(SanitizerKind::SignedIntegerWrap)) {
+ if (!E->canOverflow())
+ return Builder.CreateNSWAdd(InVal, Amount, Name);
+ return EmitOverflowCheckedBinOp(createBinOpInfoFromIncDec(
+ E, InVal, IsInc, E->getFPFeaturesInEffect(CGF.getLangOpts())));
+ }
return Builder.CreateAdd(InVal, Amount, Name);
case LangOptions::SOB_Undefined:
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
@@ -3410,7 +3421,7 @@ Value *ScalarExprEmitter::EmitCompoundAssign(const CompoundAssignOperator *E,
void ScalarExprEmitter::EmitUndefinedBehaviorIntegerDivAndRemCheck(
const BinOpInfo &Ops, llvm::Value *Zero, bool isDiv) {
- SmallVector<std::pair<llvm::Value *, SanitizerMask>, 2> Checks;
+ SmallVector<std::pair<llvm::Value *, SanitizerMask>, 3> Checks;
if (CGF.SanOpts.has(SanitizerKind::IntegerDivideByZero)) {
Checks.push_back(std::make_pair(Builder.CreateICmpNE(Ops.RHS, Zero),
@@ -3418,7 +3429,8 @@ void ScalarExprEmitter::EmitUndefinedBehaviorIntegerDivAndRemCheck(
}
const auto *BO = cast<BinaryOperator>(Ops.E);
- if (CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow) &&
+ if (CGF.SanOpts.hasOneOf(SanitizerKind::SignedIntegerOverflow |
+ SanitizerKind::SignedIntegerWrap) &&
Ops.Ty->hasSignedIntegerRepresentation() &&
!IsWidenedIntegerOp(CGF.getContext(), BO->getLHS()) &&
Ops.mayHaveIntegerOverflow()) {
@@ -3431,8 +3443,13 @@ void ScalarExprEmitter::EmitUndefinedBehaviorIntegerDivAndRemCheck(
llvm::Value *LHSCmp = Builder.CreateICmpNE(Ops.LHS, IntMin);
llvm::Value *RHSCmp = Builder.CreateICmpNE(Ops.RHS, NegOne);
llvm::Value *NotOverflow = Builder.CreateOr(LHSCmp, RHSCmp, "or");
- Checks.push_back(
- std::make_pair(NotOverflow, SanitizerKind::SignedIntegerOverflow));
+ if (CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
+ Checks.push_back(
+ std::make_pair(NotOverflow, SanitizerKind::SignedIntegerOverflow));
+ if (CGF.SanOpts.has(SanitizerKind::SignedIntegerWrap))
+ Checks.push_back(
+ std::make_pair(NotOverflow, SanitizerKind::SignedIntegerWrap));
+
}
if (Checks.size() > 0)
@@ -3442,8 +3459,9 @@ void ScalarExprEmitter::EmitUndefinedBehaviorIntegerDivAndRemCheck(
Value *ScalarExprEmitter::EmitDiv(const BinOpInfo &Ops) {
{
CodeGenFunction::SanitizerScope SanScope(&CGF);
- if ((CGF.SanOpts.has(SanitizerKind::IntegerDivideByZero) ||
- CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)) &&
+ if (CGF.SanOpts.hasOneOf(SanitizerKind::IntegerDivideByZero |
+ SanitizerKind::SignedIntegerOverflow |
+ SanitizerKind::SignedIntegerWrap) &&
Ops.Ty->isIntegerType() &&
(Ops.mayHaveIntegerDivisionByZero() || Ops.mayHaveIntegerOverflow())) {
llvm::Value *Zero = llvm::Constant::getNullValue(ConvertType(Ops.Ty));
@@ -3491,8 +3509,9 @@ Value *ScalarExprEmitter::EmitDiv(const BinOpInfo &Ops) {
Value *ScalarExprEmitter::EmitRem(const BinOpInfo &Ops) {
// Rem in C can't be a floating point type: C99 6.5.5p2.
- if ((CGF.SanOpts.has(SanitizerKind::IntegerDivideByZero) ||
- CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)) &&
+ if (CGF.SanOpts.hasOneOf(SanitizerKind::IntegerDivideByZero |
+ SanitizerKind::SignedIntegerOverflow |
+ SanitizerKind::SignedIntegerWrap) &&
Ops.Ty->isIntegerType() &&
(Ops.mayHaveIntegerDivisionByZero() || Ops.mayHaveIntegerOverflow())) {
CodeGenFunction::SanitizerScope SanScope(&CGF);
@@ -3554,12 +3573,19 @@ Value *ScalarExprEmitter::EmitOverflowCheckedBinOp(const BinOpInfo &Ops) {
const std::string *handlerName =
&CGF.getLangOpts().OverflowHandler;
if (handlerName->empty()) {
- // If the signed-integer-overflow sanitizer is enabled, emit a call to its
- // runtime. Otherwise, this is a -ftrapv check, so just emit a trap.
- if (!isSigned || CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)) {
+ // If the signed-integer-overflow or signed-integer-wrap sanitizer is
+ // enabled, emit a call to its runtime. Otherwise, this is a -ftrapv check,
+ // so just emit a trap.
+ if (!isSigned || CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow)
+ || CGF.SanOpts.has(SanitizerKind::SignedIntegerWrap)) {
llvm::Value *NotOverflow = Builder.CreateNot(overflow);
- SanitizerMask Kind = isSigned ? SanitizerKind::SignedIntegerOverflow
- : SanitizerKind::UnsignedIntegerOverflow;
+
+ SanitizerMask Kind = SanitizerKind::UnsignedIntegerOverflow;
+ if (isSigned)
+ Kind = CGF.getLangOpts().getSignedOverflowBehavior() ==
+ LangOptions::SOB_Defined ? SanitizerKind::SignedIntegerWrap :
+ SanitizerKind::SignedIntegerOverflow;
+
EmitBinOpCheck(std::make_pair(NotOverflow, Kind), Ops);
} else
CGF.EmitTrapCheck(Builder.CreateNot(overflow), OverflowKind);
@@ -3862,6 +3888,11 @@ Value *ScalarExprEmitter::EmitAdd(const BinOpInfo &op) {
if (op.Ty->isSignedIntegerOrEnumerationType()) {
switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
case LangOptions::SOB_Defined:
+ if (CGF.SanOpts.has(SanitizerKind::SignedIntegerWrap)) {
+ if (CanElideOverflowCheck(CGF.getContext(), op))
+ return Builder.CreateNSWAdd(op.LHS, op.RHS, "add");
+ return EmitOverflowCheckedBinOp(op);
+ }
return Builder.CreateAdd(op.LHS, op.RHS, "add");
case LangOptions::SOB_Undefined:
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
@@ -4016,6 +4047,11 @@ Value *ScalarExprEmitter::EmitSub(const BinOpInfo &op) {
if (op.Ty->isSignedIntegerOrEnumerationType()) {
switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
case LangOptions::SOB_Defined:
+ if (CGF.SanOpts.has(SanitizerKind::SignedIntegerWrap)) {
+ if (CanElideOverflowCheck(CGF.getContext(), op))
+ return Builder.CreateNSWSub(op.LHS, op.RHS, "sub");
+ return EmitOverflowCheckedBinOp(op);
+ }
return Builder.CreateSub(op.LHS, op.RHS, "sub");
case LangOptions::SOB_Undefined:
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
diff --git a/clang/test/CodeGen/integer-wrap.c b/clang/test/CodeGen/integer-wrap.c
new file mode 100644
index 0000000000000..b0a99e9becaa8
--- /dev/null
+++ b/clang/test/CodeGen/integer-wrap.c
@@ -0,0 +1,66 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s -fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECK
+
+// Check that -fsanitize=signed-integer-overflow doesn't instrument with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s -fsanitize=signed-integer-overflow | FileCheck %s --check-prefix=CHECKSIO
+
+extern volatile int a, b, c;
+
+// CHECK-LABEL: define void @test_add_overflow
+void test_add_overflow(void) {
+ // CHECK: [[ADD0:%.*]] = load {{.*}} i32
+ // CHECK-NEXT: [[ADD1:%.*]] = load {{.*}} i32
+ // CHECK-NEXT: {{%.*}} = call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[ADD0]], i32 [[ADD1]])
+ // CHECK: call void @__ubsan_handle_add_overflow
+
+ // CHECKSIO-NOT: call void @__ubsan_handle_add_overflow
+ a = b + c;
+}
+
+// CHECK-LABEL: define void @test_inc_overflow
+void test_inc_overflow(void) {
+ // This decays and gets handled by __ubsan_handle_add_overflow...
+ // CHECK: [[INC0:%.*]] = load {{.*}} i32
+ // CHECK-NEXT: call { i32, i1 } @llvm.sadd.with.overflow.i32(i32 [[INC0]], i32 1)
+ // CHECK: br {{.*}} %handler.add_overflow
+
+ // CHECKSIO-NOT: br {{.*}} %handler.add_overflow
+ ++a;
+ a++;
+}
+
+// CHECK-LABEL: define void @test_sub_overflow
+void test_sub_overflow(void) {
+ // CHECK: [[SUB0:%.*]] = load {{.*}} i32
+ // CHECK-NEXT: [[SUB1:%.*]] = load {{.*}} i32
+ // CHECK-NEXT: call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 [[SUB0]], i32 [[SUB1]])
+ // CHECK: call void @__ubsan_handle_sub_overflow
+
+ // CHECKSIO-NOT: call void @__ubsan_handle_sub_overflow
+ a = b - c;
+}
+
+// CHECK-LABEL: define void @test_mul_overflow
+void test_mul_overflow(void) {
+ // CHECK: [[MUL0:%.*]] = load {{.*}} i32
+ // CHECK-NEXT: [[MUL1:%.*]] = load {{.*}} i32
+ // CHECK-NEXT: call { i32, i1 } @llvm.smul.with.overflow.i32(i32 [[MUL0]], i32 [[MUL1]])
+ // CHECK: call void @__ubsan_handle_mul_overflow
+
+ // CHECKSIO-NOT: call void @__ubsan_handle_mul_overflow
+ a = b * c;
+}
+
+// CHECK-LABEL: define void @test_div_overflow
+void test_div_overflow(void) {
+ // CHECK: [[DIV0:%.*]] = load {{.*}} i32
+ // CHECK-NEXT: [[DIV1:%.*]] = load {{.*}} i32
+ // CHECK-NEXT: [[DIV2:%.*]] = icmp ne i32 [[DIV0]], -2147483648
+ // CHECK-NEXT: [[DIV3:%.*]] = icmp ne i32 [[DIV1]], -1
+ // CHECK-NEXT: [[DIVOR:%or]] = or i1 [[DIV2]], [[DIV3]]
+ // CHECK-NEXT: br {{.*}} %handler.divrem_overflow
+
+ // -fsanitize=signed-integer-overflow still instruments division even with -fwrapv
+ // CHECKSIO: br {{.*}} %handler.divrem_overflow
+ a = b / c;
+}
diff --git a/clang/test/Driver/fsanitize.c b/clang/test/Driver/fsanitize.c
index 1671825042c32..86172febf4723 100644
--- a/clang/test/Driver/fsanitize.c
+++ b/clang/test/Driver/fsanitize.c
@@ -1,19 +1,21 @@
// RUN: %clang --target=x86_64-linux-gnu -fsanitize=undefined -fsanitize-trap=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang --target=x86_64-linux-gnu -fsanitize=undefined -fsanitize-trap=undefined -fno-sanitize-trap=signed-integer-overflow %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP2
+// RUN: %clang --target=x86_64-linux-gnu -fsanitize=undefined -fsanitize-trap=undefined -fno-sanitize-trap=signed-integer-wrap %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP3
// RUN: %clang --target=x86_64-linux-gnu -fsanitize=undefined -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang --target=x86_64-linux-gnu -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang --target=x86_64-linux-gnu -fsanitize-undefined-trap-on-error -fsanitize=undefined-trap %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// RUN: %clang --target=x86_64-linux-gnu -fsanitize-trap -fsanitize=undefined-trap %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
// CHECK-UNDEFINED-TRAP-NOT: -fsanitize-recover
-// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function),?){18}"}}
-// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
-// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|signed-integer-wrap|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function),?){19}"}}
+// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,signed-integer-wrap,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-wrap,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP3: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
// RUN: %clang --target=x86_64-linux-gnu -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED
-// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){19}"}}
+// CHECK-UNDEFINED: "-fsanitize={{((signed-integer-overflow|signed-integer-wrap|integer-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|vptr|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){20}"}}
// RUN: %clang --target=x86_64-apple-darwin10 -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-DARWIN
-// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){18}"}}
+// CHECK-UNDEFINED-DARWIN: "-fsanitize={{((signed-integer-overflow|signed-integer-wrap|integer-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){19}"}}
// RUN: %clang --target=i386-pc-win32 -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-UNDEFINED-WIN32,CHECK-UNDEFINED-MSVC
// RUN: %clang --target=i386-pc-win32 -fsanitize=undefined -x c++ %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-UNDEFINED-WIN32,CHECK-UNDEFINED-WIN-CXX,CHECK-UNDEFINED-MSVC
@@ -24,8 +26,8 @@
// CHECK-UNDEFINED-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone{{(-x86_64)?}}.lib"
// CHECK-UNDEFINED-WIN64-MINGW: "--dependent-lib={{[^"]*}}libclang_rt.ubsan_standalone{{(-x86_64)?}}.a"
// CHECK-UNDEFINED-WIN-CXX: "--dependent-lib={{[^"]*}}ubsan_standalone_cxx{{[^"]*}}.lib"
-// CHECK-UNDEFINED-MSVC-SAME: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function),?){18}"}}
-// CHECK-UNDEFINED-WIN64-MINGW-SAME: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function|vptr),?){19}"}}
+// CHECK-UNDEFINED-MSVC-SAME: "-fsanitize={{((signed-integer-overflow|signed-integer-wrap|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function),?){19}"}}
+// CHECK-UNDEFINED-WIN64-MINGW-SAME: "-fsanitize={{((signed-integer-overflow|signed-integer-wrap|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function|vptr),?){20}"}}
// RUN: %clang --target=i386-pc-win32 -fsanitize-coverage=bb %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-COVERAGE-WIN32
// CHECK-COVERAGE-WIN32: "--dependent-lib={{[^"]*}}ubsan_standalone{{(-i386)?}}.lib"
@@ -33,7 +35,7 @@
// CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone{{(-x86_64)?}}.lib"
// RUN: %clang --target=%itanium_abi_triple -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change|unsigned-shift-base),?){9}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|signed-integer-wrap|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change|unsigned-shift-base),?){10}"}}
// RUN: %clang -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
// RUN: %clang -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
@@ -90,7 +92,7 @@
// CHECK-FNO-SANITIZE-ALL: "-fsanitize=thread"
// RUN: %clang --target=x86_64-linux-gnu -fsanitize=thread,undefined -fno-sanitize=thread -fno-sanitize=float-cast-overflow,vptr,bool,builtin,enum %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-PARTIAL-UNDEFINED
-// CHECK-PARTIAL-UNDEFINED: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|function|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|array-bounds|returns-nonnull-attribute|nonnull-attribute),?){14}"}...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
https://clang.llvm.org/docs/ClangFormat.html#git-integration tells you how to narrow it down to just your changes. Once that's setup you can use the command from the comment the bot left. Sometimes it will try to format too much, in which case it's fine to ignore it, but looking at what the bot reported here that's not the case. |
I usually do |
|
Hey, does anyone know why the CI bot (buildkite) fails my builds for a trailing whitespace in a file I did not touch? It says there's some trailing whitespace in some documentation files that my PR doesn't touch (unless I'm misinterpreting its output): edit: resolved. the build bot just builds the tree as it appears and doesn't patch my changeset onto a known working tree; the red build isn't my fault -- right? |
That's mostly just bad luck. It's been fixed now, though, in commit 0217d2e so it should pass correctly now. |
|
ping! |
|
I've tested this with the Linux kernel, and it is working as expected. It is ready and waiting for this option to gain back a bunch of sanitizer coverage for CIs and likely in production kernels. :) |
There was a problem hiding this comment.
PATCH mostly LGTM
UBSan is documented clang/docs/UndefinedBehaviorSanitizer.rst https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
Please make sure to update the list of available checks.
Please also update the release notes to mention this feature.
clang/docs/ReleaseNotes.rst
The kernel enables -fno-strict-overflow which implies -fwrapv.
While Linux is the only kernel I've contributed to, it's important to be explicit as to which kernel you're referring to. 😛
and likely in production kernels. :)
I doubt you meant running in production. 😛
I very much do -- I expect to use this like we use the array-bounds sanitizer, which is on in production in at least Ubuntu and Android. It may be a long road to getting sane coverage without endless false positives, of course. But the intent is to use it in production. |
JustinStitt
force-pushed
the
pr/signed-integer-wrap
branch
from
f790b65 to
01699c1
Compare
There was a problem hiding this comment.
I think this patch is good to go. Thanks for working on it!
cc @AaronBallman @efriedma-quic @rjmccall @nikic in case there's any last minute concerns. I'm happy to sign off on this, but consider giving folks some time to speak up before landing this.
Thanks for the review Nick! BTW, I forgot to update the |
No concerns from me, thanks! |
|
Why not just enforce -fsanitize=signed-integer-overflow with -fwrapv? I suspect it's just overlook, and not intentional behavior. |
+1 We should consider this direction |
The UB-vs-non-UB seemed to be a really specific goal in the existing code. i.e. that the sanitizer was disabled didn't look like an accident. For people using this to find only UB, this would be a behavioral change, so to me it seems like a separate name makes the most sense. Anyone wanting wrap-around checking can use -wrap, and anyone wanting UB checking can use -overflow. |
Isn't this still UB even with -fwrapv? UB is a language feature, not compiler. |
Or maybe I am missing the bigger picture? Is there a plan fix signed-integer-overflow for -fwrapv as well? |
|
Right. |
|
This is a UI discussion about how command line options should behave. A -fwrapv user may either:
Our -fsanitize=signed-integer-overflow design have been assuming that -fwrapv users don't need the check. https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html is clear that not all checks are undefined behavior in the standards.
Sure -fwrapv makes wraparound defined, but it doesn't prevent us from making -fsanitize=signed-integer-overflow useful. "-fwrapv => no signed-integer-overflow" is not a solid argument. I think we can try making -fsanitize=signed-integer-overflow effective even when -fwrapv if specified. There is a precedent that -fsanitize=undefined enables different checks for different targets. |
In earlier GCC discussions, it seemed very much like the |
|
My original idea was to get the SIO sanitizer working with case LangOptions::SOB_Undefined:
if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))... I think the best option is to implement a wrap sanitizer (which this PR does). A wrap sanitizer best captures the language semantics at hand while maintaining existing functionality of the SIO sanitizer. I think Kees can speak first hand about how picky some folks are about the language being used for this arithmetic overflow/wraparound stuff (he linked some gcc threads above my comment but I've also seen some spicy LKML discussions about how OVERFLOW doesn't exist in the Linux Kernel and as such WRAPAROUND instrumentation is needed). I think this PR bridges the gap between folks like Kees (who just want all this suspicious kernel arithmetic to go away) and folks like Linus (who is really particular about language). |
|
GCC folks have not answered. Adding -wrap keeps the behavior for -overflow the same between GCC and Clang. Can we please move this forward and land it as is? We can trivially change this in the future if we need to. |
I believe we can move forward by reusing |
I see a few problems with changing
Should a compiler not at least put some effort into properly representing the semantics at hand? @MaskRay, there's been lots of good review on this PR with folks more or less liking the direction of it. I'd like to find some common ground on this so we can move it forward. If you really think changing the SIO sanitizer is the way to go I'll probably close this PR and open a new one as it represents a wholly different idea. |
| handled by ``-fsanitize=implicit-conversion`` group of checks. Note that | ||
| ``-fwrapv`` implicitly disables instrumentation for much of the arithmetic | ||
| covered by ``-fsanitize=signed-integer-overflow``. | ||
| - ``-fsanitize=signed-integer-wrap``: Signed Integer wraparound, where the |
There was a problem hiding this comment.
Can we refrase this differently?
Instead (or in addition) of reference to signed-integer-overflow, just explain that it will detect?
There was a problem hiding this comment.
I agree that the current version is not so clear. I must admit, though, I am not 100% sure what changes you would like. Vitaly, do you think it should be worded like such:
Note that ``-fwrapv`` implicitly disables most checks performed by this sanitizer
as the overflow behavior is well-defined.
If this doesn't match what you were thinking could you let me know?
| result of a signed integer computation wraps around. Behaves identically | ||
| to ``-fsanitize=signed-integer-overflow`` when ``-fwrapv`` is enabled. | ||
| Without ``-fwrapv`` or ``-fno-strict-overflow``, this sanitizer will only | ||
| instrument division operations. |
There was a problem hiding this comment.
Why? division looks like overflow to me?
There was a problem hiding this comment.
Actually it's very inconsistent that the sanitizer is less strict without -fwrapv, when for signed-integer-overflow we have an opposite.
There was a problem hiding this comment.
I was going off the spec:
An implementation that defines signed integer types as also being modulo need not detect integer overflow, in which case, only integer divide-by-zero need be detected.
From H.2.2 Integer Types
I initially read this as meaning we need to instrument division no matter what (just in case it's divide by zero or similar case). What is your interpretation?
I can add a check for the signed overflow behavior for the division steps -- if needed.
There was a problem hiding this comment.
Actually it's very inconsistent that the sanitizer is less strict without
-fwrapv, when forsigned-integer-overflowwe have an opposite.
Yes, this proposed "wrap" sanitizer is less strict when nothing is defined as wrapping (i.e: missing -fwrapv). Should this not be the case?
In the same way, signed-integer-overflow is more strict without -fwrapv as things are overflowing and not wrapping.
While I understand adding a new way (to do similar things instead of improving the existing feature) can be tempting, as it avoids "disrupting" current users, I feel that "not breaking existing users" is sometimes exaggerated to argue for not improving an existing feature. I noticed in CodeSearch that this BUILD.gn seems used quite a bit. The author seems aware that with overflow checks, -fwrapv would not make sense.
If there are semantics improvement, unrelated to "let's not break users for -fwrapv", I think a new mode is still worth considering.
I agree that a new PR could be useful. |
|
With PR #82432 landed, this PR is redundant. Thanks for changing the option name! Closing... |
…w if -fwrapv (#85501) Linux kernel uses -fwrapv to change signed integer overflows from undefined behaviors to defined behaviors. However, the security folks still want -fsanitize=signed-integer-overflow diagnostics. Their intention can be expressed with -fwrapv -fsanitize=signed-integer-overflow (#80089). This mode by default reports recoverable errors while still making signed integer overflows defined (most UBSan checks are recoverable by default: you get errors in stderr, but the program is not halted). -fsanitize=undefined -fwrapv users likely want to suppress signed-integer-overflow, unless signed-integer-overflow is explicitly enabled. Implement this suppression.
…w if -fwrapv (llvm#85501) Linux kernel uses -fwrapv to change signed integer overflows from undefined behaviors to defined behaviors. However, the security folks still want -fsanitize=signed-integer-overflow diagnostics. Their intention can be expressed with -fwrapv -fsanitize=signed-integer-overflow (llvm#80089). This mode by default reports recoverable errors while still making signed integer overflows defined (most UBSan checks are recoverable by default: you get errors in stderr, but the program is not halted). -fsanitize=undefined -fwrapv users likely want to suppress signed-integer-overflow, unless signed-integer-overflow is explicitly enabled. Implement this suppression.
Reasoning
Clang has a
signed-integer-overflowsanitizer to catch arithmetic overflow; however, most* of its instrumentation fails to apply when-fwrapvis enabled.The Linux kernel enables
-fno-strict-overflowwhich implies-fwrapv. This means we are currently unable to detect signed-integer wrap-around. All the while, the root cause of many security vulnerabilities in the Linux kernel is arithmetic overflowIn short, we need a way to instrument signed arithmetic even when the overflow behavior is technically defined due to
-fwrapv. I propose we name the sanitizer "signed-integer-wrap" as it best captures the language semantics at hand.* it seems division and modulo arithmetic is still instrumented with
-fsanitize=signed-integer-overflowand-fwrapvOther Notes
The
unsigned-integer-overflowsanitizer is probably named incorrectly because "a computation involving unsigned operands can never overflow" 1.Perhaps this sanitizer does not belong in the Undefined Behavior Sanitizer as wrap-around is not UB (although the instrumentation is exactly the same).
I tried running clang-format but the diff was huge and beyond just my changes.(fixed in 46ef570)cc: @nickdesaulniers @kees @nathanchance @bwendling