-
Notifications
You must be signed in to change notification settings - Fork 15.7k
[analyzer] Fix BuiltingFunctionChecker crash on large types #174335
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.
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
[analyzer] Fix BuiltingFunctionChecker crash on large types #174335
Conversation
Previously, if the result type was 'large' (at least 65 bits), then the ASTContext::getIntTypeForBitwidth would return an empty QualType, causing later a crash when we assume it's non-empty. Instead of using this API, we could piggyback on the BigInt type to formulate a "large enough" type for calculating the mathematically correct result for the operation to check against. Fixes llvm#173795 rdar://166709144
|
@llvm/pr-subscribers-clang Author: Balázs Benics (steakhal) ChangesPreviously, if the result type was 'large' (at least 65 bits), then the ASTContext::getIntTypeForBitwidth would return an empty QualType, causing later a crash when we assume it's non-empty. Instead of using this API, we could piggyback on the BigInt type to formulate a "large enough" type for calculating the mathematically correct result for the operation to check against. Fixes #173795 rdar://166709144 Full diff: https://github.com/llvm/llvm-project/pull/174335.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index 8eb68b1fa638e..f50a3b08671e2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -36,9 +36,8 @@ QualType getSufficientTypeForOverflowOp(CheckerContext &C, const QualType &T) {
assert(T->isIntegerType());
ASTContext &ACtx = C.getASTContext();
-
unsigned BitWidth = ACtx.getIntWidth(T);
- return ACtx.getIntTypeForBitwidth(BitWidth * 2, T->isSignedIntegerType());
+ return ACtx.getBitIntType(T->isUnsignedIntegerType(), BitWidth * 2);
}
QualType getOverflowBuiltinResultType(const CallEvent &Call) {
@@ -208,8 +207,10 @@ void BuiltinFunctionChecker::handleOverflowBuiltin(const CallEvent &Call,
SVal Arg1 = Call.getArgSVal(0);
SVal Arg2 = Call.getArgSVal(1);
- SVal RetValMax = SVB.evalBinOp(State, Op, Arg1, Arg2,
- getSufficientTypeForOverflowOp(C, ResultType));
+ QualType SufficientlyWideTy = getSufficientTypeForOverflowOp(C, ResultType);
+ assert(!SufficientlyWideTy.isNull());
+
+ SVal RetValMax = SVB.evalBinOp(State, Op, Arg1, Arg2, SufficientlyWideTy);
SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
auto [Overflow, NotOverflow] = checkOverflow(C, RetValMax, ResultType);
diff --git a/clang/test/Analysis/builtin_overflow.c b/clang/test/Analysis/builtin_overflow.c
index d290333071dc9..ba7e52703fefe 100644
--- a/clang/test/Analysis/builtin_overflow.c
+++ b/clang/test/Analysis/builtin_overflow.c
@@ -1,11 +1,14 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -verify %s \
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -verify %s -Wno-strict-prototypes \
// RUN: -analyzer-checker=core,debug.ExprInspection,alpha.core.BoolAssignment
#define __UINT_MAX__ (__INT_MAX__ * 2U + 1U)
#define __INT_MIN__ (-__INT_MAX__ - 1)
+#define __UINT128_MAX__ ((__uint128_t)((__int128_t)(-1L)))
+#define __INT128_MAX__ ((__int128_t)(__UINT128_MAX__ >> 1))
+#define __UBITINT_MAX__(BITS) ((unsigned _BitInt(BITS))-1)
+#define __BITINT_MAX__(BITS) ((_BitInt(BITS))(__UBITINT_MAX__(BITS) >> 1))
-void clang_analyzer_dump_int(int);
-void clang_analyzer_dump_long(long);
+void clang_analyzer_dump(/*not specified*/);
void clang_analyzer_eval(int);
void clang_analyzer_warnIfReached(void);
@@ -14,11 +17,11 @@ void test_add_nooverflow(void)
int res;
if (__builtin_add_overflow(10, 20, &res)) {
- clang_analyzer_warnIfReached();
+ clang_analyzer_warnIfReached(); // no-wrapping happened
return;
}
- clang_analyzer_dump_int(res); //expected-warning{{30 S32b}}
+ clang_analyzer_dump(res); //expected-warning{{30 S32b}}
}
void test_add_overflow(void)
@@ -26,7 +29,7 @@ void test_add_overflow(void)
int res;
if (__builtin_add_overflow(__INT_MAX__, 1, &res)) {
- clang_analyzer_dump_int(res); //expected-warning{{-2147483648 S32b}}
+ clang_analyzer_dump(res); //expected-warning{{-2147483648 S32b}}
return;
}
@@ -38,7 +41,7 @@ void test_add_underoverflow(void)
int res;
if (__builtin_add_overflow(__INT_MIN__, -1, &res)) {
- clang_analyzer_dump_int(res); //expected-warning{{2147483647 S32b}}
+ clang_analyzer_dump(res); //expected-warning{{2147483647 S32b}}
return;
}
@@ -76,7 +79,7 @@ void test_sub_nooverflow(void)
return;
}
- clang_analyzer_dump_int(res); //expected-warning{{2147483646 S32b}}
+ clang_analyzer_dump(res); //expected-warning{{2147483646 S32b}}
}
void test_mul_overflow(void)
@@ -110,7 +113,7 @@ void test_mul_nooverflow(void)
return;
}
- clang_analyzer_dump_int(res); //expected-warning{{-20 S32b}}
+ clang_analyzer_dump(res); //expected-warning{{-20 S32b}}
}
void test_nooverflow_diff_types(void)
@@ -123,7 +126,7 @@ void test_nooverflow_diff_types(void)
return;
}
- clang_analyzer_dump_long(res); //expected-warning{{2147483648 S64b}}
+ clang_analyzer_dump(res); //expected-warning{{2147483648 S64b}}
}
void test_uaddll_overflow_contraints(unsigned long a, unsigned long b)
@@ -164,3 +167,51 @@ void test_bool_assign(void)
// should return _Bool, but not int.
_Bool ret = __builtin_mul_overflow(10, 20, &res); // no crash
}
+
+void no_crash_with_int128(__int128_t a, __int128_t b) {
+ __int128_t result = 0;
+ (void)__builtin_add_overflow(a, b, &result); // no-crash
+}
+
+void no_crash_with_uint128(__uint128_t a, __uint128_t b) {
+ __uint128_t result = 0;
+ (void)__builtin_add_overflow(a, b, &result); // no-crash
+}
+
+void no_crash_with_bigint(_BitInt(111) a, _BitInt(111) b) {
+ _BitInt(111) result = 0;
+ (void)__builtin_add_overflow(a, b, &result); // no-crash
+}
+
+void test_add_overflow_128(void) {
+ __int128_t res;
+
+ if (__builtin_add_overflow(__INT128_MAX__, 1, &res)) {
+ clang_analyzer_dump(res); // expected-warning {{-170141183460469231731687303715884105728 S128b}}
+ return;
+ }
+
+ clang_analyzer_warnIfReached(); // no-warning: we always get an overflow, thus choose the other branch
+}
+
+void test_add_overflow_u111(void) {
+ unsigned _BitInt(111) res;
+
+ if (__builtin_add_overflow(__UBITINT_MAX__(111), 1, &res)) {
+ clang_analyzer_dump(res); // expected-warning {{0 U111b}}
+ return;
+ }
+
+ clang_analyzer_warnIfReached(); // no-warning: we always get an overflow, thus choose the other branch
+}
+
+void test_add_overflow_s111(void) {
+ _BitInt(111) res;
+
+ if (__builtin_add_overflow(__BITINT_MAX__(111), 1, &res)) {
+ clang_analyzer_dump(res); // expected-warning {{-1298074214633706907132624082305024 S111b}}
+ return;
+ }
+
+ clang_analyzer_warnIfReached(); // no-warning: we always get an overflow, thus choose the other branch
+}
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Benics (steakhal) ChangesPreviously, if the result type was 'large' (at least 65 bits), then the ASTContext::getIntTypeForBitwidth would return an empty QualType, causing later a crash when we assume it's non-empty. Instead of using this API, we could piggyback on the BigInt type to formulate a "large enough" type for calculating the mathematically correct result for the operation to check against. Fixes #173795 rdar://166709144 Full diff: https://github.com/llvm/llvm-project/pull/174335.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index 8eb68b1fa638e..f50a3b08671e2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -36,9 +36,8 @@ QualType getSufficientTypeForOverflowOp(CheckerContext &C, const QualType &T) {
assert(T->isIntegerType());
ASTContext &ACtx = C.getASTContext();
-
unsigned BitWidth = ACtx.getIntWidth(T);
- return ACtx.getIntTypeForBitwidth(BitWidth * 2, T->isSignedIntegerType());
+ return ACtx.getBitIntType(T->isUnsignedIntegerType(), BitWidth * 2);
}
QualType getOverflowBuiltinResultType(const CallEvent &Call) {
@@ -208,8 +207,10 @@ void BuiltinFunctionChecker::handleOverflowBuiltin(const CallEvent &Call,
SVal Arg1 = Call.getArgSVal(0);
SVal Arg2 = Call.getArgSVal(1);
- SVal RetValMax = SVB.evalBinOp(State, Op, Arg1, Arg2,
- getSufficientTypeForOverflowOp(C, ResultType));
+ QualType SufficientlyWideTy = getSufficientTypeForOverflowOp(C, ResultType);
+ assert(!SufficientlyWideTy.isNull());
+
+ SVal RetValMax = SVB.evalBinOp(State, Op, Arg1, Arg2, SufficientlyWideTy);
SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
auto [Overflow, NotOverflow] = checkOverflow(C, RetValMax, ResultType);
diff --git a/clang/test/Analysis/builtin_overflow.c b/clang/test/Analysis/builtin_overflow.c
index d290333071dc9..ba7e52703fefe 100644
--- a/clang/test/Analysis/builtin_overflow.c
+++ b/clang/test/Analysis/builtin_overflow.c
@@ -1,11 +1,14 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -verify %s \
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -verify %s -Wno-strict-prototypes \
// RUN: -analyzer-checker=core,debug.ExprInspection,alpha.core.BoolAssignment
#define __UINT_MAX__ (__INT_MAX__ * 2U + 1U)
#define __INT_MIN__ (-__INT_MAX__ - 1)
+#define __UINT128_MAX__ ((__uint128_t)((__int128_t)(-1L)))
+#define __INT128_MAX__ ((__int128_t)(__UINT128_MAX__ >> 1))
+#define __UBITINT_MAX__(BITS) ((unsigned _BitInt(BITS))-1)
+#define __BITINT_MAX__(BITS) ((_BitInt(BITS))(__UBITINT_MAX__(BITS) >> 1))
-void clang_analyzer_dump_int(int);
-void clang_analyzer_dump_long(long);
+void clang_analyzer_dump(/*not specified*/);
void clang_analyzer_eval(int);
void clang_analyzer_warnIfReached(void);
@@ -14,11 +17,11 @@ void test_add_nooverflow(void)
int res;
if (__builtin_add_overflow(10, 20, &res)) {
- clang_analyzer_warnIfReached();
+ clang_analyzer_warnIfReached(); // no-wrapping happened
return;
}
- clang_analyzer_dump_int(res); //expected-warning{{30 S32b}}
+ clang_analyzer_dump(res); //expected-warning{{30 S32b}}
}
void test_add_overflow(void)
@@ -26,7 +29,7 @@ void test_add_overflow(void)
int res;
if (__builtin_add_overflow(__INT_MAX__, 1, &res)) {
- clang_analyzer_dump_int(res); //expected-warning{{-2147483648 S32b}}
+ clang_analyzer_dump(res); //expected-warning{{-2147483648 S32b}}
return;
}
@@ -38,7 +41,7 @@ void test_add_underoverflow(void)
int res;
if (__builtin_add_overflow(__INT_MIN__, -1, &res)) {
- clang_analyzer_dump_int(res); //expected-warning{{2147483647 S32b}}
+ clang_analyzer_dump(res); //expected-warning{{2147483647 S32b}}
return;
}
@@ -76,7 +79,7 @@ void test_sub_nooverflow(void)
return;
}
- clang_analyzer_dump_int(res); //expected-warning{{2147483646 S32b}}
+ clang_analyzer_dump(res); //expected-warning{{2147483646 S32b}}
}
void test_mul_overflow(void)
@@ -110,7 +113,7 @@ void test_mul_nooverflow(void)
return;
}
- clang_analyzer_dump_int(res); //expected-warning{{-20 S32b}}
+ clang_analyzer_dump(res); //expected-warning{{-20 S32b}}
}
void test_nooverflow_diff_types(void)
@@ -123,7 +126,7 @@ void test_nooverflow_diff_types(void)
return;
}
- clang_analyzer_dump_long(res); //expected-warning{{2147483648 S64b}}
+ clang_analyzer_dump(res); //expected-warning{{2147483648 S64b}}
}
void test_uaddll_overflow_contraints(unsigned long a, unsigned long b)
@@ -164,3 +167,51 @@ void test_bool_assign(void)
// should return _Bool, but not int.
_Bool ret = __builtin_mul_overflow(10, 20, &res); // no crash
}
+
+void no_crash_with_int128(__int128_t a, __int128_t b) {
+ __int128_t result = 0;
+ (void)__builtin_add_overflow(a, b, &result); // no-crash
+}
+
+void no_crash_with_uint128(__uint128_t a, __uint128_t b) {
+ __uint128_t result = 0;
+ (void)__builtin_add_overflow(a, b, &result); // no-crash
+}
+
+void no_crash_with_bigint(_BitInt(111) a, _BitInt(111) b) {
+ _BitInt(111) result = 0;
+ (void)__builtin_add_overflow(a, b, &result); // no-crash
+}
+
+void test_add_overflow_128(void) {
+ __int128_t res;
+
+ if (__builtin_add_overflow(__INT128_MAX__, 1, &res)) {
+ clang_analyzer_dump(res); // expected-warning {{-170141183460469231731687303715884105728 S128b}}
+ return;
+ }
+
+ clang_analyzer_warnIfReached(); // no-warning: we always get an overflow, thus choose the other branch
+}
+
+void test_add_overflow_u111(void) {
+ unsigned _BitInt(111) res;
+
+ if (__builtin_add_overflow(__UBITINT_MAX__(111), 1, &res)) {
+ clang_analyzer_dump(res); // expected-warning {{0 U111b}}
+ return;
+ }
+
+ clang_analyzer_warnIfReached(); // no-warning: we always get an overflow, thus choose the other branch
+}
+
+void test_add_overflow_s111(void) {
+ _BitInt(111) res;
+
+ if (__builtin_add_overflow(__BITINT_MAX__(111), 1, &res)) {
+ clang_analyzer_dump(res); // expected-warning {{-1298074214633706907132624082305024 S111b}}
+ return;
+ }
+
+ clang_analyzer_warnIfReached(); // no-warning: we always get an overflow, thus choose the other branch
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I thought that APInt supports big integers, but apparently it does not... Thanks for fixing this!
APInt supports big integers. It's that we would need to pass a QualType for the evalBinOp APIs, and previously we just picked the next large enough primitive type, such as 32, 64, 128 bit wide integral type. However, when In this patch, I simply sidestep this one by using BigInts, which provide arbitrary precision. Remember, this is only used for checking if wrapping happens. |
Is this type always available on all platforms? |
I dont see any reason why it wouldnt work cross platform. This is to compute the mathematical result (that we discard once we figured out that wrapping happened or not to bind the correct return value) but the calculation itself still has wrapping semantics. |
) Previously, if the result type was 'large' (at least 65 bits), then the ASTContext::getIntTypeForBitwidth would return an empty QualType, causing later a crash when we assume it's non-empty. Instead of using this API, we could piggyback on the BigInt type to formulate a "large enough" type for calculating the mathematically correct result for the operation to check against. Crash: https://godbolt.org/z/dGY3vh39a ```c++ void bigint(_BitInt(63) a, _BitInt(63) b) { _BitInt(63) result = 0; (void)__builtin_add_overflow(a, b, &result); // crashes here } ``` Fixes llvm#173795 rdar://166709144 (cherry picked from commit 984c577)
) Previously, if the result type was 'large' (at least 65 bits), then the ASTContext::getIntTypeForBitwidth would return an empty QualType, causing later a crash when we assume it's non-empty. Instead of using this API, we could piggyback on the BigInt type to formulate a "large enough" type for calculating the mathematically correct result for the operation to check against. Crash: https://godbolt.org/z/dGY3vh39a ```c++ void bigint(_BitInt(63) a, _BitInt(63) b) { _BitInt(63) result = 0; (void)__builtin_add_overflow(a, b, &result); // crashes here } ``` Fixes llvm#173795 rdar://166709144
Previously, if the result type was 'large' (at least 65 bits), then the ASTContext::getIntTypeForBitwidth would return an empty QualType, causing later a crash when we assume it's non-empty.
Instead of using this API, we could piggyback on the BigInt type to formulate a "large enough" type for calculating the mathematically correct result for the operation to check against.
Crash: https://godbolt.org/z/dGY3vh39a
Fixes #173795
rdar://166709144