Looks good, please commit. =]<br><br><div class="gmail_quote">On Mon, Aug 29, 2011 at 4:30 PM, Caitlin Sadowski <span dir="ltr"><<a href="mailto:supertri@google.com">supertri@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Ted,<br>
<br>
Good point!<br>
<br>
Here is a patch that makes the fix; I will commit shortly if it looks ok.<br>
<br>
Cheers,<br>
<span class="HOEnZb"><font color="#888888"><br>
Caitlin<br>
</font></span><div class="HOEnZb"><div></div><div class="h5"><br>
On Mon, Aug 29, 2011 at 4:11 PM, Ted Kremenek <<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>> wrote:<br>
> Hi Caitlin,<br>
><br>
> Instead of doing:<br>
><br>
>  InGroup<DiagGroup<"thread-safety">><br>
><br>
> repeatedly, it might be worth defining that group up front, and then just writing:<br>
><br>
>  InGroup<ThreadSafety><br>
><br>
> That would eliminate the possibility of a typo introducing a new warning group (under a different flag).<br>
><br>
> On Aug 29, 2011, at 3:27 PM, Caitlin Sadowski wrote:<br>
><br>
>> Author: supertri<br>
>> Date: Mon Aug 29 17:27:51 2011<br>
>> New Revision: 138774<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=138774&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=138774&view=rev</a><br>
>> Log:<br>
>> 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.<br>

>><br>
>> Modified:<br>
>>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
>>    cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp<br>
>>    cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp<br>
>><br>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=138774&r1=138773&r2=138774&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=138774&r1=138773&r2=138774&view=diff</a><br>

>> ==============================================================================<br>
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Aug 29 17:27:51 2011<br>
>> @@ -1404,6 +1404,19 @@<br>
>> def warn_expecting_lock_held_on_loop : Warning<<br>
>>   "expecting lock '%0' to be held at start of each loop">,<br>
>>   InGroup<DiagGroup<"thread-safety">>, DefaultIgnore;<br>
>> +def warn_variable_requires_lock : Warning<<br>
>> +  "accessing variable '%0' requires lock '%1'">,<br>
>> +  InGroup<DiagGroup<"thread-safety">>, DefaultIgnore;<br>
>> +def warn_variable_requires_any_lock : Warning<<br>
>> +  "accessing variable '%0' requires some lock">,<br>
>> +  InGroup<DiagGroup<"thread-safety">>, DefaultIgnore;<br>
>> +def warn_var_deref_requires_lock : Warning<<br>
>> +  "accessing the value pointed to by '%0' requires lock '%1'">,<br>
>> +  InGroup<DiagGroup<"thread-safety">>, DefaultIgnore;<br>
>> +def warn_var_deref_requires_any_lock : Warning<<br>
>> +  "accessing the value pointed to by '%0' requires some lock">,<br>
>> +  InGroup<DiagGroup<"thread-safety">>, DefaultIgnore;<br>
>> +<br>
>><br>
>> def warn_impcast_vector_scalar : Warning<<br>
>>   "implicit conversion turns vector to scalar: %0 to %1">,<br>
>><br>
>> Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=138774&r1=138773&r2=138774&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=138774&r1=138773&r2=138774&view=diff</a><br>

