[Analyzer] More accurate modeling about the increment operator of the operand with type bool.

[MTC]

Summary

There is a problem with analyzer that a wrong value is given when modeling the increment operator of the operand with type bool. After rL307604 is applied, a unsigned overflow may occur.

Example:

void func() {
  bool b = true;
  // unsigned overflow occur, 2 -> 0 U1b
  b++;
}

The use of an operand of type bool with the ++ operators is deprecated but valid untill C++17. And if the operand of the increment operator is of type bool, it is set to true.

This patch includes two parts:

• If the operand of the increment operator is of type bool or type _Bool, set to true.
• Modify BasicValueFactory::getTruthValue(), use getIntWidth() instead getTypeSize() and use unsigned instead signed.

[alexander-shaposhnikov]

i see, but just in case - what about the decrement operator ?

[MTC]

In D43741#1024949, @alexshap wrote:

i see, but just in case - what about the decrement operator ?

@alexshap, If I'm not wrong, decrement operation is not allowed on bool type in C++98 and C++14.

5.2.6 Increment and decrement [expr.post.incr]
The operand of postfix -- is decremented analogously to the postfix ++ operator, except that the operand
shall not be of type bool. [ Note: For prefix increment and decrement, see 5.3.2. — end note ]

[NoQ]

Nice catch, thanks!

lib/StaticAnalyzer/Core/ExprEngineC.cpp

1078–1086

untill seems to be deprecated in favor of until.

1082

Doesn't isBooleanType() imply CPlusPlus? I guess we need to see if it works in Objective-C++ as well.

test/Analysis/bool.cpp

65 ↗

