Page MenuHomePhabricator

delesley (Delesley Hutchins)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 25 2012, 3:32 PM (486 w, 10 h)

Recent Activity

Jun 25 2021

delesley accepted D104649: Thread safety analysis: Rename parameters of ThreadSafetyAnalyzer::intersectAndWarn (NFC).

Looks good. Thanks!

Jun 25 2021, 11:24 AM · Restricted Project
delesley accepted D104261: Thread safety analysis: Always warn when dropping locks on back edges.

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...

Jun 25 2021, 11:24 AM · Restricted Project

May 27 2021

delesley updated subscribers of D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities.
  • 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 27 2021, 10:19 AM · Restricted Project

May 25 2021

delesley accepted D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities.

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

May 25 2021, 12:48 PM · Restricted Project

May 24 2021

delesley added inline comments to D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities.
May 24 2021, 9:15 AM · Restricted Project
delesley added a comment to D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities.

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?

May 24 2021, 9:09 AM · Restricted Project

Apr 6 2021

delesley accepted D98747: Thread safety analysis: Don't warn about managed locks on join points.

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! :-)

Apr 6 2021, 9:35 AM · Restricted Project

Mar 24 2021

delesley added a comment to D98747: Thread safety analysis: Don't warn about managed locks on join points.

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 24 2021, 11:11 AM · Restricted Project

Mar 22 2019

delesley accepted D59523: Thread Safety: also look at ObjC methods.

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

Mar 22 2019, 3:23 PM · Restricted Project, Restricted Project

Nov 29 2018

delesley accepted D52578: Thread safety analysis: Allow scoped releasing of capabilities.

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

Nov 29 2018, 2:04 PM

Nov 15 2018

delesley added inline comments to D52578: Thread safety analysis: Allow scoped releasing of capabilities.
Nov 15 2018, 11:41 AM

Oct 4 2018

delesley accepted D52443: Thread safety analysis: Examine constructor arguments.
Oct 4 2018, 3:51 PM
delesley added inline comments to D52578: Thread safety analysis: Allow scoped releasing of capabilities.
Oct 4 2018, 3:21 PM
delesley added inline comments to D52578: Thread safety analysis: Allow scoped releasing of capabilities.
Oct 4 2018, 3:19 PM
delesley added a comment to D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference.

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!

Oct 4 2018, 12:53 PM · Restricted Project

Sep 27 2018

delesley added inline comments to D52578: Thread safety analysis: Allow scoped releasing of capabilities.
Sep 27 2018, 3:53 PM
delesley added inline comments to D52443: Thread safety analysis: Examine constructor arguments.
Sep 27 2018, 3:32 PM
delesley added a comment to D52443: Thread safety analysis: Examine constructor arguments.

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

Sep 27 2018, 3:27 PM
delesley added a comment to D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference.

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.

Sep 27 2018, 3:22 PM · Restricted Project
delesley added a comment to D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference.

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 27 2018, 3:17 PM · Restricted Project

Sep 21 2018

delesley added a comment to D51187: [RFC] Thread safety analysis: Track status of scoped capability.

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.

Sep 21 2018, 3:44 PM

Sep 19 2018

delesley added inline comments to D51187: [RFC] Thread safety analysis: Track status of scoped capability.
Sep 19 2018, 4:18 PM
delesley accepted D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate.
Sep 19 2018, 12:59 PM
delesley added inline comments to D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate.
Sep 19 2018, 12:59 PM
delesley accepted D51901: Thread Safety Analysis: warnings for attributes without arguments.

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

Sep 19 2018, 12:21 PM

Aug 20 2018

delesley accepted D49885: Thread safety analysis: Allow relockable scopes.

This looks good to me. Thanks for the patch.

Aug 20 2018, 4:13 PM

Jul 25 2018

delesley accepted D49355: Thread safety analysis: Allow lock upgrading and downgrading.

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.)

Jul 25 2018, 2:38 PM

Jan 11 2018

delesley accepted D41933: handle scoped_lockable objects being returned by value in C++17.

LGTM. Thanks, Richard!

