[PATCH] D104649: Thread safety analysis: Rename parameters of ThreadSafetyAnalyzer::intersectAndWarn (NFC)

delesley accepted this revision. delesley added a comment. Looks good. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104649/new/ https://reviews.llvm.org/D104649 ___ cfe-commits mailing list

[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. 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

[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. Thanks for taking the time to discuss things with me. :-) Wrt. to the TEST_LOCKED_FUNCTION, I agree that you can simulate the behavior using Assert and Lock. But that pattern is too

[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

delesley added inline comments. Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:4570 mu_.AssertHeld(); -mu_.Unlock(); - } // should this be a warning? +mu_.Unlock(); // should this be a warning? + } This function should have a

[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

delesley added a comment. 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

[PATCH] D98747: Thread safety analysis: Don't warn about managed locks on join points

delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. 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,

[PATCH] D98747: Thread safety analysis: Don't warn about managed locks on join points

delesley added a comment. 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

[PATCH] D59523: Thread Safety: also look at ObjC methods

delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. The if logic does not enhance readability, but I suppose it can't be helped. Looks good to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59523/new/

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

delesley accepted this revision. delesley added a comment. Just to be clear, I'm approving the change, but I'd still like the methods to be renamed. :-) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D52578/new/ https://reviews.llvm.org/D52578

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

delesley added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:951 +} } else { +// We're removing the underlying mutex. Warn on double unlocking. aaronpuchert wrote: > aaronpuchert wrote: > > delesley wrote: > > > aaronpuchert

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

delesley accepted this revision. delesley added inline comments. This revision is now accepted and ready to land. Comment at: lib/Analysis/ThreadSafety.cpp:2046 const CXXConstructorDecl *D = Exp->getConstructor(); if (D && D->isCopyConstructor()) { const Expr* Source

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

delesley added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:893 private: - SmallVector UnderlyingMutexes; + enum UnderlyingCapabilityKind { +UCK_Acquired, ///< Any kind of acquired capability. aaronpuchert wrote: > delesley wrote: >

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

delesley added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:951 +} } else { +// We're removing the underlying mutex. Warn on double unlocking. aaronpuchert wrote: > delesley wrote: > > I find this very confusing. A lock here

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

delesley added a comment. For future patches, please add Richard Trieu (rtr...@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! BTW, the issue is not just that changes may introduce false

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

delesley added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:893 private: - SmallVector UnderlyingMutexes; + enum UnderlyingCapabilityKind { +UCK_Acquired, ///< Any kind of acquired capability. IMHO, it would make more sense to break

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

delesley added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:2046 const CXXConstructorDecl *D = Exp->getConstructor(); if (D && D->isCopyConstructor()) { const Expr* Source = Exp->getArg(0); As a side note, we should probably special-case

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

delesley added a comment. This looks good, and resolves an outstanding bug that was on my list. Thanks for the patch! Comment at: lib/Analysis/ThreadSafety.cpp:1537 void handleCall(const Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr); + void

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

delesley added a comment. 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

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

delesley added a comment. 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

[PATCH] D51187: [RFC] Thread safety analysis: Track status of scoped capability

delesley added a comment. In https://reviews.llvm.org/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

[PATCH] D51187: [RFC] Thread safety analysis: Track status of scoped capability

delesley added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:929 + return Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc()); +Locked = true; + It's been a while since I last looked at this code, but I don't think you can

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

delesley added inline comments. Comment at: lib/Analysis/ThreadSafetyCommon.cpp:362 + til::Project *P = new (Arena) til::Project(E, D); + if (hasCppPointerType(BE)) +P->setArrow(true); aaronpuchert wrote: > rjmccall wrote: > > aaron.ballman wrote: > > > I

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. This looks okay to me, but I have not tested it on a large code base to see if it breaks anything. Comment at: lib/Sema/SemaDeclAttr.cpp:589 +<< AL <<

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

delesley accepted this revision. delesley added a comment. This looks good to me. Thanks for the patch. Comment at: lib/Analysis/ThreadSafety.cpp:932 + // We're relocking the underlying mutexes. Warn on double locking. + if (FSet.findLock(FactMan, UnderCp)) +

[PATCH] D49355: Thread safety analysis: Allow lock upgrading and downgrading

delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. 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

[PATCH] D41933: handle scoped_lockable objects being returned by value in C++17

delesley accepted this revision. delesley added a comment. LGTM. Thanks, Richard! Repository: rC Clang https://reviews.llvm.org/D41933 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D36237: Reland "Thread Safety Analysis: fix assert_capability.", add warnings

delesley added a comment. 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,

[PATCH] D36122: Thread Safety Analysis: fix assert_capability.

delesley accepted this revision. delesley added a comment. This revision is now accepted and ready to land. Thanks! Repository: rL LLVM https://reviews.llvm.org/D36122 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29685: Lit C++11 Compatibility - Function Attributes

delesley added a comment. 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

[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support

delesley added a comment. This looks good to me. https://reviews.llvm.org/D28520 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support

delesley added a comment. 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

[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support

delesley added a comment. 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