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

[Russell Yanofsky via Phabricator via cfe-commits]

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

Previous description was also ambiguous about (or slightly overstated) the
noreturn assumption made by thread safety analysis, implying the assumption had
to be true about the function's behavior in general, and not just its behavior
in places where it's used. Stating the assumption specifically should avoid a
perceived need to disable thread safety analysis in places where only asserting
that a specific capability is held would be better. (For an example, see
https://github.com/bitcoin/bitcoin/pull/19929)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87629

Files:
  clang/docs/ThreadSafetyAnalysis.rst


Index: clang/docs/ThreadSafetyAnalysis.rst
===
--- clang/docs/ThreadSafetyAnalysis.rst
+++ clang/docs/ThreadSafetyAnalysis.rst
@@ -447,10 +447,14 @@
 
 *Previously:*  ``ASSERT_EXCLUSIVE_LOCK``, ``ASSERT_SHARED_LOCK``
 
-These are attributes on a function or method that does a run-time test to see
-whether the calling thread holds the given capability.  The function is assumed
-to fail (no return) if the capability is not held.  See :ref:`mutexheader`,
-below, for example uses.
+These are attributes on a function or method which asserts the calling thread
+already holds the given capability, for example by performing a run-time test
+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`, below, for example uses.
+
+The given capability must be held on entry to the function, or the function is
+assumed to fail (no return).
 
 
 GUARDED_VAR and PT_GUARDED_VAR


Index: clang/docs/ThreadSafetyAnalysis.rst
===
--- clang/docs/ThreadSafetyAnalysis.rst
+++ clang/docs/ThreadSafetyAnalysis.rst
@@ -447,10 +447,14 @@
 
 *Previously:*  ``ASSERT_EXCLUSIVE_LOCK``, ``ASSERT_SHARED_LOCK``
 
-These are attributes on a function or method that does a run-time test to see
-whether the calling thread holds the given capability.  The function is assumed
-to fail (no return) if the capability is not held.  See :ref:`mutexheader`,
-below, for example uses.
+These are attributes on a function or method which asserts the calling thread
+already holds the given capability, for example by performing a run-time test
+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`, below, for example uses.
+
+The given capability must be held on entry to the function, or the function is
+assumed to fail (no return).
 
 
 GUARDED_VAR and PT_GUARDED_VAR
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Yanofsky via Phabricator via cfe-commits]

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.

Re: "I got the impression that you want both runtime checks and compile-time 
checks in parallel". Personally I only want the compile checks, and would like 
to drop all the runtime checks except in handful of places where compile time 
checks don't work. ASSERT_CAPABILITY does a great job in these places so I 
don't want people to be scared off from using it unduly because of 
documentation!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87629/new/

https://reviews.llvm.org/D87629

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Yanofsky via Phabricator via cfe-commits]

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 calls, dropped 
"no return" sentence because it's redundant and could still imply annotation 
shouldn't be used with NDEBUG asserts that do return


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87629/new/

https://reviews.llvm.org/D87629

Files:
  clang/docs/ThreadSafetyAnalysis.rst


Index: clang/docs/ThreadSafetyAnalysis.rst
===
--- clang/docs/ThreadSafetyAnalysis.rst
+++ clang/docs/ThreadSafetyAnalysis.rst
@@ -144,6 +144,9 @@
 attributes; example definitions can be found in :ref:`mutexheader`, below.
 The following documentation assumes the use of macros.
 
+The attributes only control assumptions made by thread safety analysis and the
+warnings it issues.  They don't affect generated code or behavior at run-time.
+
 For historical reasons, prior versions of thread safety used macro names that
 were very lock-centric.  These macros have since been renamed to fit a more
 general capability model.  The prior names are still in use, and will be
@@ -447,10 +450,11 @@
 
 *Previously:*  ``ASSERT_EXCLUSIVE_LOCK``, ``ASSERT_SHARED_LOCK``
 
-These are attributes on a function or method that does a run-time test to see
-whether the calling thread holds the given capability.  The function is assumed
-to fail (no return) if the capability is not held.  See :ref:`mutexheader`,
-below, for example uses.
+These are attributes on a function or method which asserts the calling thread
+already holds the given capability, for example by performing a run-time test
+and terminating if the capability is not held.  Presence of this annotation
+causes the analysis to assume the capability is held after calls to the
+annotated function.  See :ref:`mutexheader`, below, for example uses.
 
 
 GUARDED_VAR and PT_GUARDED_VAR


Index: clang/docs/ThreadSafetyAnalysis.rst
===
--- clang/docs/ThreadSafetyAnalysis.rst
+++ clang/docs/ThreadSafetyAnalysis.rst
@@ -144,6 +144,9 @@
 attributes; example definitions can be found in :ref:`mutexheader`, below.
 The following documentation assumes the use of macros.
 
+The attributes only control assumptions made by thread safety analysis and the
+warnings it issues.  They don't affect generated code or behavior at run-time.
+
 For historical reasons, prior versions of thread safety used macro names that
 were very lock-centric.  These macros have since been renamed to fit a more
 general capability model.  The prior names are still in use, and will be
@@ -447,10 +450,11 @@
 
 *Previously:*  ``ASSERT_EXCLUSIVE_LOCK``, ``ASSERT_SHARED_LOCK``
 
