aaronpuchert added a comment.
In D102026#2777438 <https://reviews.llvm.org/D102026#2777438>, @delesley wrote:
> The warn/not-warn is consistent with more relaxed handling of managed locks,
> but exclusive/shared difference could lead to confusion. Both mechanisms are
> sound; they're just not consistent. Any thoughts?
Essentially I'm suggesting here to treat exclusive/shared joins the same way as
locked/unlocked joins: we allow them for managed locks and take the "weaker"
state afterwards to prevent false negatives, but not for unmanaged locks, where
we take the "stronger" state afterwards to prevent spamming the user.
> Second, the mixing of AssertHeld() and Lock() on different control flow
> branches looks very weird to me. I can see one obvious case where you might
> want to do that, e.g. if (mu_.is_locked()) mu_.AssertHeld(); else mu_.Lock().
This pattern already works, but not with exclusive/shared mismatch.
> However, if a function locks mu_ on one branch, and assert mu_ on the other,
> then that usually indicates a problem. It means that the lock-state going
> into that function was unknown; the programmer didn't know whether mu_ was
> locked or not, and that's never good.
Generally I agree with you. It's something that repeatedly came up though,
people were having these functions that were called sometimes with lock,
sometimes without. It's not incredibly common, but I saw it a few times. I
recommended the above pattern to get rid of the warnings that popped up, when
reworking this into a function with clear preconditions didn't work.
> The //only// valid condition in that case is to check whether mu_ is locked
> -- any other condition would be unsound.
Well, that information might not always come from `mu_` itself. It might be
tracked in the class that owns `mu_`.
> The correct way to deal with this situation is to mark the is_locked() method
> with an attribute, e.g. TEST_LOCKED_FUNCTION, rather than relying on the
> AssertHeld mechanism. That's a lot more work, but I would prefer to at least
> have a TODO comment indicating that the AssertHeld is a workaround,
It certainly is a workaround, but it's powerful enough to cover everything that
we could do with `TEST_LOCKED_FUNCTION`, except that we can't find cases where
the branches have been mixed up.
I've thought about such an attribute (after all it shouldn't be much different
from `try_acquire` implementation-wise), but I've found it hard to justify
because we can just use these assertions to achieve the same thing. You can
even do
if (mu.isLocked())
[]() ASSERT_CAPABILITY(mu) {}();
else
mu.Lock();
> and restrict the mixing of assert/lock to managed locks in the mean time.
I think we should disallow releasing asserted locks, and then naturally we
can't allow assert/lock joins for unmanaged locks. I'm planning another change
for this.
It is the status quo: we allow these joins, just not with mixed mode
(shared/exclusive). So this change will allow shared/exclusive -
asserted/unmanaged joins until my follow-up will disallow them again, though
because of the asserted/unmanaged part and not the shared/exclusive part. If
you're not happy about this gap in warning coverage, we can also postpone this
change until we have the asserted/unmanaged joins disallowed.
================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:4570
mu_.AssertHeld();
- mu_.Unlock();
- } // should this be a warning?
+ mu_.Unlock(); // should this be a warning?
+ }
----------------
delesley wrote:
> This function should have a warning because is not marked with
> UNLOCK_FUNCTION. The lock was held on entry, and not held on exit. So the
> original location of the comment, on the closing curly brace, was correct.
Putting it on the `Unlock` call would some more natural to me, and also more
helpful. Natural, because that's where we (should) discover the issue, and
helpful, because in a longer function you wouldn't know what's going on if the
unlock is somewhere in the middle.
If we didn't have `AssertHeld`, we would also warn on the `Unlock` call, and I
think we should treat this no different (other than having a different warning
message).
================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:4629
+ mu_.AssertHeld(); // expected-note {{the other acquisition of mutex
'mu_' is here}}
+ // FIXME: should instead warn because it's unclear whether we need to
release or not.
+ int b = a;
----------------
delesley wrote:
> I'm not wild about having FIXMEs in test code. I would prefer to have the
> patch actually issue a warning here (but see below).
>
> HOWEVER, does the current code issue a warning in this situation? If not,
> leave it as-is and keep the FIXME. You can't make the analysis more strict
> without checking with the C++ compiler team at Google (which I am no longer
> on), because it may break the build on our end.
>
The comment is that the warning should be different. It's somewhat related with
the other discussion: one shouldn't be able to release asserted locks, and thus
the join here should fail for that reason and not because of a shared/exclusive
mismatch.
Naturally I want to do those things together in one change, that's why I'm not
addressing it here.
I'm not particularly worried that this is going to have a big impact, but we've
had interactions with someone at Google whom I might ask to try it out.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102026/new/
https://reviews.llvm.org/D102026
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
