[analyzer] Fix assertion failure after getKnownValue call

[martong]

Summary

Depends on D126560. getKnownValue has been changed by the parent patch
in a way that simplification was removed. This is not correct when the
function is called by the Checkers. Thus, a new internal function is
introduced, getConstValue, which simply queries the constraint manager.
This getConstValue is used internally in the SimpleSValBuilder when a
binop is evaluated, this way we avoid the recursion into the Simplifier.

[steakhal]

LGTM
Schedule a measurement to see what changes. Let's hope it won't introduce more crashes.
If everything checks out, it gets approved.

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

25–28

I'm also wondering if this should be at the top. I would rather put it below simplifySValOnce().

[steakhal]

Check the summary for typos. Try to mark named entities like this.

[martong]

In D127285#3566034, @steakhal wrote:

... Try to mark named entities like this.

Could you please elaborate?

[steakhal]

In D127285#3566198, @martong wrote:

In D127285#3566034, @steakhal wrote:

... Try to mark named entities like this.

Could you please elaborate?

Oh sure. The SimpleSValBuilder is not escaped like code or class entities should be in the summary. But it's really not that important.

[martong]

In D127285#3566397, @steakhal wrote:

In D127285#3566198, @martong wrote:

In D127285#3566034, @steakhal wrote:

... Try to mark named entities like this.

Could you please elaborate?

Oh sure. The SimpleSValBuilder is not escaped like code or class entities should be in the summary. But it's really not that important.

Ok, thanks!

[martong]

• Add one more sentence in the comments.

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

25–28

It is a good idea to draw the checker writer's attention to use getKnownValue, however, this is a private function deliberately for this exact reason.

I'm also wondering if this should be at the top. I would rather put it below simplifySValOnce().

I've put it here because simplifySValOnce references this even in its header comment.

[steakhal]

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

25–28

It is a good idea to draw the checker writer's attention to use getKnownValue, however, this is a private function deliberately for this exact reason.

I know. My point is that the name of those two functions won't express the difference; thus I wanted to have a more verbose way of expressing that the private impl. fn. should not be exposed. I won't insist though.

I've put it here because simplifySValOnce references this even in its header comment.

Well, it also refers to simplifySValOnce(), yet that's below that comment.

[martong]

In D127285#3566032, @steakhal wrote:

LGTM
Schedule a measurement to see what changes. Let's hope it won't introduce more crashes.
If everything checks out, it gets approved.

Results are good. No new assertion failures, neither crashes. Runtime is the same (in the margin of error). Redis has been added to the set of analyzed projects this time.

stats.html211 KBDownload

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 class SimpleSValBuilder : public SValBuilder {
 
+  // Query the constraint manager whether the SVal has only one possible
+  // (integer) value. If that is the case, the value is returned. Otherwise,
+  // returns NULL.
+  const llvm::APSInt *getConstValue(ProgramStateRef state, SVal V);
+
   // With one `simplifySValOnce` call, a compound symbols might collapse to
   // simpler symbol tree that is still possible to further simplify. Thus, we
   // do the simplification on a new symbol tree until we reach the simplest
   // form, i.e. the fixpoint.
   // Consider the following symbol `(b * b) * b * b` which has this tree:
   //       *
   //      / \
   //     *   b
   //    /  \
   //   /    b
   // (b * b)
   // Now, if the `b * b == 1` new constraint is added then during the first
   // iteration we have the following transformations:
   //       *                  *
   //      / \                / \
   //     *   b     -->      b   b
   //    /  \
   //   /    b
   //  1
   // We need another iteration to reach the final result `1`.
   SVal simplifyUntilFixpoint(ProgramStateRef State, SVal Val);
 
   // Recursively descends into symbolic expressions and replaces symbols
-  // with their known values (in the sense of the getKnownValue() method).
+  // with their known values (in the sense of the getConstValue() method).
   // We traverse the symbol tree and query the constraint values for the
   // sub-trees and if a value is a constant we do the constant folding.
   SVal simplifySValOnce(ProgramStateRef State, SVal V);
 
 public:
   SimpleSValBuilder(llvm::BumpPtrAllocator &alloc, ASTContext &context,
                     ProgramStateManager &stateMgr)
       : SValBuilder(alloc, context, stateMgr) {}
   ~SimpleSValBuilder() override {}
 
   SVal evalMinus(NonLoc val) override;
   SVal evalComplement(NonLoc val) override;
   SVal evalBinOpNN(ProgramStateRef state, BinaryOperator::Opcode op,
                    NonLoc lhs, NonLoc rhs, QualType resultTy) override;
   SVal evalBinOpLL(ProgramStateRef state, BinaryOperator::Opcode op,
                    Loc lhs, Loc rhs, QualType resultTy) override;
   SVal evalBinOpLN(ProgramStateRef state, BinaryOperator::Opcode op,
                    Loc lhs, NonLoc rhs, QualType resultTy) override;
 
-  /// getKnownValue - evaluates a given SVal. If the SVal has only one possible
-  ///  (integer) value, that value is returned. Otherwise, returns NULL.
+  /// Evaluates a given SVal by recursively evaluating and
+  /// simplifying the children SVals. If the SVal has only one possible
+  /// (integer) value, that value is returned. Otherwise, returns NULL.
   const llvm::APSInt *getKnownValue(ProgramStateRef state, SVal V) override;
 
   SVal simplifySVal(ProgramStateRef State, SVal V) override;
 
   SVal MakeSymIntVal(const SymExpr *LHS, BinaryOperator::Opcode op,
                      const llvm::APSInt &RHS, QualType resultTy);
 };
 } // end anonymous namespace
