[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 16:30:50 PDT 2011
Ted,
Good point!
Here is a patch that makes the fix; I will commit shortly if it looks ok.
Cheers,
Caitlin
On Mon, Aug 29, 2011 at 4:11 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Hi Caitlin,
>
> Instead of doing:
>
> InGroup<DiagGroup<"thread-safety">>
>
> repeatedly, it might be worth defining that group up front, and then just writing:
>
> InGroup<ThreadSafety>
>
> That would eliminate the possibility of a typo introducing a new warning group (under a different flag).
>
> On Aug 29, 2011, at 3:27 PM, Caitlin Sadowski wrote:
>
>> 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}}
>> +}
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: threadsafetygroup.patch
Type: text/x-patch
Size: 3270 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110829/cb640156/attachment.bin>
More information about the cfe-commits
mailing list