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

This revision was automatically updated to reflect the committed changes. Closed by commit rL349300: Thread safety analysis: Allow scoped releasing of capabilities (authored by aaronpuchert, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE LAST ACTION

[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

pwnall added a comment. In D52578#1307145 , @aaronpuchert wrote: > To be clear, I'm not a big fan of this change myself, I just wanted to see if > it was feasible. My personal opinion, as I wrote in the bug report, is that > scoped releasing of mutexes

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

aaronpuchert added a comment. > As the analysis grew more complex, I switched to the current system based on > "facts". There are a number of facts that are potentially useful in static > analysis, such as whether one expression aliases another, and most of them > don't look at all like capabil

[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 wr

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

aaronpuchert updated this revision to Diff 171026. aaronpuchert added a comment. Negative capabilities don't need a LockKind. Repository: rC Clang https://reviews.llvm.org/D52578 Files: lib/Analysis/ThreadSafety.cpp test/SemaCXX/warn-thread-safety-analysis.cpp Index: test/SemaCXX/warn-t

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

aaronpuchert updated this revision to Diff 171019. aaronpuchert added a comment. Introduced helper functions to clarify lock handling. The previous version was too tightly coupled, and the introduction of AddCp and RemoveCp didn't help readability. Repository: rC Clang https://reviews.llvm.

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

aaronpuchert added a comment. I hope the cleanup makes the code more easily digestible, and to some extent might also transport why I think this is the most elegant approach. I think we should document the semantics of scoped capabilities in more detail, and I will do so once this is either mer

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

aaronpuchert updated this revision to Diff 170545. aaronpuchert marked 3 inline comments as done. aaronpuchert added a comment. This revision is now accepted and ready to land. Addressed some review comments and simplified the code. There is a lot less duplication and maybe it's even easier to un

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

pwnall added a comment. Thank you for clarifying, Aaron! I probably used Phabricator incorrectly. My intent was to state that the tests LGTM and express support for the functionality in this patch. Please definitely address all review comments before landing. Repository: rC Clang https://r

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

aaronpuchert planned changes to this revision. aaronpuchert added a comment. I think I'll try to simplify this and address @delesley's comments before we commit this. I'll admit that the semantics are somewhat counter-intuitive, but as I explained I think it's more consistent this way. Because t

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

pwnall accepted this revision. pwnall added a comment. This revision is now accepted and ready to land. test/SemaCXX/warn-thread-safety-analysis.cpp LGTM -- this is the functionality that we need in Chrome to use thread safety annotations for AutoUnlock. very non-authoritative opinion - I think

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

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

[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] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert added a comment. Maybe you should have a look at the tests first. I thought about the semantics that I think you are suggesting, but then we could have code like: class SCOPED_LOCKABLE MutexLockUnlock { public: MutexLockUnlock(Mutex *mu1, Mutex *mu2) EXCLUSIVE_UNLOCK_FUNCTIO

[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] D52578: Thread safety analysis: Allow scoped releasing of capabilities

aaronpuchert added a comment. See the bug for further discussion. I'm not sure if we want to have this, but if the pattern is used more widely it might make sense. It blows up the code a bit, although I hope that https://reviews.llvm.org/D51187 might reduce it again. Repository: rC Clang ht

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

aaronpuchert created this revision. aaronpuchert added reviewers: aaron.ballman, delesley. Herald added a subscriber: cfe-commits. The pattern is problematic with C++ exceptions, and not as widespread as scoped locks, but it's still used by some, for example Chromium. We are a bit stricter here a