[PATCH] D60110: [analyzer] When failing to evaluate a __builtin_constant_p, presume it's false.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, jdoerfert, dkrupp, donat.nagy, kristina, a.sidorin, szepet.
Herald added a project: clang.

`__builtin_constant_p(x)` is a compiler builtin that evaluates to `1` when its argument `x` is a compile-time constant and to `0` otherwise. In CodeGen it is simply lowered to the respective LLVM intrinsic <https://llvm.org/docs/LangRef.html#llvm-is-constant-intrinsic>. In the Analyzer we've been trying to delegate modeling to `Expr::EvaluateAsInt`, which can sometimes fail simply because the expression is too complicated.

When it fails, let's conservatively return `false`. The behavior of this builtin is so unpredictable (in fact, it even depends on optimization level) that any code that's using the builtin should expect anything except maybe a concrete integer to be described by it as "not a constant". Therefore, returning "false" when we're unsure is unlikely to trigger weird execution paths in the code, while returning "unknown (potentially true)" may do so. Which is something i've just seen on some real-world code within a weird macro expansion that tries to emulate compile-time assertions in C by declaring arrays of size -1 when it fails, which triggers the VLA checker to warn if "-1" wasn't in fact a constant. Something like this:

  if (__builtin_constant_p((p == 0))) { // EvaluateAsInt returns Unknown here.
    char __compile_time_assert__[((p == 0)) ? 1 : -1]; // Warning: array of size -1.
    (void)__compile_time_assert__;
  }


Repository:
  rC Clang

https://reviews.llvm.org/D60110

Files:
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/test/Analysis/builtin-functions.cpp


Index: clang/test/Analysis/builtin-functions.cpp
===================================================================
--- clang/test/Analysis/builtin-functions.cpp
+++ clang/test/Analysis/builtin-functions.cpp
@@ -65,19 +65,20 @@
   }
 }
 
-void test_constant_p() {
+void test_constant_p(void *ptr) {
   int i = 1;
   const int j = 2;
   constexpr int k = 3;
   clang_analyzer_eval(__builtin_constant_p(42) == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(j) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k) == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(j + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(" ") == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 0); // expected-warning {{FALSE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(ptr == 0)); // expected-warning {{FALSE}}
 }
Index: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -100,17 +100,25 @@
   case Builtin::BI__builtin_constant_p: {
     // This must be resolvable at compile time, so we defer to the constant
     // evaluator for a value.
+    SValBuilder &SVB = C.getSValBuilder();
     SVal V = UnknownVal();
     Expr::EvalResult EVResult;
     if (CE->EvaluateAsInt(EVResult, C.getASTContext(), Expr::SE_NoSideEffects)) {
       // Make sure the result has the correct type.
       llvm::APSInt Result = EVResult.Val.getInt();
-      SValBuilder &SVB = C.getSValBuilder();
       BasicValueFactory &BVF = SVB.getBasicValueFactory();
       BVF.getAPSIntType(CE->getType()).apply(Result);
       V = SVB.makeIntVal(Result);
     }
 
+    if (FD->getBuiltinID() == Builtin::BI__builtin_constant_p) {
+      // If we didn't manage to figure out if the value is constant or not,
+      // it is safe to assume that it's not constant and unsafe to assume
+      // that it's constant.
+      if (V.isUnknown())
+        V = SVB.makeIntVal(0, CE->getType());
+    }
+
     C.addTransition(state->BindExpr(CE, LCtx, V));
     return true;
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D60110.193225.patch
Type: text/x-patch
Size: 3019 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190402/56196154/attachment.bin>