From: Caitlin Sadowski Date: Mon, 29 Aug 2011 22:27:51 +0000 (+0000) Subject: Thread safety: added basic handling for pt_guarded_by/var and guarded_by/var annotati... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=05b436ef550837e2141c55c590fb16010b8658d8;p=clang Thread safety: added basic handling for pt_guarded_by/var and guarded_by/var annotations. We identify situations where we are accessing (reading or writing) guarded variables, and report an error if the appropriate locks are not held. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@138774 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 7bf2dc4d9c..5c2768c71e 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -1404,6 +1404,19 @@ def warn_lock_not_released_in_scope : Warning< def warn_expecting_lock_held_on_loop : Warning< "expecting lock '%0' to be held at start of each loop">, InGroup>, DefaultIgnore; +def warn_variable_requires_lock : Warning< + "accessing variable '%0' requires lock '%1'">, + InGroup>, DefaultIgnore; +def warn_variable_requires_any_lock : Warning< + "accessing variable '%0' requires some lock">, + InGroup>, DefaultIgnore; +def warn_var_deref_requires_lock : Warning< + "accessing the value pointed to by '%0' requires lock '%1'">, + InGroup>, DefaultIgnore; +def warn_var_deref_requires_any_lock : Warning< + "accessing the value pointed to by '%0' requires some lock">, + InGroup>, DefaultIgnore; + def warn_impcast_vector_scalar : Warning< "implicit conversion turns vector to scalar: %0 to %1">, diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index 830846df46..f3f168a5c1 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -814,6 +814,9 @@ class BuildLockset : public StmtVisitor { // Helper functions void removeLock(SourceLocation UnlockLoc, Expr *LockExp); void addLock(SourceLocation LockLoc, Expr *LockExp); + const ValueDecl *getValueDecl(Expr *Exp); + void checkAccess(Expr *Exp); + void checkDereference(Expr *Exp); public: BuildLockset(Sema &S, Lockset LS, Lockset::Factory &F) @@ -824,13 +827,15 @@ public: return LSet; } - void VisitDeclRefExpr(DeclRefExpr *Exp); + void VisitUnaryOperator(UnaryOperator *UO); + void VisitBinaryOperator(BinaryOperator *BO); + void VisitCastExpr(CastExpr *CE); void VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp); }; /// \brief Add a new lock to the lockset, warning if the lock is already there. -/// \param LockExp The lock expression corresponding to the lock to be added /// \param LockLoc The source location of the acquire +/// \param LockExp The lock expression corresponding to the lock to be added void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp) { LockID Lock(LockExp); LockData NewLockData(LockLoc); @@ -854,13 +859,124 @@ void BuildLockset::removeLock(SourceLocation UnlockLoc, Expr *LockExp) { LSet = NewLSet; } -void BuildLockset::VisitDeclRefExpr(DeclRefExpr *Exp) { - // FIXME: checking for guarded_by/var and pt_guarded_by/var +/// \brief Gets the value decl pointer from DeclRefExprs or MemberExprs +const ValueDecl *BuildLockset::getValueDecl(Expr *Exp) { + if (const DeclRefExpr *DR = dyn_cast(Exp)) + return DR->getDecl(); + + if (const MemberExpr *ME = dyn_cast(Exp)) + return ME->getMemberDecl(); + + return 0; } +/// \brief This method identifies variable dereferences and checks pt_guarded_by +/// and pt_guarded_var annotations. Note that we only check these annotations +/// at the time a pointer is dereferenced. +/// FIXME: We need to check for other types of pointer dereferences +/// (e.g. [], ->) and deal with them here. +/// \param Exp An expression that has been read or written. +void BuildLockset::checkDereference(Expr *Exp) { + UnaryOperator *UO = dyn_cast(Exp); + if (!UO || UO->getOpcode() != clang::UO_Deref) + return; + Exp = UO->getSubExpr()->IgnoreParenCasts(); + + const ValueDecl *D = getValueDecl(Exp); + if(!D || !D->hasAttrs()) + return; + + if (D->getAttr() && LSet.isEmpty()) + S.Diag(Exp->getExprLoc(), diag::warn_var_deref_requires_any_lock) + << D->getName(); + + const AttrVec &ArgAttrs = D->getAttrs(); + for(unsigned i = 0, Size = ArgAttrs.size(); i < Size; ++i) { + if (ArgAttrs[i]->getKind() != attr::PtGuardedBy) + continue; + PtGuardedByAttr *PGBAttr = cast(ArgAttrs[i]); + LockID Lock(PGBAttr->getArg()); + if (!LSet.contains(Lock)) + S.Diag(Exp->getExprLoc(), diag::warn_var_deref_requires_lock) + << D->getName() << Lock.getName(); + } +} + +/// \brief Checks guarded_by and guarded_var attributes. +/// Whenever we identify an access (read or write) of a DeclRefExpr or +/// MemberExpr, we need to check whether there are any guarded_by or +/// guarded_var attributes, and make sure we hold the appropriate locks. +void BuildLockset::checkAccess(Expr *Exp) { + const ValueDecl *D = getValueDecl(Exp); + if(!D || !D->hasAttrs()) + return; + + if (D->getAttr() && LSet.isEmpty()) + S.Diag(Exp->getExprLoc(), diag::warn_variable_requires_any_lock) + << D->getName(); + + const AttrVec &ArgAttrs = D->getAttrs(); + for(unsigned i = 0, Size = ArgAttrs.size(); i < Size; ++i) { + if (ArgAttrs[i]->getKind() != attr::GuardedBy) + continue; + GuardedByAttr *GBAttr = cast(ArgAttrs[i]); + LockID Lock(GBAttr->getArg()); + if (!LSet.contains(Lock)) + S.Diag(Exp->getExprLoc(), diag::warn_variable_requires_lock) + << D->getName() << Lock.getName(); + } +} + +/// \brief For unary operations which read and write a variable, we need to +/// check whether we hold any required locks. Reads are checked in +/// VisitCastExpr. +void BuildLockset::VisitUnaryOperator(UnaryOperator *UO) { + switch (UO->getOpcode()) { + case clang::UO_PostDec: + case clang::UO_PostInc: + case clang::UO_PreDec: + case clang::UO_PreInc: { + Expr *SubExp = UO->getSubExpr()->IgnoreParenCasts(); + checkAccess(SubExp); + checkDereference(SubExp); + break; + } + default: + break; + } +} + +/// For binary operations which assign to a variable (writes), we need to check +/// whether we hold any required locks. +/// FIXME: Deal with non-primitive types. +void BuildLockset::VisitBinaryOperator(BinaryOperator *BO) { + if (!BO->isAssignmentOp()) + return; + Expr *LHSExp = BO->getLHS()->IgnoreParenCasts(); + checkAccess(LHSExp); + checkDereference(LHSExp); +} + +/// Whenever we do an LValue to Rvalue cast, we are reading a variable and +/// need to ensure we hold any required locks. +/// FIXME: Deal with non-primitive types. +void BuildLockset::VisitCastExpr(CastExpr *CE) { + if (CE->getCastKind() != CK_LValueToRValue) + return; + Expr *SubExp = CE->getSubExpr()->IgnoreParenCasts(); + checkAccess(SubExp); + checkDereference(SubExp); +} + + /// \brief When visiting CXXMemberCallExprs we need to examine the attributes on /// the method that is being called and add, remove or check locks in the /// lockset accordingly. +/// +/// FIXME: For classes annotated with one of the guarded annotations, we need +/// to treat const method calls as reads and non-const method calls as writes, +/// and check that the appropriate locks are held. Non-const method calls with +/// the same signature as const method calls can be also treated as reads. void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) { NamedDecl *D = dyn_cast_or_null(Exp->getCalleeDecl()); diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index 831c26ca3b..cbc947787d 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -266,7 +266,6 @@ void aa_fun_bad_3() { expected-warning {{lock 'aa_mu' is not released at the end of function 'aa_fun_bad_3'}} } - //--------------------------------------------------// // Regression tests for unusual method names //--------------------------------------------------// @@ -294,4 +293,117 @@ class WeirdMethods { } }; +//-----------------------------------------------// +// Errors for guarded by or guarded var variables +// ----------------------------------------------// + +int *pgb_gvar __attribute__((pt_guarded_var)); +int *pgb_var __attribute__((pt_guarded_by(sls_mu))); + +class PGBFoo { + public: + int x; + int *pgb_field __attribute__((guarded_by(sls_mu2))) + __attribute__((pt_guarded_by(sls_mu))); + void testFoo() { + pgb_field = &x; // \ + expected-warning {{accessing variable 'pgb_field' requires lock 'sls_mu2'}} + *pgb_field = x; // expected-warning {{accessing variable 'pgb_field' requires lock 'sls_mu2'}} \ + expected-warning {{accessing the value pointed to by 'pgb_field' requires lock 'sls_mu'}} + x = *pgb_field; // expected-warning {{accessing variable 'pgb_field' requires lock 'sls_mu2'}} \ + expected-warning {{accessing the value pointed to by 'pgb_field' requires lock 'sls_mu'}} + (*pgb_field)++; // expected-warning {{accessing variable 'pgb_field' requires lock 'sls_mu2'}} \ + expected-warning {{accessing the value pointed to by 'pgb_field' requires lock 'sls_mu'}} + } +}; + +class GBFoo { + public: + int gb_field __attribute__((guarded_by(sls_mu))); + + void testFoo() { + gb_field = 0; // \ + expected-warning {{accessing variable 'gb_field' requires lock 'sls_mu'}} + } +}; + +GBFoo GlobalGBFoo __attribute__((guarded_by(sls_mu))); + +void gb_fun_0() { + sls_mu.Lock(); + int x = *pgb_var; + sls_mu.Unlock(); +} + +void gb_fun_1() { + sls_mu.Lock(); + *pgb_var = 2; + sls_mu.Unlock(); +} + +void gb_fun_2() { + int x; + pgb_var = &x; +} + +void gb_fun_3() { + int *x = pgb_var; +} + +void gb_bad_0() { + sls_guard_var = 1; // \ + expected-warning {{accessing variable 'sls_guard_var' requires some lock}} +} + +void gb_bad_1() { + int x = sls_guard_var; // \ + expected-warning {{accessing variable 'sls_guard_var' requires some lock}} +} + +void gb_bad_2() { + sls_guardby_var = 1; // \ + expected-warning {{accessing variable 'sls_guardby_var' requires lock 'sls_mu'}} +} + +void gb_bad_3() { + int x = sls_guardby_var; // \ + expected-warning {{accessing variable 'sls_guardby_var' requires lock 'sls_mu'}} +} + +void gb_bad_4() { + *pgb_gvar = 1; // \ + expected-warning {{accessing the value pointed to by 'pgb_gvar' requires some lock}} +} + +void gb_bad_5() { + int x = *pgb_gvar; // \ + expected-warning {{accessing the value pointed to by 'pgb_gvar' requires some lock}} +} + +void gb_bad_6() { + *pgb_var = 1; // \ + expected-warning {{accessing the value pointed to by 'pgb_var' requires lock 'sls_mu'}} +} + +void gb_bad_7() { + int x = *pgb_var; // \ + expected-warning {{accessing the value pointed to by 'pgb_var' requires lock 'sls_mu'}} +} + +void gb_bad_8() { + GBFoo G; + G.gb_field = 0; // \ + expected-warning {{accessing variable 'gb_field' requires lock 'sls_mu'}} +} + +void gb_bad_9() { + sls_guard_var++; // \ + expected-warning {{accessing variable 'sls_guard_var' requires some lock}} + sls_guard_var--; // \ + expected-warning {{accessing variable 'sls_guard_var' requires some lock}} + ++sls_guard_var; // \ + expected-warning {{accessing variable 'sls_guard_var' requires some lock}} + --sls_guard_var; // \ + expected-warning {{accessing variable 'sls_guard_var' requires some lock}} +}