[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:45:15 PDT 2023


https://github.com/DonatNagyE created https://github.com/llvm/llvm-project/pull/66647

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.

>From f5adc9b2a86266641d046ff83ed65c25a2419948 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Mon, 18 Sep 2023 13:06:33 +0200
Subject: [PATCH] [analyzer] Remove inaccurate legacy handling of bad bitwise
 shifts

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.
---
 .../Checkers/UndefResultChecker.cpp           | 56 +----------------
 .../StaticAnalyzer/Core/BasicValueFactory.cpp |  8 ---
 .../StaticAnalyzer/Core/SimpleSValBuilder.cpp | 15 ++++-
 clang/test/Analysis/bitwise-ops-nocrash.c     | 28 ---------
 clang/test/Analysis/bitwise-ops.c             | 60 -------------------
 .../Analysis/bitwise-shift-sanity-checks.c    | 22 ++++++-
 clang/test/Analysis/left-shift-cxx2a.cpp      | 22 -------
 .../Analysis/svalbuilder-simplify-no-crash.c  |  4 +-
 8 files changed, 39 insertions(+), 176 deletions(-)
 delete mode 100644 clang/test/Analysis/bitwise-ops-nocrash.c
 delete mode 100644 clang/test/Analysis/bitwise-ops.c
 delete mode 100644 clang/test/Analysis/left-shift-cxx2a.cpp

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



More information about the cfe-commits mailing list