[clang] [analyzer] Remove inaccurate legacy handling of bad bitwise shifts (PR #66647)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 18 06:47:01 PDT 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-static-analyzer-1
<details>
<summary>Changes</summary>
Previously, bitwise shifts with constant operands were validated by the checker `core.UndefinedBinaryOperatorResult`. However, this logic was unreliable, and commit 25b9696b61e53a958e217bb3d0eab66350dc187f 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.
---
Full diff: https://github.com/llvm/llvm-project/pull/66647.diff
8 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp (+3-53)
- (modified) clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp (-8)
- (modified) clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp (+14-1)
- (removed) clang/test/Analysis/bitwise-ops-nocrash.c (-28)
- (removed) clang/test/Analysis/bitwise-ops.c (-60)
- (modified) clang/test/Analysis/bitwise-shift-sanity-checks.c (+19-3)
- (removed) clang/test/Analysis/left-shift-cxx2a.cpp (-22)
- (modified) clang/test/Analysis/svalbuilder-simplify-no-crash.c (+3-1)
``````````diff
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 at -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);
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/66647
More information about the cfe-commits
mailing list