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

[Delesley Hutchins]

This was a really excellent suggestion; thanks!  New (much simpler)
patch is enclosed.

http://codereview.appspot.com/5555057/

  -DeLesley

On Fri, Jan 13, 2012 at 5:07 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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());
>>    }
>
>



-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-function-decldef2.patch
Type: application/octet-stream
Size: 4871 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120119/8f09c397/attachment.obj>