From 6902f2870a4968f4f9214e7cd19bb2fd10f8747b Mon Sep 17 00:00:00 2001 From: Devin Coughlin Date: Fri, 8 Jul 2016 00:53:18 +0000 Subject: [PATCH] [analyzer] Add rudimentary handling of AtomicExpr. 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. This is rdar://problem/25353187 Differential Revision: http://reviews.llvm.org/D21667 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@274816 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Expr.h | 5 +- .../Core/PathSensitive/ExprEngine.h | 4 + lib/StaticAnalyzer/Core/ExprEngine.cpp | 45 ++++++++- test/Analysis/atomics.c | 95 +++++++++++++++++++ 4 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 test/Analysis/atomics.c diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h index 80710e9021..9179c7736a 100644 --- a/include/clang/AST/Expr.h +++ b/include/clang/AST/Expr.h @@ -4859,9 +4859,12 @@ public: } AtomicOp getOp() const { return Op; } - unsigned getNumSubExprs() { return NumSubExprs; } + unsigned getNumSubExprs() const { return NumSubExprs; } Expr **getSubExprs() { return reinterpret_cast(SubExprs); } + const Expr * const *getSubExprs() const { + return reinterpret_cast(SubExprs); + } bool isVolatile() const { return getPtr()->getType()->getPointeeType().isVolatileQualified(); diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index ec96560a2f..50d8505704 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -392,6 +392,10 @@ public: 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); diff --git a/lib/StaticAnalyzer/Core/ExprEngine.cpp b/lib/StaticAnalyzer/Core/ExprEngine.cpp index 85cdaf2513..83d4b6ab04 100644 --- a/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -902,7 +902,6 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, 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 @@ -1247,6 +1246,12 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred, Bldr.addNodes(Dst); break; + case Stmt::AtomicExprClass: + Bldr.takeNodes(Pred); + VisitAtomicExpr(cast(S), Pred, Dst); + Bldr.addNodes(Dst); + break; + case Stmt::ObjCIvarRefExprClass: Bldr.takeNodes(Pred); VisitLvalObjCIvarRefExpr(cast(S), Pred, Dst); @@ -2070,6 +2075,44 @@ void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred, 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 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; diff --git a/test/Analysis/atomics.c b/test/Analysis/atomics.c new file mode 100644 index 0000000000..f0f5ff0768 --- /dev/null +++ b/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 fully 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}} +} -- 2.40.0