This is an archive of the discontinued LLVM Phabricator instance.

Thread Safety Analysis: warnings for attributes without arguments
ClosedPublic

Authored by aaronpuchert on Sep 10 2018, 5:39 PM.

Details

Summary

When thread safety annotations are used without capability arguments,
they are assumed to apply to this instead. So we warn when either
this doesn't exist, or the class is not a capability type.

This is based on earlier work by Josh Gao that was committed in r310403,
but reverted in r310698 because it didn't properly work in template
classes. See also D36237.

The solution is not to go via the QualType of this, which is then a
template type, hence the attributes are not known because it could be
specialized. Instead we look directly at the class in which we are
contained.

Additionally I grouped two of the warnings together. There are two
issues here: the existence of this, which requires us to be a
non-static member function, and the appropriate annotation on the class
we are contained in. So we don't distinguish between not being in a
class and being static, because in both cases we don't have this.

Fixes PR38399.

Diff Detail

Event Timeline

aaronpuchert created this revision.Sep 10 2018, 5:39 PM

The second warning message is a bit long, so if any of you have a better idea I'd welcome it.

include/clang/Basic/DiagnosticSemaKinds.td
3016–3017

Alternative wording: "%0 attribute without capability arguments refers to 'this', but %1 isn't annotated with 'capability' or 'scoped_lockable' attribute".

jmgao added inline comments.Sep 10 2018, 6:52 PM
include/clang/Basic/DiagnosticSemaKinds.td
3016–3017

Maybe something like "implicit 'this' argument for %0 attribute isn't annotated with 'capability' or 'scoped_lockable" attribute"?

Improved handling of type-dependent base classes and slightly reworded warning message.

aaronpuchert marked an inline comment as done.Sep 11 2018, 6:24 PM
aaronpuchert added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
3016–3017

I like that it's short, but technically we want the argument's type—not the argument itself—to be annotated and I worry that this might not be clear. In the following warning message we talk about the "argument whose type is annotated" for example.

test/SemaCXX/warn-thread-safety-parsing.cpp
1286–1287

The explicit instantiation appears in the AST in its full glory, but I'm not sure how make the analysis run over it. I saw that Sema::ProcessDeclAttributeList is called from Sema::ActOnExplicitInstantiation, but that seems to check the attributes on the class/function itself, not on members.

delesley accepted this revision.Sep 19 2018, 12:21 PM

This looks okay to me, but I have not tested it on a large code base to see if it breaks anything.

lib/Sema/SemaDeclAttr.cpp
589

Curly braces around the else section.

This revision is now accepted and ready to land.Sep 19 2018, 12:21 PM
aaronpuchert marked an inline comment as done.Sep 19 2018, 5:39 PM

This looks okay to me, but I have not tested it on a large code base to see if it breaks anything.

On our code base (not as large as Google's) there was one true positive and no false positives. At least for templates we should be fine now, since we assume that all type-dependent base classes could have any attribute.

This revision was automatically updated to reflect the committed changes.