[analyzer] Analysis: Prevent bitwise operation false positives

[Charusso]

Summary

This patch introduces a new analyzer-config configuration:
-analyzer-config check-bitwise=true
which could be used to toggle whether the bitwise (and shift) operations
should be checked. It by default set to true.

[NoQ]

No-no, you're disabling the checkers here but you should be silencing them. This will be crashing because modeling is disabled.

I also recommend checker options, even if they apply to multiple checkers (@Szelethus mentioned that package options are a thing, you might use these if you don't want to make multiple options).

clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp

83–89

Ugh. This part is also wrong for the same reason.

[NoQ]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

305

We didn't do enough debugging to demonstrate that the problems are specific to the checker. I suggest we keep this on by default and only disable it on LLVM as an overfitting effort.

[Charusso]

• Fix.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

305

Well, okay.

clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp

83–89

If I do not return here then clang/test/Analysis/uninit-vals.m:113: Returning from 'swap' related notes are gone, so I will leave it untouched.

[NoQ]

Now you're silencing the *whole* checker. This is equivalent to passing an -analyzer-silence-checker flag.

[Charusso]

In D66721#1644531, @NoQ wrote:

Now you're silencing the *whole* checker. This is equivalent to passing an -analyzer-silence-checker flag.

I am silencing the *whole* checker if and only if a bitwise operation or a shift operation occurs. That is the correct behavior which is not equal to -analyzer-silence-checker which disables the *whole* checker during the *whole* analysis.

[Szelethus]

In D66721#1644514, @NoQ wrote:

No-no, you're disabling the checkers here but you should be silencing them. This will be crashing because modeling is disabled.

I also recommend checker options, even if they apply to multiple checkers (@Szelethus mentioned that package options are a thing, you might use these if you don't want to make multiple options).

Just to be clear, I object to nothing as long as it is an off-by-default developer-only feature.

clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp

66

This is one of the reasons why I believe AnalyzerOptions should be immutable once analysis begins. Say that you'd like to list the checkers currently enabled with -analyzer-list-enabled-checkers, and that list is compiled around the time CheckerRegistry is active (which is a very brief period during the initialization of CheckerManager). Wouldn't it be super infuriating to learn that this lislt may change *during* analysis?

I feel somewhat the same way about this, this should either be moved to shouldRegister*() or be handled outside the analyzer.

[NoQ]

Until we do enough debugging to understand the nature of these false positives, i recommend silencing the checker when it has false positives. Later we can improve the granularity of our checkers, so that to be able to disable smaller portions of them, or make constraint solver improvements, depending on what we see.

[MaskRay]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

308

typo: unsigned

[NoQ]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

308

Yup, see also :)

clang/include/clang/AST/Expr.h

public:
   static bool isAdditiveOp(Opcode Opc) { return Opc == BO_Add || Opc==BO_Sub; }
   bool isAdditiveOp() const { return isAdditiveOp(getOpcode()); }
   static bool isShiftOp(Opcode Opc) { return Opc == BO_Shl || Opc == BO_Shr; }
   bool isShiftOp() const { return isShiftOp(getOpcode()); }
 
   static bool isBitwiseOp(Opcode Opc) { return Opc >= BO_And && Opc <= BO_Or; }
   bool isBitwiseOp() const { return isBitwiseOp(getOpcode()); }
 
+  static bool isBitwiseOrShiftOp(Opcode Opc) {
+    return isBitwiseOp(Opc) || isShiftOp(Opc);
+  }
+  bool isBitwiseOrShiftOp() const { return isBitwiseOrShiftOp(getOpcode()); }
+
   static bool isRelationalOp(Opcode Opc) { return Opc >= BO_LT && Opc<=BO_GE; }
   bool isRelationalOp() const { return isRelationalOp(getOpcode()); }
 
   static bool isEqualityOp(Opcode Opc) { return Opc == BO_EQ || Opc == BO_NE; }
   bool isEqualityOp() const { return isEqualityOp(getOpcode()); }
 
   static bool isComparisonOp(Opcode Opc) { return Opc >= BO_Cmp && Opc<=BO_NE; }
   bool isComparisonOp() const { return isComparisonOp(getOpcode()); }

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

ANALYZER_OPTION(bool, ShouldTrackConditions, "track-conditions",
                 "Whether to track conditions that are a control dependency of "
                 "an already tracked variable.",
                 true)
 
 ANALYZER_OPTION(bool, ShouldTrackConditionsDebug, "track-conditions-debug",
                 "Whether to place an event at each tracked condition.",
                 false)
 
+ANALYZER_OPTION(bool, CheckBitwise, "check-bitwise",
+                "Whether the bitwise (and shift) operations should be checked.",
+                true)
+
+//===----------------------------------------------------------------------===//
+// Unsinged analyzer options.
+//===----------------------------------------------------------------------===//
+
 ANALYZER_OPTION(unsigned, CTUImportThreshold, "ctu-import-threshold",
                 "The maximal amount of translation units that is considered "
                 "for import when inlining functions during CTU analysis. "
                 "Lowering this threshold can alleviate the memory burder of "
                 "analysis with many interdependent definitions located in "
                 "various translation units.",
                 100u)
 
-//===----------------------------------------------------------------------===//
-// Unsinged analyzer options.
-//===----------------------------------------------------------------------===//
-
 ANALYZER_OPTION(
     unsigned, AlwaysInlineSize, "ipa-always-inline-size",
     "The size of the functions (in basic blocks), which should be considered "
     "to be small enough to always inline.",
     3)
 
 ANALYZER_OPTION(
     unsigned, GraphTrimInterval, "graph-trim-interval",

clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp

 };
 
 }
 
 void UndefBranchChecker::checkBranchCondition(const Stmt *Condition,
                                               CheckerContext &Ctx) const {
   SVal X = Ctx.getSVal(Condition);
   if (X.isUndef()) {
+    AnalyzerOptions &Opts = Ctx.getAnalysisManager().getAnalyzerOptions();
+    if (const auto *BO = dyn_cast<BinaryOperator>(Condition))
+      if (BO->isBitwiseOrShiftOp() && !Opts.CheckBitwise)
+        Opts.SilencedCheckersAndPackages.push_back("core.uninitialized.Branch");
+
     // Generate a sink node, which implicitly marks both outgoing branches as
     // infeasible.
     ExplodedNode *N = Ctx.generateErrorNode();
     if (N) {
       if (!BT)
         BT.reset(new BuiltinBug(
             this, "Branch condition evaluates to a garbage value"));
 

clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp

static bool isLeftShiftResultUnrepresentable(const BinaryOperator *B,
   const llvm::APSInt *RHS = SB.getKnownValue(State, C.getSVal(B->getRHS()));
   assert(LHS && RHS && "Values unknown, inconsistent state");
   return (unsigned)RHS->getZExtValue() > LHS->countLeadingZeros();
 }
 
 void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
                                        CheckerContext &C) const {
   if (C.getSVal(B).isUndef()) {
+    AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions();
+    if (B->isBitwiseOrShiftOp() && !Opts.CheckBitwise)
+      Opts.SilencedCheckersAndPackages.push_back(
+          "core.UndefinedBinaryOperatorResult");
 
     // Do not report assignments of uninitialized values inside swap functions.
     // This should allow to swap partially uninitialized structs
     // (radar://14129997)
     if (const FunctionDecl *EnclosingFunctionDecl =
         dyn_cast<FunctionDecl>(C.getStackFrame()->getDecl()))
       if (C.getCalleeName(EnclosingFunctionDecl) == "swap")
         return;
 
     // Generate an error node.
     ExplodedNode *N = C.generateErrorNode();
     if (!N)
       return;
 
     if (!BT)
       BT.reset(

clang/test/Analysis/analyzer-config.c

 // CHECK-NEXT: c++-template-inlining = true
 // CHECK-NEXT: cfg-conditional-static-initializers = true
 // CHECK-NEXT: cfg-implicit-dtors = true
 // CHECK-NEXT: cfg-lifetime = false
 // CHECK-NEXT: cfg-loopexit = false
 // CHECK-NEXT: cfg-rich-constructors = true
 // CHECK-NEXT: cfg-scopes = false
 // CHECK-NEXT: cfg-temporary-dtors = true
+// CHECK-NEXT: check-bitwise = true
 // CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: crosscheck-with-z3 = false
 // CHECK-NEXT: ctu-dir = ""
 // CHECK-NEXT: ctu-import-threshold = 100
 // CHECK-NEXT: ctu-index-name = externalDefMap.txt
 // CHECK-NEXT: debug.AnalysisOrder:* = false
 // CHECK-NEXT: debug.AnalysisOrder:Bind = false
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false

 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: track-conditions = true
 // CHECK-NEXT: track-conditions-debug = false
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 90
+// CHECK-NEXT: num-entries = 91