Looks good. Thanks!

This looks good to me. Thanks for the patch! Since you're adding a new warning, this may break some code somewhere, but since it's restricted to relockable managed locks, I'm not too worried...

• The assert_capability attribute is also a bit of a backdoor. Instead

of statically propagating through the code that a mutex is held, we can
just get that fact "out of thin air".

Thanks for taking the time to discuss things with me. :-)

I have a few concerns. First, this patch introduces an inconsistency between managed and unmanaged locks. For unmanaged locks, we warn, and then assume the lock is held exclusively. For managed locks, we don't warn, but assume it is held shared. The warn/not-warn is consistent with more relaxed handling of managed locks, but exclusive/shared difference could lead to confusion. Both mechanisms are sound; they're just not consistent. Any thoughts?

I am convinced by your argument. I think it is reasonable to assume that if someone is using an RAII object, then the underlying object is responsible for managing double locks/unlocks, and we can restrict our efforts to detecting race conditions. Thanks for the detailed explanation! :-)

Thanks for roping me into the conversation, Aaron, and sorry about the delay. I have mixed feelings about this patch. With respect to the purpose of thread safety analysis, finding race conditions is obviously a major concern, because race conditions are hard to find in tests. However, preventing deadlocks is almost as important, and can be even more important in certain critical applications. The consequence of a double unlock is usually a crash or an exception with an error message. The consequence of a double lock is often a deadlock, depending on the underlying thread library, which is a more serious problem that is harder to fix. Thus, I am concerned about the change in the test case on line 2895, where a potential deadlock has gone from detected, to no warning. Deadlocks are often not found in tests, because test coverage is never perfect over all possible control-flow paths.

The if logic does not enhance readability, but I suppose it can't be helped. Looks good to me.

Just to be clear, I'm approving the change, but I'd still like the methods to be renamed. :-)

For future patches, please add Richard Trieu (rtrieu@google.com) as a subscriber. I will continue to try and do code reviews, but Richard is on the team that actually rolls out new compiler changes. Thanks!

This looks good, and resolves an outstanding bug that was on my list. Thanks for the patch!

With respect to data, I really think these patches should be tested against Google's code base, because otherwise you're going to start seeing angry rollbacks. However, I don't work on the C++ team any more, and I don't have time to do it. When I was actively developing the analysis, I spent about 90% of my time just running tests and fixing the code base. Each incremental improvement in the analysis itself was a hard upward slog. If you're going to be adding lots of improvements, we need to have someone at Google running point.

I have mixed feelings about this patch. When you pass an object to a function, either by pointer or by reference, no actual load from memory has yet occurred. Thus, there is a real risk of false positives; what you are saying is that the function *might* read or write from protected memory, not that it actually will. In fact, the false positive rate is high enough that this particular warning is sequestered under -Wthread-safety-reference, and is not used with warnings-as-errors at Google.

In D51187#1241354, @aaronpuchert wrote:

Thanks for pointing out my error! Ignoring the implementation for a moment, do you think this is a good idea or will it produce too many false positives? Releasable/relockable scoped capabilities that I have seen keep track of the status, so it makes sense, but maybe there are others out there.

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

This looks good to me. Thanks for the patch.

Looks good to me. Thanks for the patch, and my apologies for the slow response. (I'm on a different project now, so I'm afraid thread safety doesn't always get the attention from me that it deserves.)

LGTM. Thanks, Richard!

Overall looks good. However, I would change the wording on the warning to the following. The terms "free function" and "instance method" may be confusing to some people. Also, warn_thread_attribute_noargs_static_method should not mention the capability attribute, which is checked separately in warn_thread_attribute_noargs_not_lockable.

Thanks!

LOL. Thanks!

I agree with Aaron here. Those thread safety errors are supposed to fire; simply disabling the unit tests because they no longer fire is not acceptable. I also don't understand how this could be a bug with the thread safety analysis, since these particular errors are issued when the attributes are parsed, not when the analysis is run. I haven't looked at the relevant code in a long time; have there been significant changes to the attribute-parsing mechanism? There's no member initialization going on here, so why would changes to member initialization have this side effect? I'm confused.

This looks good to me.

The big question for me is whether these functions are exposed as part of the public libcxx API, or whether they are only used internally. (Again, I'm not a libcxx expert.) If they are part of the public API, then users may want to switch them on and off in their own code. In that case, I'm happy with the current variant.

Sorry about the slow response. My main concern here is that the thread safety analysis was designed for use with a library that wraps the system mutex in a separate Mutex class. We did that specifically to avoid breaking anything; code has to opt-in to the static checking by defining and using a Mutex class, and the API of that class is restricted to calls that can be easily checked via annotations. Including attributes directly in the standard library has the potential to cause lots of breakage and false positives.

Looks good for the thread safety stuff.

LGTM. Released as r235051.

Nice catch! LGTM. BTW, I'm surprised it has taken so long for this bug
to manifest, since we should have lots of code with multiple
late parsed attributes. When did it show up?

LGTM. Thanks for the patch!