shylie/llvm-project
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[analyzer] Remove inaccurate legacy handling of bad bitwise shifts (#66647)

Browse Source 
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.
This commit is contained in:
DonatNagyE 2023-09-29 20:02:38 +02:00 committed by GitHub
parent 22ebf1e9b7
commit 23b88e8123
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 39 additions and 176 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
View File

@ -115,60 +115,10 @@ 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";
}
}
auto report = std::make_unique<PathSensitiveBugReport>(*BT, OS.str(), N);
if (Ex) {
report->addRange(Ex->getSourceRange());

8
clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
View File

@ -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 ));
}

15
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
View File

@ -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);
}

28
clang/test/Analysis/bitwise-ops-nocrash.c
View File

@ -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;
}

60
clang/test/Analysis/bitwise-ops.c
View File

@ -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;
}

22
clang/test/Analysis/bitwise-shift-sanity-checks.c
View File

@ -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
}

22
clang/test/Analysis/left-shift-cxx2a.cpp
View File

@ -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;
}

4
clang/test/Analysis/svalbuilder-simplify-no-crash.c
View File

@ -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);
}