(On Diff #135820)

dump() exposes too much internals, we try to only use it for debugging, not for tests.

Would eg. clang_analyzer_eval(b == 1) be enough?

[MTC]

• If the operand of the ++ operator is of type _Bool, also set to true.
• Add test file _Bool-increment-decement.c.

[MTC]

Thank you for your review, @NoQ!

• isBooleanType() is used to check _Bool in C99/C11 and bool in C++. For _Bool , there is the same overflow problem.
• In C++98/C++11/C++14, for ++bool and bool+, both sets true directly.
• In C++, --bool and bool-- is illeagal.
• In C99/C11 standard, there is not much information about _Bool++ and _Bool--.

From the implementation of the compiler, _Bool++ and _Bool-- are divided into the following situations.

• _Bool b = 0; b++; // b -> 1
• _Bool b = 1; b++; // b -> 1
• _Bool b = 0; b--; // b -> 1
• _Bool b = 1; b--; // b -> 0

So it's reasonable to set to true if the operand of the increment operator is of type _Bool, just my opinion.

I not familiar with Objective-C++, can you provide a appropriate test about Objective-C++ for me, thank you!

And I'm not a native speaker of English, the grammar of the comments may not be correct. If so, please correct me, thanks!

[NoQ]

LGTM, thank you for the fix!

I not familiar with Objective-C++, can you provide a appropriate test about Objective-C++ for me, thank you!

Now that the explicit check and the respective code path is removed, i don't think we should explicitly test it.

And I'm not a native speaker of English

Same here, i guess.

test/Analysis/_Bool-increment-decrement.c

1–2

We don't write -analyzer-store=region these days because that's the default value and it's unlikely that anybody would ever care.

[MTC]

Remove the default configuration -analyzer-store=region in the test file.

include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h

const llvm::APSInt &getZeroWithPtrWidth(bool isUnsigned = true) {
     return getValue(0, Ctx.getTypeSize(Ctx.VoidPtrTy), isUnsigned);
   }
 
   const llvm::APSInt &getIntWithPtrWidth(uint64_t X, bool isUnsigned) {
     return getValue(X, Ctx.getTypeSize(Ctx.VoidPtrTy), isUnsigned);
   }
 
   const llvm::APSInt &getTruthValue(bool b, QualType T) {
-    return getValue(b ? 1 : 0, Ctx.getTypeSize(T), false);
+    return getValue(b ? 1 : 0, Ctx.getIntWidth(T), true);
   }
 
   const llvm::APSInt &getTruthValue(bool b) {
     return getTruthValue(b, Ctx.getLogicalOperationType());
   }
 
   const CompoundValData *getCompoundValData(QualType T,
                                             llvm::ImmutableList<SVal> Vals);

lib/StaticAnalyzer/Core/ExprEngineC.cpp

for (ExplodedNodeSet::iterator I=Tmp.begin(), E=Tmp.end();I!=E;++I) {
 
     // Handle all other values.
     BinaryOperator::Opcode Op = U->isIncrementOp() ? BO_Add : BO_Sub;
 
     // If the UnaryOperator has non-location type, use its type to create the
     // constant value. If the UnaryOperator has location type, create the
     // constant with int type and pointer width.
     SVal RHS;
+    SVal Result;
 
     if (U->getType()->isAnyPointerType())
       RHS = svalBuilder.makeArrayIndex(1);
     else if (U->getType()->isIntegralOrEnumerationType())
       RHS = svalBuilder.makeIntVal(1, U->getType());
     else
       RHS = UnknownVal();
 
-    SVal Result = evalBinOp(state, Op, V2, RHS, U->getType());
+    // The use of an operand of type bool with the ++ operators is deprecated
+    // but valid until C++17. And if the operand of the ++ operator is of type
+    // bool, it is set to true until C++17. Note that for '_Bool', it is also
+    // set to true when it encounters ++ operator.
+    if (U->getType()->isBooleanType() && U->isIncrementOp())
+      Result = svalBuilder.makeTruthVal(true, U->getType());
+    else
+      Result = evalBinOp(state, Op, V2, RHS, U->getType());
 
     // Conjure a new symbol if necessary to recover precision.
     if (Result.isUnknown()){
       DefinedOrUnknownSVal SymVal =
         svalBuilder.conjureSymbolVal(nullptr, U, LCtx,
                                      currBldrCtx->blockCount());
       Result = SymVal;
 
       // If the value is a location, ++/-- should always preserve
       // non-nullness.  Check if the original value was non-null, and if so
       // propagate that constraint.
       if (Loc::isLocType(U->getType())) {
         DefinedOrUnknownSVal Constraint =
         svalBuilder.evalEQ(state, V2,svalBuilder.makeZeroVal(U->getType()));
 
         if (!state->assume(Constraint, true)) {
           // It isn't feasible for the original value to be null.
           // Propagate this constraint.
           Constraint = svalBuilder.evalEQ(state, SymVal,
                                        svalBuilder.makeZeroVal(U->getType()));
 
-
           state = state->assume(Constraint, false);
           assert(state);
         }
       }
     }
 
     // Since the lvalue-to-rvalue conversion is explicit in the AST,
     // we bind an l-value if the operator is prefix and an lvalue (in C++).

test/Analysis/_Bool-increment-decrement.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c99 -Dbool=_Bool -Dtrue=1 -Dfalse=0 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c11 -Dbool=_Bool -Dtrue=1 -Dfalse=0 %s
+extern void clang_analyzer_eval(bool);
+
+void test__Bool_value() {
+  {
+    bool b = true;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = false;
+    clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = -10;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 10;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 10;
+    b++;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 0;
+    b++;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+}
+
+void test__Bool_increment() {
+  {
+    bool b = true;
+    b++;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = false;
+    b++;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = true;
+    ++b;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = false;
+    ++b;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 0;
+    ++b;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 10;
+    ++b;
+    ++b;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = -10;
+    ++b;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = -1;
+    ++b;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+}
+
+void test__Bool_decrement() {
+  {
+    bool b = true;
+    b--;
+    clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = false;
+    b--;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = true;
+    --b;
+    clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = false;
+    --b;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 0;
+    --b;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 10;
+    --b;
+    clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+    --b;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = -10;
+    --b;
+    clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 1;
+    --b;
+    clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+}

test/Analysis/bool-increment.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c++98 -Wno-deprecated %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c++11 -Wno-deprecated %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c++14 -Wno-deprecated %s
+
+extern void clang_analyzer_eval(bool);
+
+void test_bool_value() {
+  {
+    bool b = true;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = false;
+    clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = -10;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 10;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 10;
+    b++;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 0;
+    b++;
+    clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+}
+
+void test_bool_increment() {
+  {
+    bool b = true;
+    b++;
+    clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = false;
+    b++;
+    clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = true;
+    ++b;
+    clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = false;
+    ++b;
+    clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 0;
+    ++b;
+    clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = 10;
+    ++b;
+    ++b;
+    clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+    bool b = -10;
+    ++b;
+    clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+}