Skip to content
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

Jump to bottom

[Sanitizer] add signed-integer-wrap sanitizer #80089

Closed
wants to merge 11 commits into from
Closed

[Sanitizer] add signed-integer-wrap sanitizer #80089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7774e40
add signed-integer-wrap sanitizer
JustinStitt Jan 23, 2024
7c959a1
add frontend tests
JustinStitt Jan 30, 2024
77732cb
add codegen tests
JustinStitt Jan 31, 2024
d9d7737
run clang-format
JustinStitt Jan 31, 2024
f00e6a3
check for overflow bit in codegen tests
JustinStitt Jan 31, 2024
01699c1
prefer CHECK{SIW|SIO} to CHECK
JustinStitt Jan 31, 2024
5497e8b
move wrap tests to integer-overflow.c
JustinStitt Feb 13, 2024
e964431
prefer hasOneOf() over multiple has()
JustinStitt Feb 13, 2024
e15b6e6
add docs for signed-integer-wrap
JustinStitt Feb 13, 2024
b978be6
update docs further
JustinStitt Feb 13, 2024
a9c4ef3
add entry to ReleaseNotes.rst
JustinStitt Feb 14, 2024
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
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,9 @@ Moved checkers
Sanitizers
----------

- Added ``-fsanitize=signed-integer-wrap`` to catch suspicious signed arithmetic
that results in wraparound caused by ``-fwrapv``.

Python Binding Changes
----------------------

Expand Down
14 changes: 11 additions & 3 deletions clang/docs/UndefinedBehaviorSanitizer.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,14 @@ Available checks are:
signed division overflow (``INT_MIN/-1``), but not checks for
lossy implicit conversions performed before the computation
(see ``-fsanitize=implicit-conversion``). Both of these two issues are
handled by ``-fsanitize=implicit-conversion`` group of checks.
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we refrase this differently?
Instead (or in addition) of reference to signed-integer-overflow, just explain that it will detect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? division looks like overflow to me?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually it's very inconsistent that the sanitizer is less strict without -fwrapv, when for signed-integer-overflow we have an opposite.

Copy link
Contributor Author

@JustinStitt JustinStitt Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@JustinStitt JustinStitt Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually it's very inconsistent that the sanitizer is less strict without -fwrapv, when for signed-integer-overflow we 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.

- ``-fsanitize=unreachable``: If control flow reaches an unreachable
program point.
- ``-fsanitize=unsigned-integer-overflow``: Unsigned integer overflow, where
Expand Down Expand Up @@ -233,8 +240,9 @@ You can also use the following check groups:
Enables ``signed-integer-overflow``, ``unsigned-integer-overflow``,
``shift``, ``integer-divide-by-zero``,
``implicit-unsigned-integer-truncation``,
``implicit-signed-integer-truncation``, and
``implicit-integer-sign-change``.
``implicit-signed-integer-truncation``,
``implicit-integer-sign-change``, and
``signed-integer-wrap``.
- ``-fsanitize=nullability``: Enables ``nullability-arg``,
``nullability-assign``, and ``nullability-return``. While violating
nullability does not have undefined behavior, it is often unintentional,
Expand Down
5 changes: 3 additions & 2 deletions clang/include/clang/Basic/Sanitizers.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
62 changes: 49 additions & 13 deletions clang/lib/CodeGen/CGExprScalar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -2516,6 +2521,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))
Expand Down Expand Up @@ -3409,15 +3420,16 @@ 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),
SanitizerKind::IntegerDivideByZero));
}

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()) {
Expand All @@ -3430,8 +3442,12 @@ 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)
Expand All @@ -3441,8 +3457,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));
Expand Down Expand Up @@ -3490,8 +3507,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);
Expand Down Expand Up @@ -3553,12 +3571,20 @@ 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.hasOneOf(SanitizerKind::SignedIntegerOverflow |
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);
Expand Down Expand Up @@ -3861,6 +3887,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))
Expand Down Expand Up @@ -4015,6 +4046,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))
Expand Down
32 changes: 23 additions & 9 deletions clang/test/CodeGen/integer-overflow.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - -ftrapv | FileCheck %s --check-prefix=TRAPV
// RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - -fsanitize=signed-integer-overflow | FileCheck %s --check-prefix=CATCH_UB
// RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - -ftrapv -ftrapv-handler foo | FileCheck %s --check-prefix=TRAPV_HANDLER
// RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - -fwrapv -fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CATCH_WRAP