>> ==============================================================================<br>
>> --- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)<br>
>> +++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Mon Aug 29 17:27:51 2011<br>
>> @@ -814,6 +814,9 @@<br>
>>   // Helper functions<br>
>>   void removeLock(SourceLocation UnlockLoc, Expr *LockExp);<br>
>>   void addLock(SourceLocation LockLoc, Expr *LockExp);<br>
>> +  const ValueDecl *getValueDecl(Expr *Exp);<br>
>> +  void checkAccess(Expr *Exp);<br>
>> +  void checkDereference(Expr *Exp);<br>
>><br>
>> public:<br>
>>   BuildLockset(Sema &S, Lockset LS, Lockset::Factory &F)<br>
>> @@ -824,13 +827,15 @@<br>
>>     return LSet;<br>
>>   }<br>
>><br>
>> -  void VisitDeclRefExpr(DeclRefExpr *Exp);<br>
>> +  void VisitUnaryOperator(UnaryOperator *UO);<br>
>> +  void VisitBinaryOperator(BinaryOperator *BO);<br>
>> +  void VisitCastExpr(CastExpr *CE);<br>
>>   void VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp);<br>
>> };<br>
>><br>
>> /// \brief Add a new lock to the lockset, warning if the lock is already there.<br>
>> -/// \param LockExp The lock expression corresponding to the lock to be added<br>
>> /// \param LockLoc The source location of the acquire<br>
>> +/// \param LockExp The lock expression corresponding to the lock to be added<br>
>> void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp) {<br>
>>   LockID Lock(LockExp);<br>
>>   LockData NewLockData(LockLoc);<br>
>> @@ -854,13 +859,124 @@<br>
>>   LSet = NewLSet;<br>
>> }<br>
>><br>
>> -void BuildLockset::VisitDeclRefExpr(DeclRefExpr *Exp) {<br>
>> -  // FIXME: checking for guarded_by/var and pt_guarded_by/var<br>
>> +/// \brief Gets the value decl pointer from DeclRefExprs or MemberExprs<br>
>> +const ValueDecl *BuildLockset::getValueDecl(Expr *Exp) {<br>
>> +  if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Exp))<br>
>> +    return DR->getDecl();<br>
>> +<br>
>> +  if (const MemberExpr *ME = dyn_cast<MemberExpr>(Exp))<br>
>> +    return ME->getMemberDecl();<br>
>> +<br>
>> +  return 0;<br>
>> +}<br>
>> +<br>
>> +/// \brief This method identifies variable dereferences and checks pt_guarded_by<br>
>> +/// and pt_guarded_var annotations. Note that we only check these annotations<br>
>> +/// at the time a pointer is dereferenced.<br>
>> +/// FIXME: We need to check for other types of pointer dereferences<br>
>> +/// (e.g. [], ->) and deal with them here.<br>
>> +/// \param Exp An expression that has been read or written.<br>
>> +void BuildLockset::checkDereference(Expr *Exp) {<br>
>> +  UnaryOperator *UO = dyn_cast<UnaryOperator>(Exp);<br>
>> +  if (!UO || UO->getOpcode() != clang::UO_Deref)<br>
>> +    return;<br>
>> +  Exp = UO->getSubExpr()->IgnoreParenCasts();<br>
>> +<br>
>> +  const ValueDecl *D = getValueDecl(Exp);<br>
>> +  if(!D || !D->hasAttrs())<br>
>> +    return;<br>
>> +<br>
>> +  if (D->getAttr<PtGuardedVarAttr>() && LSet.isEmpty())<br>
>> +    S.Diag(Exp->getExprLoc(), diag::warn_var_deref_requires_any_lock)<br>
>> +      << D->getName();<br>
>> +<br>
>> +  const AttrVec &ArgAttrs = D->getAttrs();<br>
>> +  for(unsigned i = 0, Size = ArgAttrs.size(); i < Size; ++i) {<br>
>> +    if (ArgAttrs[i]->getKind() != attr::PtGuardedBy)<br>
>> +      continue;<br>
>> +    PtGuardedByAttr *PGBAttr = cast<PtGuardedByAttr>(ArgAttrs[i]);<br>
>> +    LockID Lock(PGBAttr->getArg());<br>
>> +    if (!LSet.contains(Lock))<br>
>> +      S.Diag(Exp->getExprLoc(), diag::warn_var_deref_requires_lock)<br>
>> +        << D->getName() << Lock.getName();<br>
>> +  }<br>
>> +}<br>
>> +<br>
>> +/// \brief Checks guarded_by and guarded_var attributes.<br>
>> +/// Whenever we identify an access (read or write) of a DeclRefExpr or<br>
>> +/// MemberExpr, we need to check whether there are any guarded_by or<br>
>> +/// guarded_var attributes, and make sure we hold the appropriate locks.<br>
>> +void BuildLockset::checkAccess(Expr *Exp) {<br>
>> +  const ValueDecl *D = getValueDecl(Exp);<br>
>> +  if(!D || !D->hasAttrs())<br>
>> +    return;<br>
>> +<br>
>> +  if (D->getAttr<GuardedVarAttr>() && LSet.isEmpty())<br>
>> +    S.Diag(Exp->getExprLoc(), diag::warn_variable_requires_any_lock)<br>
>> +      << D->getName();<br>
>> +<br>
>> +  const AttrVec &ArgAttrs = D->getAttrs();<br>
>> +  for(unsigned i = 0, Size = ArgAttrs.size(); i < Size; ++i) {<br>
>> +    if (ArgAttrs[i]->getKind() != attr::GuardedBy)<br>
>> +      continue;<br>
>> +    GuardedByAttr *GBAttr = cast<GuardedByAttr>(ArgAttrs[i]);<br>
>> +    LockID Lock(GBAttr->getArg());<br>
>> +    if (!LSet.contains(Lock))<br>
>> +      S.Diag(Exp->getExprLoc(), diag::warn_variable_requires_lock)<br>
>> +        << D->getName() << Lock.getName();<br>
>> +  }<br>
>> +}<br>
>> +<br>
>> +/// \brief For unary operations which read and write a variable, we need to<br>
>> +/// check whether we hold any required locks. Reads are checked in<br>
>> +/// VisitCastExpr.<br>
>> +void BuildLockset::VisitUnaryOperator(UnaryOperator *UO) {<br>
>> +  switch (UO->getOpcode()) {<br>
>> +    case clang::UO_PostDec:<br>
>> +    case clang::UO_PostInc:<br>
>> +    case clang::UO_PreDec:<br>
>> +    case clang::UO_PreInc: {<br>
>> +      Expr *SubExp = UO->getSubExpr()->IgnoreParenCasts();<br>
>> +      checkAccess(SubExp);<br>
>> +      checkDereference(SubExp);<br>
>> +      break;<br>
>> +    }<br>
>> +    default:<br>
>> +      break;<br>
>> +  }<br>
>> +}<br>
>> +<br>
>> +/// For binary operations which assign to a variable (writes), we need to check<br>
>> +/// whether we hold any required locks.<br>
>> +/// FIXME: Deal with non-primitive types.<br>
>> +void BuildLockset::VisitBinaryOperator(BinaryOperator *BO) {<br>
>> +  if (!BO->isAssignmentOp())<br>
>> +    return;<br>
>> +  Expr *LHSExp = BO->getLHS()->IgnoreParenCasts();<br>
>> +  checkAccess(LHSExp);<br>
>> +  checkDereference(LHSExp);<br>
>> +}<br>
>> +<br>
>> +/// Whenever we do an LValue to Rvalue cast, we are reading a variable and<br>
>> +/// need to ensure we hold any required locks.<br>
>> +/// FIXME: Deal with non-primitive types.<br>
>> +void BuildLockset::VisitCastExpr(CastExpr *CE) {<br>
>> +  if (CE->getCastKind() != CK_LValueToRValue)<br>
>> +    return;<br>
>> +  Expr *SubExp = CE->getSubExpr()->IgnoreParenCasts();<br>
>> +  checkAccess(SubExp);<br>
>> +  checkDereference(SubExp);<br>
>> }<br>
>><br>
>> +<br>
>> /// \brief When visiting CXXMemberCallExprs we need to examine the attributes on<br>
>> /// the method that is being called and add, remove or check locks in the<br>
>> /// lockset accordingly.<br>
>> +///<br>
>> +/// FIXME: For classes annotated with one of the guarded annotations, we need<br>
>> +/// to treat const method calls as reads and non-const method calls as writes,<br>
>> +/// and check that the appropriate locks are held. Non-const method calls with<br>
>> +/// the same signature as const method calls can be also treated as reads.<br>
>> void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) {<br>
>>   NamedDecl *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl());<br>
>><br>
>><br>
>> Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp<br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=138774&r1=138773&r2=138774&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=138774&r1=138773&r2=138774&view=diff</a><br>

