[cfe-commits] [PATCH] Thread-safety analysis: properly handle attributes on functions with separate definitions.

[Richard Smith]

On Thu, January 5, 2012 21:38, Delesley Hutchins wrote:
> This patch fixes thread safety attributes so that attributes which are
> applied to a function declaration will be properly rewritten when transferred
> to the definition.  E.g.
>
> class Foo { void foo(MyObj *o) EXCLUSIVE_LOCKS_REQUIRED(o->mu_); }
>
> // o->mu_ must be rewritten to obj->mu_ in this scope.
> Foo::foo(MyObj *obj)  { ... }
>
> We do this by storing the original Decl that the attribute was
> attached to.  When examining the attribute later, if the original Decl does
> not match the current Decl, it is rewritten to the current Decl.

I wonder whether storing the original Decl can be avoided. A ParmVarDecl is
aware of its FunctionScopeIndex, which, when combined with a check that the
canonical DeclContext matches, is enough to uniquely identify which parameter
you're referring to. It may also be worth defensively checking the
FunctionScopeDepth matches, although I can't offhand think of any way you can
get a ParmVarDecl with a nontrivial FunctionScopeDepth in your attribute.

> --- a/lib/Analysis/ThreadSafety.cpp
> +++ b/lib/Analysis/ThreadSafety.cpp
> @@ -194,6 +194,26 @@ public:
>      return !DeclSeq.empty();
>    }
>
> +  // Rewrites MutexID from the scope of OldD to the scope of NewD.
> +  // References to parameters in OldD will be replaced with those in NewD.
> +  void rewriteScope(const NamedDecl *OldD, const NamedDecl *NewD) {
> +    const FunctionDecl *OldFD = dyn_cast_or_null<FunctionDecl>(OldD);
> +    const FunctionDecl *NewFD = dyn_cast_or_null<FunctionDecl>(NewD);
> +    if (!OldFD || !NewFD)
> +      return;
> +
> +    unsigned ni = OldFD->getNumParams();
> +    assert(ni == NewFD->getNumParams());
> +
> +    for (unsigned i = 0; i < ni; ++i) {
> +      const Decl *OD = OldFD->getParamDecl(i);
> +      for (unsigned j = 0, nj = DeclSeq.size(); j < nj; ++j) {
> +        if (DeclSeq[j] == OD)

You can avoid the nested loop here by looping over just the DeclSeq and
checking whether Decl is a ParmVarDecl whose DeclContext is OldFD (after
getCanonicalDecl).

> +          DeclSeq[j] = NewFD->getParamDecl(i);
> +      }
> +    }
> +  }
> +
>    /// Issue a warning about an invalid lock expression
>    static void warnInvalidLock(ThreadSafetyHandler &Handler, Expr* MutexExp,
>                                Expr *DeclExp, const NamedDecl* D) {
> @@ -525,6 +545,15 @@ void BuildLockset::checkAccess(Expr *Exp, AccessKind AK) {
>        warnIfMutexNotHeld(D, Exp, AK, GBAttr->getArg(), POK_VarAccess);
>  }
>
> +
> +template <class AT>
> +NamedDecl* getOriginalDecl(AT *Attr, const NamedDecl *D) {
> +  if (NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Attr->getOriginalDecl()))
> +    return ND;
> +  return const_cast<NamedDecl*>(D);

Do you ever create Attr's without an OriginalDecl?

> +}
> +
> +
>  /// \brief Process a function call, method call, constructor call,
>  /// or destructor call.  This involves looking at the attributes on the
>  /// corresponding function/method/constructor/destructor, issuing warnings,
> --- a/lib/Parse/ParseDecl.cpp
> +++ b/lib/Parse/ParseDecl.cpp
> @@ -857,7 +857,7 @@ void Parser::ParseThreadSafetyAttribute(IdentifierInfo
&AttrName,
>      ConsumeToken(); // Eat the comma, move to the next argument
>    }
>    // Match the ')'.
> -  if (ArgExprsOk && !T.consumeClose() && ArgExprs.size() > 0) {
> +  if (ArgExprsOk && !T.consumeClose()) {

Is this to support things like __attribute__((lockable()))? This is fine to go
in separately (with a test).

>      Attrs.addNew(&AttrName, AttrNameLoc, 0, AttrNameLoc, 0, SourceLocation(),
>                   ArgExprs.take(), ArgExprs.size());
>    }