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

[Aaron Puchert via Phabricator via cfe-commits]

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 forcing the user to add a `RELEASE(mu)`annotation on `h`, but there is none. Should we disallow releasing of asserted capabilities?

In some way, `ASSERT_CAPABILITY` is a bit of an outsider that doesn't really fit in with the other attributes.



================
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
----------------
The assertion is just a promise though, so it could only happen in debug builds, for example.


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


================
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.
----------------
It's implemented a bit differently:

```
Mutex mu;
bool a GUARDED_BY(mu);

void f() {
  a = 0; // warning.
  mu.AssertHeld();
  a = 0; // Ok.
}
```

If we were to assume that `mu` is held at the call site, it would also be held right before it.

I'm not sure if this is the right behavior, but assuming that it is, calling an `ASSERT_CAPABILITY` function introduces an assumption which can be used from that point on, and which disappears on join points with branches that don't have the assumption.


================
Comment at: clang/docs/ThreadSafetyAnalysis.rst:456-457
+
+The given capability must be held on entry to the function, or the function is
+assumed to fail (no return).
 
----------------
"The function is assumed to return only if the given capability is held."?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87629