[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

This revision was automatically updated to reflect the committed changes. Closed by commit rGf702a6fa7c9e: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY (authored by ryanofsky, committed by aaronpuchert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

aaronpuchert accepted this revision. aaronpuchert added a comment. Maybe we'll have to clarify this further in the future, but for now this is an improvement. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87629/new/ https://reviews.llvm.org/D87629

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

aaronpuchert added a comment. In D87629#2280475 , @ajtowns wrote: > Sure, it makes perfect sense that it's too hard. It's not really too hard, there is an existing parameter somewhere in the CFG generation that controls these exception handling edges, an

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

vasild accepted this revision. vasild added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87629/new/ https://reviews.llvm.org/D87629 ___ c

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

aaronpuchert added a comment. Looks pretty good, thanks! I think this clears up the misunderstandings. Since I'm not a native speaker I'd like to wait for @aaron.ballman's opinion before we merge this. Comment at: clang/docs/ThreadSafetyAnalysis.rst:128-135 The set of capabi

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

ryanofsky marked 4 inline comments as done. ryanofsky added a comment. In D87629#2280475 , @ajtowns wrote: > AJ, maybe this discussion could moved to another issue? I find the details hard to follow, so having another issue would be helpful just to unde

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

ajtowns added a comment. In D87629#2278580 , @aaronpuchert wrote: >> Not sure the `try { AssertHeld } catch (...) { a = 0; }` example reveals >> very much: it seems like thread safety annotations aren't checked within a >> catch block at all? >> `void g

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

ryanofsky marked 4 inline comments as done. ryanofsky added a comment. Agree with everything in D87629#2278073 . "Assumed" is the key word, and a wrong assumption doesn't imply UB or generating incorrect code

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

ryanofsky updated this revision to Diff 292576. ryanofsky added a comment. Removed bad information about exceptions, added sentence to clarify what it means for the analysis to "assume" something, tweaked description to only say asserts affect assumptions after calls instead of at or before call

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

aaronpuchert added a comment. In D87629#2278383 , @ajtowns wrote: > Not sure the `try { AssertHeld } catch (...) { a = 0; }` example reveals very > much: it seems like thread safety annotations aren't checked within a catch > block at all? > > Mutex m;

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

ajtowns added a comment. In D87629#2272676 , @aaronpuchert wrote: > (The existing `LockAssertion` class that the change removed looks like it > should use `ASSERT_CAPABILITY` instead—please don't use `ACQUIRE` when the > capability is assumed to be held

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

aaronpuchert added a comment. In D87629#2275639 , @ryanofsky wrote: > The mistakes about exceptions came from me taking "(no return)" in the > previous documentation too literally thinking it was referring to > https://en.cppreference.com/w/cpp/language/

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

ryanofsky added a comment. Great feedback! I need to absorb it all then I'll fix the changeset. The mistakes about exceptions came from me taking "(no return)" in the previous documentation too literally thinking it was referring to https://en.cppreference.com/w/cpp/language/attributes/noreturn

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

vasild added inline comments. Comment at: clang/docs/ThreadSafetyAnalysis.rst:452-454 +and terminating or throwing if the capability is not held. Presence of this +annotation causes the analysis to assume the capability is held at the point of +the call. See :ref:`mutexheader`,

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

aaronpuchert added a comment. You can view it this way: there is a dynamic set and a static set of capabilities. The static set is always the same at any particular point in a function, regardless of the circumstances we're called from. It's what we determine in the analysis. The dynamic set de

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

aaronpuchert added reviewers: aaron.ballman, aaronpuchert. aaronpuchert added a comment. We don't really have a good understanding of `ASSERT_CAPABILITY` ourselves. For example, there is this loophole: void h() { mu.AssertHeld(); mu.Unlock(); } One would expect to get a warning forc

[PATCH] D87629: Thread safety analysis: Improve documentation for ASSERT_CAPABILITY

ryanofsky created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. ryanofsky requested review of this revision. Previous description didn't actually state the effect the attribute has on thread safety analysis (causing analysis to assume the capability is held)