-These are attributes on a function or method that does a run-time test to see
-whether the calling thread holds the given capability.  The function is assumed
-to fail (no return) if the capability is not held.  See :ref:`mutexheader`,
-below, for example uses.
+These are attributes on a function or method which asserts the calling thread
+already holds the given capability, for example by performing a run-time test
+and terminating if the capability is not held.  Presence of this annotation
+causes the analysis to assume the capability is held after calls to the
+annotated function.  See :ref:`mutexheader`, below, for example uses.
 
 
 GUARDED_VAR and PT_GUARDED_VAR
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Yanofsky via Phabricator via cfe-commits]

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 
. Also 
comment is good summary of the compile-time and run-time checking tradeoffs.




Comment at: clang/docs/ThreadSafetyAnalysis.rst:450
 
-These are attributes on a function or method that does a run-time test to see
-whether the calling thread holds the given capability.  The function is assumed
-to fail (no return) if the capability is not held.  See :ref:`mutexheader`,
-below, for example uses.
+These are attributes on a function or method which asserts the calling thread
+already holds the given capability, for example by performing a run-time test

aaronpuchert wrote:
> The assertion is just a promise though, so it could only happen in debug 
> builds, for example.
> The assertion is just a promise though, so it could only happen in debug 
> builds, for example.

This is true, but the statements seem compatible to me. I'd be happy to update 
it if I know what change to make.




Comment at: clang/docs/ThreadSafetyAnalysis.rst:452
+already holds the given capability, for example by performing a run-time test
+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

vasild wrote:
> aaronpuchert wrote:
> > That's an interesting point. We currently do assume that the capability is 
> > held even when an exception is thrown, although that's primarily because 
> > Clang's source-level CFG doesn't really model exception handling that well:
> > 
> > ```
> > Mutex mu;
> > bool a GUARDED_BY(mu);
> > 
> > void except() {
> >   try {
> > mu.AssertHeld();
> >   } catch (...) {
> > a = 0; // No warning.
> >   }
> > }
> > ```
> > 
> > We can't warn there because `AssertHeld` might terminate in case the mutex 
> > isn't held.
> > 
> > Not sure if we need to clarify this here: we just assume that the 
> > capability is held on a regular return. The other case is basically UB for 
> > us.
> Maybe don't mention "throwing", given the example above and `s/at the 
> call/after the call/`.
> It is similar to:
> 
> ```
> void f(char x, unsigned char y)
> {
> assert(x >= 0);
> assert(y <= 127);
> if (x > y) { // it is ok to omit a warning about comparing signed and 
> unsigned because AFTER the asserts we know we are good.
> ...
> }
> }
> ```
> That's an interesting point.

Removed language about throwing now. That was just my misunderstanding.



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`, below, for example uses.

ryanofsky wrote:
> vasild wrote:
> > aaronpuchert wrote:
> > > That's an interesting point. We currently do assume that the capability 
> > > is held even when an exception is thrown, although that's primarily 
> > > because Clang's source-level CFG doesn't really model exception handling 
> > > that well:
> > > 
> > > ```
> > > Mutex mu;
> > > bool a GUARDED_BY(mu);
> > > 
> > > void except() {
> > >   try {
> > > mu.AssertHeld();
> > >   } catch (...) {
> > > a = 0; // No warning.
> > >   }
> > > }
> > > ```
> > > 
> > > We can't warn there because `AssertHeld` might terminate in case the 
> > > mutex isn't held.
> > > 
> > > Not sure if we need to clarify this here: we just assume that the 
> > > capability is held on a regular return. The other case is basically UB 
> > > for us.
> > Maybe don't mention "throwing", given the example above and `s/at the 
> > call/after the call/`.
> > It is similar to:
> > 
> > ```
> > void f(char x, unsigned char y)
> > {
> > assert(x >= 0);
> > assert(y <= 127);
> > if (x > y) { // it is ok to omit a warning about comparing signed and 
> > unsigned because AFTER the asserts we know we are good.
> > ...
> > }
> > }
> > ```
> > That's an interesting point.
> 
> Removed language about throwing now. That was just my misunderstanding.
> Maybe don't mention "throwing", given the example above and `s/at the 
> call/after the call/`.

Thanks, update has both changes



Comment at: clang/docs/ThreadSafetyAnalysis.rst:453
+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`, below, for example uses.

aaronpuchert wrote:
> It's implemented a bit differently:
> 
> ```
> Mutex mu;
> bool a GUARDED_BY(mu);
> 
> void f() {
>   a = 0; // war

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

[Russell Yanofsky via Phabricator via cfe-commits]

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 understand what 
changes you might be requesting. Of course if I am doing something wrong with 
changes in this changeset definitely let me know.

> I mean, if I'm using ASSERT_CAPABILITY in the first place, it's because I'm 
> already getting false positives.

It probably would help to have a precise definition of "false positive" but I 
don't think the main use of ASSERT_CAPABILITY is to eliminate mistakes in the 
analysis. I'd say the main use is to provide the analysis new information it 
doesn't have another way to know about. More specifically the purpose is to add 
a capability from the dynamic capability set to the static set (D87629#2272676 
)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87629/new/

https://reviews.llvm.org/D87629

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits