[analyzer] Add checker to model builtin functions

[xazax.hun]

Summary

Model __builtin_assume. The motivation behind using __builtin_assume from the analyzers point of view is to add assumptions. The conventional way to express invariants is to use asserts (with correct annotations for noreturn functions). However on some platform the users do not want to pay the price of an assert, even in the debug build (especially in a hot code). This way we can express these invariants to the analyzer without runtime overhead. Also it is good to use these constraints even if they are only there to aid the optimizer.

What do you think?

[NoQ]

Hmm, shouldn't this be part of BuiltinFunctionChecker aka core.builtin.BuiltinFunctions? We already have __builtin_assume_aligned here (though it doesn't seem to assume anything because that particular assumption is hard to model).

lib/StaticAnalyzer/Checkers/ModelBuiltinChecker.cpp

49 ↗

(On Diff #98602)

We could also pretend we modeled the call, so that to avoid invalidation.

[xazax.hun]

• Move this to the right checker.

[xazax.hun]

In D33092#752039, @NoQ wrote:

Hmm, shouldn't this be part of BuiltinFunctionChecker aka core.builtin.BuiltinFunctions? We already have __builtin_assume_aligned here (though it doesn't seem to assume anything because that particular assumption is hard to model).

You are completely right! I do not know how could I miss that. :)

[NoQ]

Great, thanks!

lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp

51 ↗

(On Diff #98609)

That's a good question, because assume(false) seems much more broken than assert(false).

But because we are overaggressive on branching as compared to only relying on presumption of code liveness (eg. splitting void foo(x, y) { if (x) ...; if (y) ...; } into 4 paths is unsafe - all code may be live without all paths being feasible - but we do that anyway, same with loops and eager assumptions and our aliasing approach and the whole inlining thing), i think we'd rather not warn.

I mean, due to the above, it is much more likely that the assumption is valid and the branch on which it fails is infeasible due to some function contract we dont' see, than that the assumption is actually violated. We may do well about null dereferences and such, but generally the static analyzer should try to avoid parts of code that have been carefully thought out :) We may probably want to catch cases when the assumption fails on all paths reaching it, but we shouldn't use symbolic execution for all-paths warnings.

cfe/trunk/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp

bool BuiltinFunctionChecker::evalCall(const CallExpr *CE,
   const LocationContext *LCtx = C.getLocationContext();
   if (!FD)
     return false;
 
   switch (FD->getBuiltinID()) {
   default:
     return false;
 
+  case Builtin::BI__builtin_assume: {
+    assert (CE->arg_begin() != CE->arg_end());
+    SVal ArgSVal = state->getSVal(CE->getArg(0), LCtx);
+    if (ArgSVal.isUndef())
+      return true; // Return true to model purity.
+
+    state = state->assume(ArgSVal.castAs<DefinedOrUnknownSVal>(), true);
+    // FIXME: do we want to warn here? Not right now. The most reports might
+    // come from infeasible paths, thus being false positives.
+    if (!state)
+      return true;
+
+    C.addTransition(state);
+    return true;
+  }
+
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
   case Builtin::BI__builtin_assume_aligned:
   case Builtin::BI__builtin_addressof: {
     // For __builtin_unpredictable, __builtin_expect, and
     // __builtin_assume_aligned, just return the value of the subexpression.
     // __builtin_addressof is going from a reference to a pointer, but those
     // are represented the same way in the analyzer.

cfe/trunk/test/Analysis/builtin-assume.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+void f(int i) {
+  __builtin_assume(i < 10);
+  clang_analyzer_eval(i < 15); // expected-warning {{TRUE}}
+}