[cfe-commits] r138774 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/AnalysisBasedWarnings.cpp test/SemaCXX/warn-thread-safety-analysis.cpp

[Ted Kremenek]

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