>> ==============================================================================<br>
>> --- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)<br>
>> +++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Mon Aug 29 17:27:51 2011<br>
>> @@ -266,7 +266,6 @@<br>
>>     expected-warning {{lock 'aa_mu' is not released at the end of function 'aa_fun_bad_3'}}<br>
>> }<br>
>><br>
>> -<br>
>> //--------------------------------------------------//<br>
>> // Regression tests for unusual method names<br>
>> //--------------------------------------------------//<br>
>> @@ -294,4 +293,117 @@<br>
>>   }<br>
>> };<br>
>><br>
>> +//-----------------------------------------------//<br>
>> +// Errors for guarded by or guarded var variables<br>
>> +// ----------------------------------------------//<br>
>> +<br>
>> +int *pgb_gvar __attribute__((pt_guarded_var));<br>
>> +int *pgb_var __attribute__((pt_guarded_by(sls_mu)));<br>
>> +<br>
>> +class PGBFoo {<br>
>> + public:<br>
>> +  int x;<br>
>> +  int *pgb_field __attribute__((guarded_by(sls_mu2)))<br>
>> +                 __attribute__((pt_guarded_by(sls_mu)));<br>
>> +  void testFoo() {<br>
>> +    pgb_field = &x; // \<br>
>> +      expected-warning {{accessing variable 'pgb_field' requires lock 'sls_mu2'}}<br>
>> +    *pgb_field = x; // expected-warning {{accessing variable 'pgb_field' requires lock 'sls_mu2'}} \<br>
>> +      expected-warning {{accessing the value pointed to by 'pgb_field' requires lock 'sls_mu'}}<br>
>> +    x = *pgb_field; // expected-warning {{accessing variable 'pgb_field' requires lock 'sls_mu2'}} \<br>
>> +      expected-warning {{accessing the value pointed to by 'pgb_field' requires lock 'sls_mu'}}<br>
>> +    (*pgb_field)++; // expected-warning {{accessing variable 'pgb_field' requires lock 'sls_mu2'}} \<br>
>> +      expected-warning {{accessing the value pointed to by 'pgb_field' requires lock 'sls_mu'}}<br>
>> +  }<br>
>> +};<br>
>> +<br>
>> +class GBFoo {<br>
>> + public:<br>
>> +  int gb_field __attribute__((guarded_by(sls_mu)));<br>
>> +<br>
>> +  void testFoo() {<br>
>> +    gb_field = 0; // \<br>
>> +      expected-warning {{accessing variable 'gb_field' requires lock 'sls_mu'}}<br>
>> +  }<br>
>> +};<br>
>> +<br>
>> +GBFoo GlobalGBFoo __attribute__((guarded_by(sls_mu)));<br>
>> +<br>
>> +void gb_fun_0() {<br>
>> +  sls_mu.Lock();<br>
>> +  int x = *pgb_var;<br>
>> +  sls_mu.Unlock();<br>
>> +}<br>
>> +<br>
>> +void gb_fun_1() {<br>
>> +  sls_mu.Lock();<br>
>> +  *pgb_var = 2;<br>
>> +  sls_mu.Unlock();<br>
>> +}<br>
>> +<br>
>> +void gb_fun_2() {<br>
>> +  int x;<br>
>> +  pgb_var = &x;<br>
>> +}<br>
>> +<br>
>> +void gb_fun_3() {<br>
>> +  int *x = pgb_var;<br>
>> +}<br>
>> +<br>
>> +void gb_bad_0() {<br>
>> +  sls_guard_var = 1; // \<br>
>> +    expected-warning {{accessing variable 'sls_guard_var' requires some lock}}<br>
>> +}<br>
>> +<br>
>> +void gb_bad_1() {<br>
>> +  int x = sls_guard_var; // \<br>
>> +    expected-warning {{accessing variable 'sls_guard_var' requires some lock}}<br>
>> +}<br>
>> +<br>
>> +void gb_bad_2() {<br>
>> +  sls_guardby_var = 1; // \<br>
>> +    expected-warning {{accessing variable 'sls_guardby_var' requires lock 'sls_mu'}}<br>
>> +}<br>
>> +<br>
>> +void gb_bad_3() {<br>
>> +  int x = sls_guardby_var; // \<br>
>> +    expected-warning {{accessing variable 'sls_guardby_var' requires lock 'sls_mu'}}<br>
>> +}<br>
>> +<br>
>> +void gb_bad_4() {<br>
>> +  *pgb_gvar = 1; // \<br>
>> +    expected-warning {{accessing the value pointed to by 'pgb_gvar' requires some lock}}<br>
>> +}<br>
>> +<br>
>> +void gb_bad_5() {<br>
>> +  int x = *pgb_gvar; // \<br>
>> +    expected-warning {{accessing the value pointed to by 'pgb_gvar' requires some lock}}<br>
>> +}<br>
>> +<br>
>> +void gb_bad_6() {<br>
>> +  *pgb_var = 1; // \<br>
>> +    expected-warning {{accessing the value pointed to by 'pgb_var' requires lock 'sls_mu'}}<br>
>> +}<br>
>> +<br>
>> +void gb_bad_7() {<br>
>> +  int x = *pgb_var; // \<br>
>> +    expected-warning {{accessing the value pointed to by 'pgb_var' requires lock 'sls_mu'}}<br>
>> +}<br>
>> +<br>
>> +void gb_bad_8() {<br>
>> +  GBFoo G;<br>
>> +  G.gb_field = 0; // \<br>
>> +    expected-warning {{accessing variable 'gb_field' requires lock 'sls_mu'}}<br>
>> +}<br>
>> +<br>
>> +void gb_bad_9() {<br>
>> +  sls_guard_var++; // \<br>
>> +    expected-warning {{accessing variable 'sls_guard_var' requires some lock}}<br>
>> +  sls_guard_var--; // \<br>
>> +    expected-warning {{accessing variable 'sls_guard_var' requires some lock}}<br>
>> +  ++sls_guard_var; // \<br>
>> +    expected-warning {{accessing variable 'sls_guard_var' requires some lock}}<br>
>> +  --sls_guard_var; // \<br>
>> +    expected-warning {{accessing variable 'sls_guard_var' requires some lock}}<br>
>> +}<br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
</div></div></blockquote></div><br>