[analyzer] Suppress bugreports from constexpr contexts

[steakhal]

Summary

Assuming that the code compiles, bugreports coming from constexpr
variable initializer expressions are by definition false-positives.
There is code smell-ish bugreports, like
alpha.deadcode.UnreachableCode but suppressing everything doesn't feel
that bad overall.

Ideally, we should outright skip the evaluation of the constexpr
variable initializer expression evaluation, but I could not find a grasp
achieving that.
Since we walk the CFG block, the expression-tree of the initializer is
sequenced before the variable declaration. We could only infer from the
variable declaration that the evaluation of the previous instruction
must have been evaluated in constexpr context.

What we could do is to replace the SVal associated with the
initializer expression, saying that the Expr::EvaluateAsConstantExpr()
is either the same or more accurate than the analyzer engine itself.
According to the analyzer tests, I could not see any difference though.

[NoQ]

Assuming that the code compiles, bugreports coming from constexpr variable initializer expressions are by definition false-positives.

Can you elaborate, like describe specific false positives you've encountered? Why is this specific to variables?

I also wonder if we should be symbolically executing these contexts to begin with, maybe we should rely on the compiler to give us the answer in one step?

[steakhal]

In D117553#3252887, @NoQ wrote:

Assuming that the code compiles, bugreports coming from constexpr variable initializer expressions are by definition false-positives.

Can you elaborate, like describe specific false positives you've encountered?

There are checkers which produce more false-positive reports than others. Such as CStringChecker or OOB access reports.
But if we take one step back we can argue that any reports produced by any checker are highly likely a false-positive. Except for those checkers which are enforcing coding style rules, which won't produce the constexpr evaluator to halt.

Why is this specific to variables?

AFAIK the only way of enforcing the compiler to evaluate some expression in constexpr-context is to initialize a constexpr variable by the expression in question.
If the constexpr evaluator engine finds undefined behavior, the code should not compile. And this is the fact that the analyzer should take into consideration.

I also wonder if we should be symbolically executing these contexts to begin with, maybe we should rely on the compiler to give us the answer in one step?

I agree. However, it doesn't seem to be that easy to achieve.
In the engine AFAIK we evaluate CFGElements in the order they are produced by the CFG builder. So, by the time we arrive at the DeclStmt from which we could query VarDecl->isConstexpr() the exprengine already evaluated all the expressions of the initializer expression.
By inspecting the CFGElement stream it's hard to guess which instruction is the first instruction contributing to the initializer expression.

Prior building the CFG we could set the AddScopes to have markers immediately before the first CFGElement constituting to the initializer expression. The elements of the initializer expression should finish exactly by the time we reach the DeclStmt they are initializing.
We could exploit this to skip these instructions and bind the constexpr evaluated result of the initializer expression by calling Expr::EvaluateAsConstantExpr().

I experimented with this idea, but it seems like the CFG looks surprising to me when I enable the AddScopes option. Unlike its name suggests, the produced CFG will look completely different, most just add the scope-begin and scope-end CFGElements.
I think it's a bug somewhere. There is a flag in the analyzer options basically flipping this flag: -analyzer-config cfg-scopes=true
I assume that the analyzer should behave exactly the same and pass all the tests if we enable this option by default, but it will fail all the tests - even though ExprEngine::processCFGElement() explicitly ignores the ScopeBegin and ScopeEnd entries.
This is surprising as well.

So, I decided to simply suppress these reports. I must admit, I wanted to spare some time and deliver something that is useful for the users for now and tweak these ideas in the future.

[NoQ]

Oh interesting, so it's the same problem that causes ConstructionContexts to be necessary: evaluation of a function depends on its AST parents that we didn't yet encounter in the CFG.

You're saying there's ultimately only one context: local variable initialization. In this case can we simply reuse the ParentMap trick that you have already implemented in this patch (I mean hasAncestor() works through ParentMap, they're all the same to me), but at evaluation-time?

[steakhal]

In D117553#3256398, @NoQ wrote:

Oh interesting, so it's the same problem that causes ConstructionContexts to be necessary: evaluation of a function depends on its AST parents that we didn't yet encounter in the CFG.

You're saying there's ultimately only one context: local variable initialization. In this case can we simply reuse the ParentMap trick that you have already implemented in this patch (I mean hasAncestor() works through ParentMap, they're all the same to me), but at evaluation-time?

Yes, we could implement it if the CFG would not be broken with AddScopes=true. But as of now, suppression is the best we can come up with IMO.

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtCXX.h"
 #include "clang/AST/StmtObjC.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/CFGStmtMap.h"
 #include "clang/Analysis/PathDiagnostic.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
if (!R->isValid())
       break;
 
     NextNode = Pred;
   }
 
   return Notes;
 }
 
