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

[Richard Smith]

Happy I could help. LGTM.

On Thu, January 19, 2012 22:32, Delesley Hutchins wrote:
> 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
>
>