[cfe-commits] [cfe-dev] Proposal for thread safety attributes for Clang

[Caitlin Sadowski]

Doug,

Thanks for reviewing the patch! Here is an updated patch, with
comments inline. Let me know if it is ok to submit, or if you have any
other edits.

> I don't see the reason for this change:
>
> --- a/lib/Sema/SemaDeclAttr.cpp
> +++ b/lib/Sema/SemaDeclAttr.cpp
> @@ -27,7 +27,7 @@ using namespace sema;
>
>  /// These constants match the enumerated choices of
>  /// warn_attribute_wrong_decl_type and err_attribute_wrong_decl_type.
> -enum AttributeDeclType {
> +enum {

I added the name back in, but called it AttributeDeclKind. (The name
was introduced in the prior thread safety patch, but was in the end
not used.)

> LLVM style generally puts the '*' next to the declarator-id, e.g.,
>
>        RecordDecl *RD

Fixed.

> Usually written as isa<LockableAttr>(ArgAttrs[i])

Fixed.

> +/// \brief Thread Safety Analysis: Checks that all attribute arguments, starting
> +/// from Sidx, resolve to a lockable object. May flag an error.
> +static bool checkAttrArgsAreLockableObjs(Sema & S, Decl *D,
>
> You probably want to check whether ArgExpr->isTypeDependent() here, and then just "continue" if it is, because when the argument type is dependent, you'll have no way to determine whether it's a lockable type or not.

Good point. Fixed.

> +    // now check if we idx into a record type function param
> +    if (!RT && ParamIdxOk) {
> +      FunctionDecl * FD = dyn_cast <FunctionDecl>(D);
> +      IntegerLiteral * IL = dyn_cast<IntegerLiteral>(ArgExp);
>
> You want exactly integer literals here? No extra parentheses, no "1+2", just an integer literal?

Yes. Only integer literals.

> +        RT = FD->getParamDecl(ParamIdx_from0)->getType()->getAs<RecordType>();
>
> Don't you want to look through parameters of reference or pointer type here?

Yes! Rearranged code to fix this.

> +/// \brief Thread Safety Analysis: Check if passed in RecordType was annotated
> +/// with the 'lockable' attribute. Argument should be non-null.
> +static bool isLockable(const RecordType * RT) {
> This whole function collapses down to
>
>        RT->getDecl()->getAttr<LockableAttr>();

That is cleaner; fixed.

> @@ -371,6 +441,21 @@ static void handleAcquireOrderAttr(Sema &S, Decl *D, const AttributeList &Attr,
>     return;
>   }
>
> +  // check that this attribute only applies to lockable types
> +  if (ValueDecl *VD = dyn_cast<ValueDecl>(D)) {
> +    if (const RecordType * RT = VD->getType()->getAs<RecordType>()) {
> +      if (!isLockable(RT)) {
> +        S.Diag(Attr.getLoc(), diag::err_attribute_decl_not_lockable)
> +          << Attr.getName();
> +        return;
> +      }
> +    }
> +  }
>
> The comment and code don't agree. The code is checking that the type of the value is either not a RecordType or is a lockable record type.
>
> If you do intend this to do what the comment says, you should also allow dependent types (which occur in templates; the attribute will need to get checked again when the variable is instantiated). Also, do you want to permit variables of reference-to-lockable or pointer-to-lockable type?\

I reorganized the code and fixed these issues.

> -  if (!isFunction(D)) {
> +  if (!isa<FunctionDecl>(D)) {
>
> This happens a number of times. You probably also want to allow FunctionTemplateDecl, and at some point we'll also want ObjCMethodDecl.

I added a check for isa<FunctionTemplateDecl> for now.

Cheers,

Caitlin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: threadsafety_finishingattrparsing.patch
Type: text/x-patch
Size: 62525 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110805/e960804e/attachment.bin>