dcoughlin created this revision.
dcoughlin added reviewers: zaks.anna, rsmith.
dcoughlin added a subscriber: cfe-commits.
This proposed patch adds crude handling of atomics to the static analyzer.
Rather than ignore AtomicExprs, as we now do, this patch causes the analyzer
to escape the arguments. This is imprecise -- and we should model the
expressions fully in the future -- but it is less wrong than ignoring their
effects altogether.
Richard: Would you mind reviewing the changes I made to AtomicExpr in the AST?
I had
to add a const accessor for the subexpressions.
Anna: Would you review the static analyzer portion?
This is rdar://problem/25353187
http://reviews.llvm.org/D21667
Files:
include/clang/AST/Expr.h
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/atomics.c
Index: test/Analysis/atomics.c
===================================================================
--- /dev/null
+++ test/Analysis/atomics.c
@@ -0,0 +1,95 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -verify %s
+
+// Tests for c11 atomics. Many of these tests currently yield unknown
+// because we don't folly model the atomics and instead imprecisely
+// treat their arguments as escaping.
+
+typedef unsigned int uint32_t;
+typedef enum memory_order {
+ memory_order_relaxed = __ATOMIC_RELAXED,
+ memory_order_consume = __ATOMIC_CONSUME,
+ memory_order_acquire = __ATOMIC_ACQUIRE,
+ memory_order_release = __ATOMIC_RELEASE,
+ memory_order_acq_rel = __ATOMIC_ACQ_REL,
+ memory_order_seq_cst = __ATOMIC_SEQ_CST
+} memory_order;
+
+void clang_analyzer_eval(int);
+
+struct RefCountedStruct {
+ uint32_t refCount;
+ void *ptr;
+};
+
+void test_atomic_fetch_add(struct RefCountedStruct *s) {
+ s->refCount = 1;
+
+ uint32_t result = __c11_atomic_fetch_add((volatile _Atomic(uint32_t) *)&s->refCount,- 1, memory_order_relaxed);
+
+ // When we model atomics fully this should (probably) be FALSE. It should never
+ // be TRUE (because the operation mutates the passed in storage).
+ clang_analyzer_eval(s->refCount == 1); // expected-warning {{UNKNOWN}}
+
+ // When fully modeled this should be TRUE
+ clang_analyzer_eval(result == 1); // expected-warning {{UNKNOWN}}
+}
+
+void test_atomic_load(struct RefCountedStruct *s) {
+ s->refCount = 1;
+
+ uint32_t result = __c11_atomic_load((volatile _Atomic(uint32_t) *)&s->refCount, memory_order_relaxed);
+
+ // When we model atomics fully this should (probably) be TRUE.
+ clang_analyzer_eval(s->refCount == 1); // expected-warning {{UNKNOWN}}
+
+ // When fully modeled this should be TRUE
+ clang_analyzer_eval(result == 1); // expected-warning {{UNKNOWN}}
+}
+
+void test_atomic_store(struct RefCountedStruct *s) {
+ s->refCount = 1;
+
+ __c11_atomic_store((volatile _Atomic(uint32_t) *)&s->refCount, 2, memory_order_relaxed);
+
+ // When we model atomics fully this should (probably) be FALSE. It should never
+ // be TRUE (because the operation mutates the passed in storage).
+ clang_analyzer_eval(s->refCount == 1); // expected-warning {{UNKNOWN}}
+}
+
+void test_atomic_exchange(struct RefCountedStruct *s) {
+ s->refCount = 1;
+
+ uint32_t result = __c11_atomic_exchange((volatile _Atomic(uint32_t) *)&s->refCount, 2, memory_order_relaxed);
+
+ // When we model atomics fully this should (probably) be FALSE. It should never
+ // be TRUE (because the operation mutates the passed in storage).
+ clang_analyzer_eval(s->refCount == 1); // expected-warning {{UNKNOWN}}
+
+ // When fully modeled this should be TRUE
+ clang_analyzer_eval(result == 1); // expected-warning {{UNKNOWN}}
+}
+
+
+void test_atomic_compare_exchange_strong(struct RefCountedStruct *s) {
+ s->refCount = 1;
+ uint32_t expected = 2;
+ uint32_t desired = 3;
+ _Bool result = __c11_atomic_compare_exchange_strong((volatile _Atomic(uint32_t) *)&s->refCount, &expected, desired, memory_order_relaxed, memory_order_relaxed);
+
+ // For now we expect both expected and refCount to be invalidated by the
+ // call. In the future we should model more precisely.
+ clang_analyzer_eval(s->refCount == 3); // expected-warning {{UNKNOWN}}
+ clang_analyzer_eval(expected == 2); // expected-warning {{UNKNOWN}}
+}
+
+void test_atomic_compare_exchange_weak(struct RefCountedStruct *s) {
+ s->refCount = 1;
+ uint32_t expected = 2;
+ uint32_t desired = 3;
+ _Bool result = __c11_atomic_compare_exchange_weak((volatile _Atomic(uint32_t) *)&s->refCount, &expected, desired, memory_order_relaxed, memory_order_relaxed);
+
+ // For now we expect both expected and refCount to be invalidated by the
+ // call. In the future we should model more precisely.
+ clang_analyzer_eval(s->refCount == 3); // expected-warning {{UNKNOWN}}
+ clang_analyzer_eval(expected == 2); // expected-warning {{UNKNOWN}}
+}
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -898,7 +898,6 @@
case Stmt::CUDAKernelCallExprClass:
case Stmt::OpaqueValueExprClass:
case Stmt::AsTypeExprClass:
- case Stmt::AtomicExprClass:
// Fall through.
// Cases we intentionally don't evaluate, since they don't need
@@ -1243,6 +1242,12 @@
Bldr.addNodes(Dst);
break;
+ case Stmt::AtomicExprClass:
+ Bldr.takeNodes(Pred);
+ VisitAtomicExpr(cast<AtomicExpr>(S), Pred, Dst);
+ Bldr.addNodes(Dst);
+ break;
+
case Stmt::ObjCIvarRefExprClass:
Bldr.takeNodes(Pred);
VisitLvalObjCIvarRefExpr(cast<ObjCIvarRefExpr>(S), Pred, Dst);
@@ -2066,6 +2071,44 @@
getCheckerManager().runCheckersForPostStmt(Dst, EvalSet, M, *this);
}
+void ExprEngine::VisitAtomicExpr(const AtomicExpr *AE, ExplodedNode *Pred,
+ ExplodedNodeSet &Dst) {
+ ExplodedNodeSet AfterPreSet;
+ getCheckerManager().runCheckersForPreStmt(AfterPreSet, Pred, AE, *this);
+
+ // For now, treat all the arguments to C11 atomics as escaping.
+ // FIXME: Ideally we should model the behavior of the atomics precisely here.
+
+ ExplodedNodeSet AfterInvalidateSet;
+ StmtNodeBuilder Bldr(AfterPreSet, AfterInvalidateSet, *currBldrCtx);
+
+ for (ExplodedNodeSet::iterator I = AfterPreSet.begin(), E = AfterPreSet.end();
+ I != E; ++I) {
+ ProgramStateRef State = (*I)->getState();
+ const LocationContext *LCtx = (*I)->getLocationContext();
+
+ SmallVector<SVal, 8> ValuesToInvalidate;
+ for (unsigned SI = 0, Count = AE->getNumSubExprs(); SI != Count; SI++) {
+ const Expr *SubExpr = AE->getSubExprs()[SI];
+ SVal SubExprVal = State->getSVal(SubExpr, LCtx);
+ ValuesToInvalidate.push_back(SubExprVal);
+ }
+
+ State = State->invalidateRegions(ValuesToInvalidate, AE,
+ currBldrCtx->blockCount(),
+ LCtx,
+ /*CausedByPointerEscape*/true,
+ /*Symbols=*/nullptr);
+
+ SVal ResultVal = UnknownVal();
+ State = State->BindExpr(AE, LCtx, ResultVal);
+ Bldr.generateNode(AE, *I, State, nullptr,
+ ProgramPoint::PostStmtKind);
+ }
+
+ getCheckerManager().runCheckersForPostStmt(Dst, AfterInvalidateSet, AE, *this);
+}
+
namespace {
class CollectReachableSymbolsCallback final : public SymbolVisitor {
InvalidatedSymbols Symbols;
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -392,6 +392,10 @@
void VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred,
ExplodedNodeSet &Dst);
+ /// VisitMemberExpr - Transfer function for builtin atomic expressions
+ void VisitAtomicExpr(const AtomicExpr *E, ExplodedNode *Pred,
+ ExplodedNodeSet &Dst);
+
/// Transfer function logic for ObjCAtSynchronizedStmts.
void VisitObjCAtSynchronizedStmt(const ObjCAtSynchronizedStmt *S,
ExplodedNode *Pred, ExplodedNodeSet &Dst);
Index: include/clang/AST/Expr.h
===================================================================
--- include/clang/AST/Expr.h
+++ include/clang/AST/Expr.h
@@ -4886,9 +4886,12 @@
}
AtomicOp getOp() const { return Op; }
- unsigned getNumSubExprs() { return NumSubExprs; }
+ unsigned getNumSubExprs() const { return NumSubExprs; }
Expr **getSubExprs() { return reinterpret_cast<Expr **>(SubExprs); }
+ const Expr * const *getSubExprs() const {
+ return reinterpret_cast<Expr * const *>(SubExprs);
+ }
bool isVolatile() const {
return getPtr()->getType()->getPointeeType().isVolatileQualified();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
