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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
