- User Since
- Sep 25 2012, 3:32 PM (584 w, 1 d)
Jun 25 2021
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...
May 27 2021
- 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".
May 25 2021
Thanks for taking the time to discuss things with me. :-)
May 24 2021
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?
Apr 6 2021
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! :-)
Mar 24 2021
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.
Mar 22 2019
The if logic does not enhance readability, but I suppose it can't be helped. Looks good to me.
Nov 29 2018
Just to be clear, I'm approving the change, but I'd still like the methods to be renamed. :-)
Nov 15 2018
Oct 4 2018
For future patches, please add Richard Trieu (email@example.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!
Sep 27 2018
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.
Sep 21 2018
Sep 19 2018
This looks okay to me, but I have not tested it on a large code base to see if it breaks anything.
Aug 20 2018
This looks good to me. Thanks for the patch.
Jul 25 2018
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.)
Jan 11 2018
LGTM. Thanks, Richard!
Dec 14 2017
Aug 8 2017
Aug 7 2017
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.
Aug 1 2017
May 5 2017
Feb 13 2017
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.
Jan 26 2017
This looks good to me.
Jan 25 2017
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.
Jan 17 2017
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.
Sep 29 2015
Sep 3 2015
Aug 19 2015
Looks good for the thread safety stuff.
Apr 15 2015
LGTM. Released as r235051.
Mar 18 2015
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?
Feb 4 2015
Feb 3 2015
May 7 2014
LGTM. Thanks for the patch!