// Tests for signed integer overflow stuff.
Expand All @@ -11,65 +13,73 @@ void test1(void) {
// WRAPV-LABEL: define{{.*}} void @test1
// TRAPV-LABEL: define{{.*}} void @test1
extern volatile int f11G, a, b;

// DEFAULT: add nsw i32
// WRAPV: add i32
// TRAPV: llvm.sadd.with.overflow.i32
// CATCH_UB: llvm.sadd.with.overflow.i32
// CATCH_WRAP: llvm.sadd.with.overflow.i32
// TRAPV_HANDLER: foo(
f11G = a + b;

// DEFAULT: sub nsw i32
// WRAPV: sub i32
// TRAPV: llvm.ssub.with.overflow.i32
// CATCH_UB: llvm.ssub.with.overflow.i32
// CATCH_WRAP: llvm.ssub.with.overflow.i32
// TRAPV_HANDLER: foo(
f11G = a - b;

// DEFAULT: mul nsw i32
// WRAPV: mul i32
// TRAPV: llvm.smul.with.overflow.i32
// CATCH_UB: llvm.smul.with.overflow.i32
// CATCH_WRAP: llvm.smul.with.overflow.i32
// TRAPV_HANDLER: foo(
f11G = a * b;

// DEFAULT: sub nsw i32 0,
// WRAPV: sub i32 0,
// DEFAULT: sub nsw i32 0,
// WRAPV: sub i32 0,
// TRAPV: llvm.ssub.with.overflow.i32(i32 0
// CATCH_UB: llvm.ssub.with.overflow.i32(i32 0
// CATCH_WRAP: llvm.ssub.with.overflow.i32(i32 0
// TRAPV_HANDLER: foo(
f11G = -a;

// PR7426 - Overflow checking for increments.

// DEFAULT: add nsw i32 {{.*}}, 1
// WRAPV: add i32 {{.*}}, 1
// TRAPV: llvm.sadd.with.overflow.i32({{.*}}, i32 1)
// CATCH_UB: llvm.sadd.with.overflow.i32({{.*}}, i32 1)
// CATCH_WRAP: llvm.sadd.with.overflow.i32({{.*}}, i32 1)
// TRAPV_HANDLER: foo(
++a;

// DEFAULT: add nsw i32 {{.*}}, -1
// WRAPV: add i32 {{.*}}, -1
// TRAPV: llvm.ssub.with.overflow.i32({{.*}}, i32 1)
// CATCH_UB: llvm.ssub.with.overflow.i32({{.*}}, i32 1)
// CATCH_WRAP: llvm.ssub.with.overflow.i32({{.*}}, i32 1)
// TRAPV_HANDLER: foo(
--a;

// -fwrapv should turn off inbounds for GEP's, PR9256
extern int* P;
++P;
// DEFAULT: getelementptr inbounds i32, ptr
// WRAPV: getelementptr i32, ptr
// TRAPV: getelementptr inbounds i32, ptr
// CATCH_UB: getelementptr inbounds i32, ptr
// CATCH_WRAP: getelementptr i32, ptr

// PR9350: char pre-increment never overflows.
extern volatile signed char PR9350_char_inc;
// DEFAULT: add i8 {{.*}}, 1
// WRAPV: add i8 {{.*}}, 1
// TRAPV: add i8 {{.*}}, 1
// CATCH_UB: add i8 {{.*}}, 1
// CATCH_WRAP: add i8 {{.*}}, 1
++PR9350_char_inc;

// PR9350: char pre-decrement never overflows.
Expand All @@ -78,6 +88,7 @@ void test1(void) {
// WRAPV: add i8 {{.*}}, -1
// TRAPV: add i8 {{.*}}, -1
// CATCH_UB: add i8 {{.*}}, -1
// CATCH_WRAP: add i8 {{.*}}, -1
--PR9350_char_dec;

// PR9350: short pre-increment never overflows.
Expand All @@ -86,6 +97,7 @@ void test1(void) {
// WRAPV: add i16 {{.*}}, 1
// TRAPV: add i16 {{.*}}, 1
// CATCH_UB: add i16 {{.*}}, 1
// CATCH_WRAP: add i16 {{.*}}, 1
++PR9350_short_inc;

// PR9350: short pre-decrement never overflows.
Expand All @@ -94,6 +106,7 @@ void test1(void) {
// WRAPV: add i16 {{.*}}, -1
// TRAPV: add i16 {{.*}}, -1
// CATCH_UB: add i16 {{.*}}, -1
// CATCH_WRAP: add i16 {{.*}}, -1
--PR9350_short_dec;

// PR24256: don't instrument __builtin_frame_address.
Expand All @@ -102,4 +115,5 @@ void test1(void) {
// WRAPV: call ptr @llvm.frameaddress.p0(i32 0)
// TRAPV: call ptr @llvm.frameaddress.p0(i32 0)
// CATCH_UB: call ptr @llvm.frameaddress.p0(i32 0)
// CATCH_WRAP: call ptr @llvm.frameaddress.p0(i32 0)
}
Loading
Loading