+/// Suppresses the bugreport if it happened in constexpr context, where
+/// generally no undefined behavior should occur in a valid source code.
+/// \returns true if it suppressed the bugreport
+static bool suppressReportsFromConstexprContext(PathSensitiveBugReport &BR) {
+  // TODO: Maybe pass the ASTContext as a parameter?
+  ASTContext &ACtx =
+      BR.getErrorNode()->getState()->getStateManager().getContext();
+  const StackFrameContext *Frame =
+      BR.getErrorNode()->getLocationContext()->getStackFrame();
+
+  const StringRef BindName = "decl";
+  using namespace clang::ast_matchers;
+  static const auto Matcher = callExpr(hasAncestor(declStmt().bind(BindName)));
+
+  while (Frame && !Frame->inTopFrame()) {
+    // CallSite is null for destructors.
+    if (const Stmt *CallSite = Frame->getCallSite()) {
+      // Locs are invalid for synthetized call bodies produced by BodyFarm.
+      if (CallSite->getSourceRange().isValid()) {
+        const auto Matches = match(Matcher, *CallSite, ACtx);
+        if (!Matches.empty()) {
+          const auto *DS = Matches[0].getNodeAs<DeclStmt>(BindName);
+          // Assumption: The CFG has one DeclStmt per Decl.
+          const auto *Var = dyn_cast_or_null<VarDecl>(*DS->decl_begin());
+          if (Var && Var->isConstexpr()) {
+            BR.markInvalid("happened in constexpr context", nullptr);
+            return true;
+          }
+        }
+      }
+    }
+    Frame = Frame->getParent()->getStackFrame();
+  }
+  return false;
+}
+
 Optional<PathDiagnosticBuilder> PathDiagnosticBuilder::findValidReport(
     ArrayRef<PathSensitiveBugReport *> &bugReports,
     PathSensitiveBugReporter &Reporter) {
 
   BugPathGetter BugGraph(&Reporter.getGraph(), bugReports);
 
   while (BugPathInfo *BugPath = BugGraph.getNextBugPath()) {
     // Find the BugReport with the original location.
     PathSensitiveBugReport *R = BugPath->Report;
     assert(R && "No original report found for sliced graph.");
     assert(R->isValid() && "Report selected by trimmed graph marked invalid.");
     const ExplodedNode *ErrorNode = BugPath->ErrorNode;
 
+    // Suppress reports materialized within constexpr context.
+    // FIXME: Maybe introduce an analyzer option for disabling this?
+    if (Reporter.getContext().getLangOpts().CPlusPlus)
+      if (suppressReportsFromConstexprContext(*R))
+        return {};
+
     // Register refutation visitors first, if they mark the bug invalid no
     // further analysis is required
     R->addVisitor<LikelyFalsePositiveSuppressionBRVisitor>();
 
     // Register additional node visitors.
     R->addVisitor<NilReceiverBRVisitor>();
     R->addVisitor<ConditionBRVisitor>();
     R->addVisitor<TagVisitor>();

clang/test/Analysis/suppress-from-constexpr-context.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,debug.ExprInspection %s -std=c++17 -verify
+
+constexpr void clang_analyzer_warnIfReached() {}
+
+constexpr int generateReport() {
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  return 0;
+}
+
+constexpr int createBugreportWhichWillBesuppressed() {
+  clang_analyzer_warnIfReached(); // no-warning: suppressed since the callsite is constexpr context
+  return 0;
+}
+
+int TestNonConstexprVarDecl() {
+  int x = generateReport(); // will have a warning
+  return x;
+}
+
+int TestSingleVarDecl() {
+  constexpr int x = createBugreportWhichWillBesuppressed(); // no-warning
+  return x;
+}
+
+int TestMultipleVarDecl() {
+  constexpr int y = createBugreportWhichWillBesuppressed(), // no-warning
+                z = createBugreportWhichWillBesuppressed(); // no-warning
+  return y + z;
+}
+
+int TestCommaExprAndLambdas() {
+  // FIXME: For Eval::Call-ed functions the location context is the
+  // callee's context, so there is no CallSite. The CallSite should
+  // still refer to the DeclStmt.
+  // expected-warning@+2 {{REACHABLE}}
+  constexpr auto f = (
+      clang_analyzer_warnIfReached(), // We should have no warning for this.
+      [](){
+        clang_analyzer_warnIfReached(); // Only evaluated later.
+      }
+    );
+
+  // Now evaluate 'f' in constexpr context.
+  constexpr int x = (f(), 1); // no-warning
+  constexpr int y = [](){ clang_analyzer_warnIfReached(); return 1; }(); // no-warning
+  return x + y;
+}
+
+constexpr int TestDeadCodeTopLevelFn() {
+  if (false) {
+    // Dead code!
+    generateReport(); // expected-warning {{This statement is never executed}}
+    return 42;
+  }
+  return 66;
+}
+
+constexpr int deadcode_callee(bool cond) {
+  if (cond) {
+    // Dead code!
+    // FIXME: We shouldn't suppress deadcode warnings from constexpr contexts.
+    createBugreportWhichWillBesuppressed(); // no-deadcode-warning: it would come from constexpr context
+    return 42;
+  }
+  return 66;
+}
+
+int TestDeadCodeInCallee() {
+  constexpr int x = deadcode_callee(/*cond=*/false); // no-warning
+  return x;
+}