Ignore assumptions with side-effects

Author: vinay-deshmukh

clang/lib/Analysis/CFG.cpp

@@ -431,9 +431,10 @@ namespace {
 class reverse_children {
   llvm::SmallVector<Stmt *, 12> childrenBuf;
   ArrayRef<Stmt *> children;
+  ASTContext *astContext;
 
 public:
-  reverse_children(Stmt *S);
+  reverse_children(Stmt *S, ASTContext *astContext = nullptr);
 
   using iterator = ArrayRef<Stmt *>::reverse_iterator;
 
@@ -443,7 +444,8 @@ class reverse_children {
 
 } // namespace
 
-reverse_children::reverse_children(Stmt *S) {
+reverse_children::reverse_children(Stmt *S, ASTContext *AstC)
+    : astContext(AstC) {
   if (CallExpr *CE = dyn_cast<CallExpr>(S)) {
     children = CE->getRawSubExprs();
     return;
@@ -457,16 +459,37 @@ reverse_children::reverse_children(Stmt *S) {
       return;
     }
     case Stmt::AttributedStmtClass: {
-      auto Attrs = cast<AttributedStmt>(S)->getAttrs();
+      assert(S->getStmtClass() == Stmt::AttributedStmtClass);
+      assert(this->astContext &&
+             "Attributes need the ast context to determine side-effects");
+      AttributedStmt *AS = cast<AttributedStmt>(S);
+      assert(attrStmt);
+
+      // for an attributed stmt, the "children()" returns only the NullStmt
+      // (;) but semantically the "children" are supposed to be the
+      // expressions _within_ i.e. the two square brackets i.e. [[ HERE ]]
+      // so we add the subexpressions first, _then_ add the "children"
+      for (const Attr *Attr : AS->getAttrs()) {
+        // Only handles [[ assume(<assumption>) ]] right now
+        CXXAssumeAttr const *AssumeAttr = llvm::dyn_cast<CXXAssumeAttr>(Attr);
+        if (!AssumeAttr) {
+          continue;
+        }
+        Expr *AssumeExpr = AssumeAttr->getAssumption();
+        // If we skip adding the assumption expression to CFG,
+        // it doesn't get "branch"-ed by symbol analysis engine
+        // presumably because it's literally not in the CFG
 
-      // We only handle `[[assume(...)]]` attributes for now.
-      if (const auto *A = getSpecificAttr<CXXAssumeAttr>(Attrs)) {
-        childrenBuf.push_back(A->getAssumption());
-        llvm::append_range(childrenBuf, S->children());
-        children = childrenBuf;
-        return;
+        if (AssumeExpr->HasSideEffects(*astContext)) {
+          continue;
+        }
+        childrenBuf.push_back(AssumeExpr);
       }
-      break;
+      // children() for an CXXAssumeAttr is NullStmt(;)
+      // for others, it will have existing behavior
+      llvm::append_range(childrenBuf, AS->children());
+      children = childrenBuf;
+      return;
     }
     default:
       break;
@@ -2436,7 +2459,7 @@ CFGBlock *CFGBuilder::VisitChildren(Stmt *S) {
 
   // Visit the children in their reverse order so that they appear in
   // left-to-right (natural) order in the CFG.
-  reverse_children RChildren(S);
+  reverse_children RChildren(S, Context);
   for (Stmt *Child : RChildren) {
     if (Child)
       if (CFGBlock *R = Visit(Child))

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

@@ -10,16 +10,23 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/AST/StmtCXX.h"
+#include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/ConstructionContext.h"
+#include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Sequence.h"
 #include <optional>
@@ -1209,9 +1216,14 @@ void ExprEngine::VisitAttributedStmt(const AttributedStmt *A,
   ExplodedNodeSet EvalSet;
   StmtNodeBuilder Bldr(CheckerPreStmt, EvalSet, *currBldrCtx);
 
-  if (const auto *AssumeAttr = getSpecificAttr<CXXAssumeAttr>(A->getAttrs())) {
+  for (const auto *attr : A->getAttrs()) {
+    CXXAssumeAttr const *AssumeAttr = llvm::dyn_cast<CXXAssumeAttr>(attr);
+    if (!AssumeAttr) {
+      continue;
+    }
+    Expr *AssumeExpr = AssumeAttr->getAssumption();
     for (ExplodedNode *N : CheckerPreStmt) {
-      Visit(AssumeAttr->getAssumption(), N, EvalSet);
+      Visit(AssumeExpr, N, EvalSet);
     }
   }
 

clang/test/Analysis/out-of-bounds-new.cpp

@@ -4,6 +4,8 @@
 template <typename T> void clang_analyzer_dump(T);
 template <typename T> void clang_analyzer_value(T);
 void clang_analyzer_eval(bool);
+template <typename T>
+void clang_analyzer_explain(T);
 
 // Tests doing an out-of-bounds access after the end of an array using:
 // - constant integer index
@@ -189,22 +191,54 @@ int test_reference_that_might_be_after_the_end(int idx) {
 extern int arrOf10[10];
 void using_builtin(int x) {
   __builtin_assume(x > 101); // CallExpr
-  arrOf10[x] = 404; // expected-warning{{Out of bound access to memory}}
+  arrOf10[x] = 404; // expected-warning {{Out of bound access to memory}}
 }
 
 void using_assume_attr(int ax) {
   [[assume(ax > 100)]]; // NullStmt with an "assume" attribute.
-  arrOf10[ax] = 405; // expected-warning{{Out of bound access to memory}}
+  arrOf10[ax] = 405; // expected-warning {{Out of bound access to memory}}
 }
 
 void using_many_assume_attr(int yx) {
   [[assume(yx > 104), assume(yx > 200), assume(yx < 300)]]; // NullStmt with an attribute
-  arrOf10[yx] = 406; // expected-warning{{Out of bounsssd access to memory}}
+  arrOf10[yx] = 406; // expected-warning{{Out of bound access to memory}}
 }
 
+
+int using_builtin_assume_has_no_sideeffects(int y) {
+  // We should not apply sideeffects of the argument of [[assume(...)]].
+  // "y" should not get incremented;
+  __builtin_assume(++y == 43); // expected-warning {{assumption is ignored because it contains (potential) side-effects}}
+  clang_analyzer_eval(y == 42); // expected-warning {{FALSE}}
+  return y;
+}
+
+
+
 int using_assume_attr_has_no_sideeffects(int y) {
+
   // We should not apply sideeffects of the argument of [[assume(...)]].
-  [[assume(++y == 43)]]; // "y" should not get incremented
+  // "y" should not get incremented;
+  [[assume(++y == 43)]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}}
+ 
   clang_analyzer_eval(y == 42); // expected-warning {{TRUE}} expected-warning {{FALSE}} FIXME: This should be only TRUE.
+
+  clang_analyzer_eval(y == 43); // expected-warning {{FALSE}} expected-warning {{TRUE}} FIXME: This should be only FALSE.
+
   return y;
 }
+
+
+int using_builtinassume_has_no_sideeffects(int u) {
+  // We should not apply sideeffects of the argument of __builtin_assume(...)
+  // "u" should not get incremented;
+  __builtin_assume(++u == 43); // expected-warning {{assumption is ignored because it contains (potential) side-effects}}
+ 
+  // FIXME: evaluate this to true
+  clang_analyzer_eval(u == 42); // expected-warning {{FALSE}}  current behavior 
+
+  // FIXME: evaluate this to false
+  clang_analyzer_eval(u == 43); // expected-warning {{TRUE}}  current behavior 
+
+  return u;
+}