Code review ACK df7a0ab. Code change looks fine, but I think this is a bad approach for a few reasons:

• The approach is more complicated than it needs to be. There is no need to have a macro-wrapped fake lock class just to assert a mutex is locked. The ASSERT_CAPABILITY annotation literally exists for this purpose. If you have a can of beans, I'm sure you can probably open it with a hammer and screwdriver, but you are probably better off using the can opener.

• This is introducing a new macro to do what clang developers specifically told us not to do: "please don't use ACQUIRE when the capability is assumed to be held previously"

• This change makes LOCK_ASSERTION look like safer alternative to AssertLockHeld when actually it more dangerous (it is not checked at compile time unlike AssertLockHeld). It would be nice if the naming could give an indication of which assert should be preferred. (Of course we can get rid of two asserts with scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865).

• Unclear why it's called a "quick fix". It's basically the same size as sync: Replace LockAssertion with AssertLockHeldUnverified #19918 except it has fewer comments and an incomplete description with no stated rationale

Code review ACK df7a0ab. Code change looks fine, but I think this is a bad approach for a few reasons:

* The approach is more complicated than it needs to be. There is no need to have a macro-wrapped fake lock class just to assert a mutex is locked. The ASSERT_CAPABILITY annotation literally exists for this purpose. If you have a can of beans, I'm sure you can probably open it with a hammer and screwdriver, but you are probably better off using the can opener.
* This is introducing a new macro to do what clang developers specifically told us [not to do](https://reviews.llvm.org/D87629#2272676): "please don't use [ACQUIRE](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquire-acquire-shared-release-release-shared-release-generic) when the capability is assumed to be held previously"

Both of those are appropriate justifications for a PR to change the approach. They'd be a valid objection to something that somehow prevents future PRs from changing the approach, but this PR doesn't do that.

* This change makes LOCK_ASSERTION look like safer alternative to AssertLockHeld when actually it more dangerous (it is not checked at compile time unlike AssertLockHeld). It would be nice if the naming could give an indication of which assert should be preferred. (Of course we can get rid of two asserts with #19865).

The developer notes state that using compile-time annotations are to be used consistently, and LOCK_ASSERTION is only to be used when that fails. The implementation of LOCK_ASSERTION as it stands will cause compile errors if you use the wrong assertion. I think you're completely mistaken in thinking LOCK_ASSERTION looks "safer" than "AssertLockHeld".

* Unclear why it's called a "quick fix". It's basically the same size as #19918 except it has fewer comments and an incomplete description with no stated rationale

It's a "fix" because it corrects obviously broken behaviour, and it's "quick" because it leaves the issues where there's debate about what to do them to be addressed elsewhere. This isn't an "alternative" to other approaches as you suggest in your edit to the PR description, it's just a first step.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Replace LockAssertion with AssertLockHeld, remove LockAssertion #19979 (Use proper TSA attributes (attempt two) by hebasto)
• sync: Replace LockAssertion with AssertLockHeldUnverified #19918 (sync: Replace LockAssertion with AssertLockHeldUnverified by ryanofsky)
• scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865 (scripted-diff: Restore AssertLockHeld after Do not hide compile-time thread safety warnings #19668, remove LockAssertion by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

No longer relevant