[analyzer] Remove inaccurate legacy handling of bad bitwise shifts #66647
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
Previously, bitwise shifts with constant operands were validated by the checker `core.UndefinedBinaryOperatorResult`. However, this logic was unreliable, and commit 25b9696 added the dedicated checker `core.BitwiseShift` which validated the preconditions of all bitwise shifts with a more accurate logic (that uses the real types from the AST instead of the unreliable type information encoded in `APSInt` objects). This commit disables the inaccurate logic that could mark bitwise shifts as 'undefined' and removes the redundant shift-related warning messages from core.UndefinedBinaryOperatorResult. The tests that were validating this logic are also deleted by this commit; but I verified that those testcases trigger the expected bug reports from `core.BitwiseShift`. (I didn't convert them to tests of `core.BitwiseShift`, because that checker already has its own extensive test suite with many analogous testcases.) I hope that there will be a time when the constant folding will be reliable, but until then we need hacky solutions like this improve the quality of results.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 ChangesPreviously, bitwise shifts with constant operands were validated by the checker This commit disables the inaccurate logic that could mark bitwise shifts as 'undefined' and removes the redundant shift-related warning messages from core.UndefinedBinaryOperatorResult. The tests that were validating this logic are also deleted by this commit; but I verified that those testcases trigger the expected bug reports from I hope that there will be a time when the constant folding will be reliable, but until then we need hacky solutions like this improve the quality of results.Full diff: https://github.com/llvm/llvm-project/pull/66647.diff 8 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
index a2b5e2987db5959..5f37b915deb8a02 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -115,59 +115,9 @@ void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
OS << " due to array index out of bounds";
} else {
// Neither operand was undefined, but the result is undefined.
- if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
- B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
- C.isNegative(B->getRHS())) {
- OS << "The result of the "
- << ((B->getOpcode() == BinaryOperatorKind::BO_Shl) ? "left"
- : "right")
- << " shift is undefined because the right operand is negative";
- Ex = B->getRHS();
- } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
- B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
- isShiftOverflow(B, C)) {
-
- OS << "The result of the "
- << ((B->getOpcode() == BinaryOperatorKind::BO_Shl) ? "left"
- : "right")
- << " shift is undefined due to shifting by ";
- Ex = B->getRHS();
-
- SValBuilder &SB = C.getSValBuilder();
- const llvm::APSInt *I =
- SB.getKnownValue(C.getState(), C.getSVal(B->getRHS()));
- if (!I)
- OS << "a value that is";
- else if (I->isUnsigned())
- OS << '\'' << I->getZExtValue() << "\', which is";
- else
- OS << '\'' << I->getSExtValue() << "\', which is";
-
- OS << " greater or equal to the width of type '"
- << B->getLHS()->getType() << "'.";
- } else if (B->getOpcode() == BinaryOperatorKind::BO_Shl &&
- C.isNegative(B->getLHS())) {
- OS << "The result of the left shift is undefined because the left "
- "operand is negative";
- Ex = B->getLHS();
- } else if (B->getOpcode() == BinaryOperatorKind::BO_Shl &&
- isLeftShiftResultUnrepresentable(B, C)) {
- ProgramStateRef State = C.getState();
- SValBuilder &SB = C.getSValBuilder();
- const llvm::APSInt *LHS =
- SB.getKnownValue(State, C.getSVal(B->getLHS()));
- const llvm::APSInt *RHS =
- SB.getKnownValue(State, C.getSVal(B->getRHS()));
- OS << "The result of the left shift is undefined due to shifting \'"
- << LHS->getSExtValue() << "\' by \'" << RHS->getZExtValue()
- << "\', which is unrepresentable in the unsigned version of "
- << "the return type \'" << B->getLHS()->getType() << "\'";
- Ex = B->getLHS();
- } else {
- OS << "The result of the '"
- << BinaryOperator::getOpcodeStr(B->getOpcode())
- << "' expression is undefined";
- }
+ OS << "The result of the '"
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined";
}
auto report = std::make_unique<PathSensitiveBugReport>(*BT, OS.str(), N);
if (Ex) {
diff --git a/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp b/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
index 5924f6a671c2ac1..e8d74b40c6fd846 100644
--- a/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -280,14 +280,6 @@ BasicValueFactory::evalAPSInt(BinaryOperator::Opcode Op,
if (Amt >= V1.getBitWidth())
return nullptr;
- if (!Ctx.getLangOpts().CPlusPlus20) {
- if (V1.isSigned() && V1.isNegative())
- return nullptr;
-
- if (V1.isSigned() && Amt > V1.countl_zero())
- return nullptr;
- }
-
return &getValue( V1.operator<<( (unsigned) Amt ));
}
diff --git a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
index 58d360a2e2dbdf1..aa2c8bbd15d4206 100644
--- a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -529,8 +529,21 @@ SVal SimpleSValBuilder::evalBinOpNN(ProgramStateRef state,
const llvm::APSInt *Result =
BasicVals.evalAPSInt(op, LHSValue, RHSValue);
- if (!Result)
+ if (!Result) {
+ if (op == BO_Shl || op == BO_Shr) {
+ // FIXME: At this point the constant folding claims that the result
+ // of a bitwise shift is undefined. However, constant folding
+ // relies on the inaccurate type information that is stored in the
+ // bit size of APSInt objects, and if we reached this point, then
+ // the checker core.BitwiseShift already determined that the shift
+ // is valid (in a PreStmt callback, by querying the real type from
+ // the AST node).
+ // To avoid embarrassing false positives, let's just say that we
+ // don't know anything about the result of the shift.
+ return UnknownVal();
+ }
return UndefinedVal();
+ }
return nonloc::ConcreteInt(*Result);
}
diff --git a/clang/test/Analysis/bitwise-ops-nocrash.c b/clang/test/Analysis/bitwise-ops-nocrash.c
deleted file mode 100644
index 4c4900bc96a1855..000000000000000
--- a/clang/test/Analysis/bitwise-ops-nocrash.c
+++ /dev/null
@@ -1,28 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-disable-checker=core.BitwiseShift -analyzer-output=text -triple x86_64-linux-gnu -Wno-shift-count-overflow -verify %s
-
-// NOTE: This test file disables the checker core.BitwiseShift (which would
-// report all undefined behavior connected to bitwise shifts) to verify the
-// behavior of core.UndefinedBinaryOperatorResult (which resports cases when
-// the constant folding in BasicValueFactory produces an "undefined" result
-// from a shift or any other binary operator).
-
-#define offsetof(type,memb) ((unsigned long)&((type*)0)->memb)
-
-typedef struct {
- unsigned long guest_counter;
- unsigned int guest_fpc;
-} S;
-
-// no crash
-int left_shift_overflow_no_crash(unsigned int i) {
- unsigned shift = 323U; // expected-note{{'shift' initialized to 323}}
- switch (i) { // expected-note{{Control jumps to 'case 8:' at line 20}}
- case offsetof(S, guest_fpc):
- return 3 << shift; // expected-warning{{The result of the left shift is undefined due to shifting by '323', which is greater or equal to the width of type 'int'}}
- // expected-note@-1{{The result of the left shift is undefined due to shifting by '323', which is greater or equal to the width of type 'int'}}
- default:
- break;
- }
-
- return 0;
-}
diff --git a/clang/test/Analysis/bitwise-ops.c b/clang/test/Analysis/bitwise-ops.c
deleted file mode 100644
index 7ff7490ba80cf63..000000000000000
--- a/clang/test/Analysis/bitwise-ops.c
+++ /dev/null
@@ -1,60 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker=core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify %s
-
-// NOTE: This test file disables the checker core.BitwiseShift (which would
-// report all undefined behavior connected to bitwise shifts) to verify the
-// behavior of core.UndefinedBinaryOperatorResult (which resports cases when
-// the constant folding in BasicValueFactory produces an "undefined" result
-// from a shift or any other binary operator).
-
-void clang_analyzer_eval(int);
-#define CHECK(expr) if (!(expr)) return; clang_analyzer_eval(expr)
-
-void testPersistentConstraints(int x, int y) {
- // Basic check
- CHECK(x); // expected-warning{{TRUE}}
- CHECK(x & 1); // expected-warning{{TRUE}}
-
- CHECK(1 - x); // expected-warning{{TRUE}}
- CHECK(x & y); // expected-warning{{TRUE}}
-}
-
-int testConstantShifts_PR18073(int which) {
- switch (which) {
- case 1:
- return 0ULL << 63; // no-warning
- case 2:
- return 0ULL << 64; // expected-warning{{The result of the left shift is undefined due to shifting by '64', which is greater or equal to the width of type 'unsigned long long'}}
- case 3:
- return 0ULL << 65; // expected-warning{{The result of the left shift is undefined due to shifting by '65', which is greater or equal to the width of type 'unsigned long long'}}
-
- default:
- return 0;
- }
-}
-
-int testOverflowShift(int a) {
- if (a == 323) {
- return 1 << a; // expected-warning{{The result of the left shift is undefined due to shifting by '323', which is greater or equal to the width of type 'int'}}
- }
- return 0;
-}
-
-int testNegativeShift(int a) {
- if (a == -5) {
- return 1 << a; // expected-warning{{The result of the left shift is undefined because the right operand is negative}}
- }
- return 0;
-}
-
-int testNegativeLeftShift(int a) {
- if (a == -3) {
- return a << 1; // expected-warning{{The result of the left shift is undefined because the left operand is negative}}
- }
- return 0;
-}
-
-int testUnrepresentableLeftShift(int a) {
- if (a == 8)
- return a << 30; // expected-warning{{The result of the left shift is undefined due to shifting '8' by '30', which is unrepresentable in the unsigned version of the return type 'int'}}
- return 0;
-}
diff --git a/clang/test/Analysis/bitwise-shift-sanity-checks.c b/clang/test/Analysis/bitwise-shift-sanity-checks.c
index 1e20d5df3567e44..a31424f18818c45 100644
--- a/clang/test/Analysis/bitwise-shift-sanity-checks.c
+++ b/clang/test/Analysis/bitwise-shift-sanity-checks.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core.BitwiseShift \
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
// RUN: -analyzer-config core.BitwiseShift:Pedantic=true \
// RUN: -verify=expected,pedantic \
// RUN: -triple x86_64-pc-linux-gnu -x c %s \
@@ -6,7 +6,7 @@
// RUN: -Wno-shift-count-overflow -Wno-shift-overflow \
// RUN: -Wno-shift-sign-overflow
//
-// RUN: %clang_analyze_cc1 -analyzer-checker=core.BitwiseShift \
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
// RUN: -analyzer-config core.BitwiseShift:Pedantic=true \
// RUN: -verify=expected,pedantic \
// RUN: -triple x86_64-pc-linux-gnu -x c++ -std=c++14 %s \
@@ -14,7 +14,7 @@
// RUN: -Wno-shift-count-overflow -Wno-shift-overflow \
// RUN: -Wno-shift-sign-overflow
//
-// RUN: %clang_analyze_cc1 -analyzer-checker=core.BitwiseShift \
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
// RUN: -verify=expected \
// RUN: -triple x86_64-pc-linux-gnu -x c++ -std=c++20 %s \
// RUN: -Wno-shift-count-negative -Wno-shift-negative-value \
@@ -23,6 +23,8 @@
// This test file verifies that the BitwiseShift checker does not crash or
// report false positives (at least on the cases that are listed here...)
+// Other core checkers are also enabled to see interactions with e.g.
+// core.UndefinedBinaryOperatorResult.
// For the sake of brevity, 'note' output is not checked in this file.
// TEST OBVIOUSLY CORRECT CODE
@@ -116,3 +118,17 @@ void doubles_cast_to_integer(int *c) {
*c = ((int)1.5) << 1; // no-crash
*c = ((int)1.5) << (int)1.5; // no-crash
}
+
+// TEST CODE THAT WAS TRIGGERING BUGS IN EARLIER REVISIONS
+//===----------------------------------------------------------------------===//
+
+unsigned int strange_cast(unsigned short sh) {
+ // This testcase triggers a bug in the constant folding (it "forgets" the
+ // cast), which is silenced in SimpleSValBuilder::evalBinOpNN() with an ugly
+ // workaround, because otherwise it would lead to a false positive from
+ // core.UndefinedBinaryOperatorResult.
+ unsigned int i;
+ sh++;
+ for (i=0; i<sh; i++) {}
+ return (unsigned int) ( ((unsigned int) sh) << 16 ); // no-warning
+}
diff --git a/clang/test/Analysis/left-shift-cxx2a.cpp b/clang/test/Analysis/left-shift-cxx2a.cpp
deleted file mode 100644
index 006642e9f06a2cb..000000000000000
--- a/clang/test/Analysis/left-shift-cxx2a.cpp
+++ /dev/null
@@ -1,22 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
-
-int testNegativeShift(int a) {
- if (a == -5) {
- return 1 << a; // expected-warning{{The result of the left shift is undefined because the right operand is negative}}
- }
- return 0;
-}
-
-int testNegativeLeftShift(int a) {
- if (a == -3) {
- return a << 1; // cxx17-warning{{The result of the left shift is undefined because the left operand is negative}}
- }
- return 0;
-}
-
-int testUnrepresentableLeftShift(int a) {
- if (a == 8)
- return a << 30; // cxx17-warning{{The result of the left shift is undefined due to shifting '8' by '30', which is unrepresentable in the unsigned version of the return type 'int'}}
- return 0;
-}
diff --git a/clang/test/Analysis/svalbuilder-simplify-no-crash.c b/clang/test/Analysis/svalbuilder-simplify-no-crash.c
index b43ccbdbfd1002d..b3166413ecace87 100644
--- a/clang/test/Analysis/svalbuilder-simplify-no-crash.c
+++ b/clang/test/Analysis/svalbuilder-simplify-no-crash.c
@@ -6,8 +6,10 @@
// Here, we test that svalbuilder simplification does not produce any
// assertion failure.
+// expected-no-diagnostics
+
void crashing(long a, _Bool b) {
(void)(a & 1 && 0);
b = a & 1;
- (void)(b << 1); // expected-warning{{core.UndefinedBinaryOperatorResult}}
+ (void)(b << 1);
}
|
|
What report diff's should we expect? |
|
The impact is limited, because The most significant change is that some "pedantic-only" issues (i.e. code like Full results:
[1] one alpha.core.Conversion result lost for unclear reason, 4 pedantic-only results and 2 FPs eliminated, one of the pedantic-only results was replaced by two new alpha.core.Conversion results |
|
I think I'm fine with the numbers. Note that I could not inspect individual reports because the URL likely refers to some internal server on your side.
|
|
Sorry for the inaccessible reports, I forgot to specify that the results should go to the server that we use for open-source reviews. I'd like to skip the conversion of the testcases that I deleted, because there won't be folks "wondering what happened with these reports": In the majority of situations, the report type already "switched over" to This PR only affects the "undefined behavior, but works in practice" cases where Also, even if I wasted some time on converting those testcases, I'd have to delete them in a later commit, because they're completely redundant with the testsuite of |
|
LGTM if ok with everyone. |
steakhal
approved these changes
Sep 29, 2023
|
This commit accidentally left behind two unused functions; thanks @kazutakahirata for quickly fixing my mistake! |
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.
Previously, bitwise shifts with constant operands were validated by the checker
core.UndefinedBinaryOperatorResult. However, this logic was unreliable, and commit 25b9696 added the dedicated checkercore.BitwiseShiftwhich validated the preconditions of all bitwise shifts with a more accurate logic (that uses the real types from the AST instead of the unreliable type information encoded inAPSIntobjects).This commit disables the inaccurate logic that could mark bitwise shifts as 'undefined' and removes the redundant shift-related warning messages from core.UndefinedBinaryOperatorResult. The tests that were validating this logic are also deleted by this commit; but I verified that those testcases trigger the expected bug reports from
core.BitwiseShift. (I didn't convert them to tests ofcore.BitwiseShift, because that checker already has its own extensive test suite with many analogous testcases.)I hope that there will be a time when the constant folding will be reliable, but until then we need hacky solutions like this improve the quality of results.