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

…66647)

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.

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) {

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

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

clang/test/Analysis/bitwise-shift-sanity-checks.c

@@ -1,20 +1,20 @@
-// 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 \
 // RUN:    -Wno-shift-count-negative -Wno-shift-negative-value \
 // 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 \
 // RUN:    -Wno-shift-count-negative -Wno-shift-negative-value \
 // 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
+}

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