Mismatched Clang lock annotations (between prototype and impl) can lead to false negatives

[PiJoules]

Bugzilla Link	42849
Version	trunk
OS	All
Attachments	reproducer
CC	@AaronBallman,@aaronpuchert,@zygoloid

Extended Description

Filing for a coworker

---

As the title says, a mismatch between lock annotations for a function/method prototype and for the function's implementation can lead to false negatives for the static thread analysis feature in clang.

I'll let the example do the talking. Here is a godbolt link which shows the behavior. https://godbolt.org/z/2rCs3U

The code is also explicitly attached to this bug for safe-keeping (I'm not sure how long godbolt links are active for). The summary of the issues are as follows.

1. If you prototype a function and provide no annotations, then you use the function with some requirement, and finally you implement the function with an annotation which conflicts with the usage requirement; clang will accept this when it should not.

2. Same as #​1, but this time you prototype the function with a compatible annotation, then you implement the function with a conflicting annotation.

3. Similar to #​2, but this time you don't even need to bother to use the function. Just prototype it twice with conflicting annotations.

This happens for methods in C++ as well (except for #​3, as C++ does not all you to re-declare methods with identical signatures in the first place, regardless of what annotations are)

IMO, all of these should be compile time failures. Specifically, I think the behavior should be as follows.

1. For a given prototype of a function, all subsequent prototypes should be required to have identical annotations. This is not to say that they have to match character for character, just that if a prototype says that it requires lock instance X, and excludes lock instance Y, then all other prototypes encountered during compilation must agree with the first prototype encountered. This includes the no-annotation state. If the first prototype encountered says no annotations, then all subsequent prototypes must agree.

2. Implementations must follow rule #​1 as well, with the following exception. If an implementation has no annotations, but a prototype has one or more annotations, the implementation is assumed to have the same annotations as the prototype. This is mostly a convenience feature as the vast majority of code I have encountered which uses these thread safety annotations uses C++, and is annotated at the prototype of the methods (and not annotated at all when you hit implementation).

[aaronpuchert]

The documentation explains that almost all annotations (except no_thread_safety_analysis [1]) belong on declarations. [2] It is expected that later declarations (including the definition) inherit the attributes from earlier declarations. So this is an enhancement proposal, not a bug.

We could warn in -Wthread-safety-attributes if a declaration has attributes that weren't visible on an earlier declaration. Multiple non-definition declarations are pretty rare though, and we want to inherit attributes from the declaration into the definition, so I think this is all we can do. Checking that the attributes are the same on all non-definition declarations (1.) would be inconsistent with how you propose to treat definitions (2.) in my opinion.

It wouldn't be very high on my priority list though. The thread safety analysis is not intended to be fool-proof, and it's easy to trick it, if you want to. As long as you keep in mind to have only one non-definition declaration (which is generally good advice) and put all annotations there, you shouldn't run into any issues.

[1] http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-thread-safety-analysis
[2] http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#frequently-asked-questions