Jan 11 2018, 2:00 PM

Dec 14 2017

delesley accepted D41224: [ThreadSafetyAnalysis] Fix isCapabilityExpr.
Dec 14 2017, 2:23 PM · Restricted Project

Aug 8 2017

delesley accepted D36237: Reland "Thread Safety Analysis: fix assert_capability.", add warnings.
Aug 8 2017, 9:17 AM

Aug 7 2017

delesley added a comment to D36237: Reland "Thread Safety Analysis: fix assert_capability.", add warnings.

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 7 2017, 11:45 AM

Aug 1 2017

delesley accepted D36122: Thread Safety Analysis: fix assert_capability..

Thanks!

Aug 1 2017, 12:00 PM

May 5 2017

delesley added a comment to D32917: [clang] ThreadSafetyAnalysis.rst: do not re-define THREAD_ANNOTATION_ATTRIBUTE__ macro.

LOL. Thanks!

May 5 2017, 12:49 PM

Feb 13 2017

delesley added a comment to D29685: Lit C++11 Compatibility - Function Attributes.

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.

Feb 13 2017, 10:19 AM

Jan 26 2017

delesley added a comment to D28520: Disable -Wthread-safety-analysis for some functions in __thread_support.

This looks good to me.

Jan 26 2017, 6:23 AM

Jan 25 2017

delesley added a comment to D28520: Disable -Wthread-safety-analysis for some functions in __thread_support.

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 25 2017, 6:23 AM

Jan 17 2017

delesley added a comment to D28520: Disable -Wthread-safety-analysis for some functions in __thread_support.

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.

Jan 17 2017, 2:41 PM

Sep 29 2015

delesley committed rL248805: Thread Safety Analysis: allow capability attribute on unions..
Thread Safety Analysis: allow capability attribute on unions.
Sep 29 2015, 9:30 AM
delesley committed rL248803: Thread Safety Analysis: fix before/after checks so that they work on global.
Thread Safety Analysis: fix before/after checks so that they work on global
Sep 29 2015, 8:33 AM

Sep 3 2015

delesley committed rL246806: Thread safety analysis: the NO_THREAD_SAFETY_ANALYSIS attribute will now.
Thread safety analysis: the NO_THREAD_SAFETY_ANALYSIS attribute will now
Sep 3 2015, 2:15 PM

Aug 19 2015

delesley added a comment to D12144: Fix 4 typos in lib/Analysis/.

Looks good for the thread safety stuff.

Aug 19 2015, 8:57 AM

Apr 15 2015

delesley added a comment to D9038: Back-Edge Detection Fix.

LGTM. Released as r235051.

Apr 15 2015, 3:37 PM
delesley committed rL235051: Fix for PR20402 in -Wconsumed..
Fix for PR20402 in -Wconsumed.
Apr 15 2015, 3:35 PM

Mar 18 2015

delesley added a comment to D8407: PR22931 - When cloning scopes, reset the scope in Sema instead of using the cloned scope as the current scope.

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?

Mar 18 2015, 8:56 AM

Feb 4 2015

delesley committed rL228194: Thread Safety Analysis: support adopting of locks, as implemented in.
Thread Safety Analysis: support adopting of locks, as implemented in
Feb 4 2015, 1:18 PM
delesley committed rL228176: Thread Safety Analysis: remove minor piece of unused code. No change in.
Thread Safety Analysis: remove minor piece of unused code. No change in
Feb 4 2015, 11:30 AM

Feb 3 2015

delesley committed rL228051: Thread Safety Analysis: add support for before/after annotations on mutexes..
Thread Safety Analysis: add support for before/after annotations on mutexes.
Feb 3 2015, 2:13 PM
delesley committed rL227997: Thread Safety Analysis: add support for before/after annotations on mutexes..
Thread Safety Analysis: add support for before/after annotations on mutexes.
Feb 3 2015, 10:19 AM

May 7 2014

delesley added a comment to D3643: Explicitly keep track of temporaries during the consumed analysis..

LGTM. Thanks for the patch!

May 7 2014, 9:03 AM