[cfe-commits] r138774 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/AnalysisBasedWarnings.cpp test/SemaCXX/warn-thread-safety-analysis.cpp
Caitlin Sadowski
supertri at google.com
Mon Aug 29 15:27:51 PDT 2011
Author: supertri
Date: Mon Aug 29 17:27:51 2011
New Revision: 138774
URL: http://llvm.org/viewvc/llvm-project?rev=138774&view=rev
Log:
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.
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=138774&r1=138773&r2=138774&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Aug 29 17:27:51 2011
@@ -1404,6 +1404,19 @@
def warn_expecting_lock_held_on_loop : Warning<
"expecting lock '%0' to be held at start of each loop">,
InGroup<DiagGroup<"thread-safety">>, DefaultIgnore;
+def warn_variable_requires_lock : Warning<
+ "accessing variable '%0' requires lock '%1'">,
+ InGroup<DiagGroup<"thread-safety">>, DefaultIgnore;
+def warn_variable_requires_any_lock : Warning<
+ "accessing variable '%0' requires some lock">,
+ InGroup<DiagGroup<"thread-safety">>, DefaultIgnore;
+def warn_var_deref_requires_lock : Warning<
+ "accessing the value pointed to by '%0' requires lock '%1'">,
+ InGroup<DiagGroup<"thread-safety">>, DefaultIgnore;
+def warn_var_deref_requires_any_lock : Warning<
+ "accessing the value pointed to by '%0' requires some lock">,
+ InGroup<DiagGroup<"thread-safety">>, DefaultIgnore;
+
def warn_impcast_vector_scalar : Warning<
"implicit conversion turns vector to scalar: %0 to %1">,
Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=138774&r1=138773&r2=138774&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Mon Aug 29 17:27:51 2011
@@ -814,6 +814,9 @@
// 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 @@
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 @@
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<DeclRefExpr>(Exp))
+ return DR->getDecl();
+
+ if (const MemberExpr *ME = dyn_cast<MemberExpr>(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<UnaryOperator>(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<PtGuardedVarAttr>() && 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<PtGuardedByAttr>(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<GuardedVarAttr>() && 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<GuardedByAttr>(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<NamedDecl>(Exp->getCalleeDecl());
Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=138774&r1=138773&r2=138774&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Mon Aug 29 17:27:51 2011
@@ -266,7 +266,6 @@
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 @@
}
};
+//-----------------------------------------------//
+// 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}}
+}
More information about the cfe-commits
mailing list