case nonloc::LocAsIntegerKind: {
               return makeSymExprValNN(op, InputLHS, InputRHS, resultTy);
           }
       }
     }
     case nonloc::ConcreteIntKind: {
       llvm::APSInt LHSValue = lhs.castAs<nonloc::ConcreteInt>().getValue();
 
       // If we're dealing with two known constants, just perform the operation.
-      if (const llvm::APSInt *KnownRHSValue = getKnownValue(state, rhs)) {
+      if (const llvm::APSInt *KnownRHSValue = getConstValue(state, rhs)) {
         llvm::APSInt RHSValue = *KnownRHSValue;
         if (BinaryOperator::isComparisonOp(op)) {
           // We're looking for a type big enough to compare the two values.
           // FIXME: This is not correct. char + short will result in a promotion
           // to int. Unfortunately we have lost types by this point.
           APSIntType CompareType = std::max(APSIntType(LHSValue),
                                             APSIntType(RHSValue));
           CompareType.apply(LHSValue);
case nonloc::SymbolValKind: {
             // Negate the comparison and make a value.
             opc = BinaryOperator::negateComparisonOp(opc);
             return makeNonLoc(symIntExpr->getLHS(), opc,
                 symIntExpr->getRHS(), resultTy);
           }
         }
 
         // For now, only handle expressions whose RHS is a constant.
-        if (const llvm::APSInt *RHSValue = getKnownValue(state, rhs)) {
+        if (const llvm::APSInt *RHSValue = getConstValue(state, rhs)) {
           // If both the LHS and the current expression are additive,
           // fold their constants and try again.
           if (BinaryOperator::isAdditiveOp(op)) {
             BinaryOperator::Opcode lop = symIntExpr->getOpcode();
             if (BinaryOperator::isAdditiveOp(lop)) {
               // Convert the two constants to a common type, then combine them.
 
               // resultTy may not be the best type to convert to, but it's
case nonloc::SymbolValKind: {
           }
 
           // Otherwise, make a SymIntExpr out of the expression.
           return MakeSymIntVal(symIntExpr, op, *RHSValue, resultTy);
         }
       }
 
       // Is the RHS a constant?
-      if (const llvm::APSInt *RHSValue = getKnownValue(state, rhs))
+      if (const llvm::APSInt *RHSValue = getConstValue(state, rhs))
         return MakeSymIntVal(Sym, op, *RHSValue, resultTy);
 
       if (Optional<NonLoc> V = tryRearrange(state, op, lhs, rhs, resultTy))
         return *V;
 
       // Give up -- this is not a symbolic expression we can handle.
       return makeSymExprValNN(op, InputLHS, InputRHS, resultTy);
     }
if (const MemRegion *region = lhs.getAsRegion()) {
     if (Optional<NonLoc> indexV = index.getAs<NonLoc>()) {
       return loc::MemRegionVal(MemMgr.getElementRegion(elementType, *indexV,
                                                        superR, getContext()));
     }
   }
   return UnknownVal();
 }
 
-const llvm::APSInt *SimpleSValBuilder::getKnownValue(ProgramStateRef state,
+const llvm::APSInt *SimpleSValBuilder::getConstValue(ProgramStateRef state,
                                                      SVal V) {
   if (V.isUnknownOrUndef())
     return nullptr;
 
   if (Optional<loc::ConcreteInt> X = V.getAs<loc::ConcreteInt>())
     return &X->getValue();
 
   if (Optional<nonloc::ConcreteInt> X = V.getAs<nonloc::ConcreteInt>())
     return &X->getValue();
 
   if (SymbolRef Sym = V.getAsSymbol())
     return state->getConstraintManager().getSymVal(state, Sym);
 
   return nullptr;
 }
 
+const llvm::APSInt *SimpleSValBuilder::getKnownValue(ProgramStateRef state,
+                                                     SVal V) {
+  return getConstValue(state, simplifySVal(state, V));
+}
+
 SVal SimpleSValBuilder::simplifyUntilFixpoint(ProgramStateRef State, SVal Val) {
   SVal SimplifiedVal = simplifySValOnce(State, Val);
   while (SimplifiedVal != Val) {
     Val = SimplifiedVal;
     SimplifiedVal = simplifySValOnce(State, Val);
   }
   return SimplifiedVal;
 }
SVal SimpleSValBuilder::simplifySValOnce(ProgramStateRef State, SVal V) {
 
   public:
     Simplifier(ProgramStateRef State)
         : State(State), SVB(State->getStateManager().getSValBuilder()) {}
 
     SVal VisitSymbolData(const SymbolData *S) {
       // No cache here.
       if (const llvm::APSInt *I =
-              SVB.getKnownValue(State, SVB.makeSymbolVal(S)))
+              State->getConstraintManager().getSymVal(State, S))
         return Loc::isLocType(S->getType()) ? (SVal)SVB.makeIntLocVal(*I)
                                             : (SVal)SVB.makeIntVal(*I);
       return SVB.makeSymbolVal(S);
     }
 
     SVal VisitSymIntExpr(const SymIntExpr *S) {
       auto I = Cached.find(S);
       if (I != Cached.end())

clang/test/Analysis/svalbuilder-simplify-no-crash.c

+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -verify
+
+// Here, we test that svalbuilder simplification does not produce any
+// assertion failure.
+
+void crashing(long a, _Bool b) {
+  (void)(a & 1 && 0);
+  b = a & 1;
+  (void)(b << 1); // expected-warning{{core.UndefinedBinaryOperatorResult}}
+}