But given that, it makes sense to use AssertLockHeld instead of WeaklyAssertLockHeld to provide runtime checking inside a lambda annotated with EXCLUSIVE_LOCKS_REQUIRED called through a std::function
Both the current LockAssertion and your proposed WeaklyAssertLockHeld do runtime checking, so there’s no advantage there.
I don’t think AssertLockHeld in lambdas ever makes much sense: there are two ways to use lambdas – one where thread safety annotations work as expected, and one where they’re broken because the lambda’s a callback passed to an unannotated function. In the former case you’re writing:
0 LOCK(cs);
1 [&]() EXCLUSIVE_LOCKS_REQUIRED(cs) { ++foo; }();
where it’s already obvious that cs is held when the lambda is called, and you’re just adding the annotation to make the compiler happy.
In the latter case, you’re writing:
0 LOCK(cs_main);
1 connman->ForEachNode([&](CNode* pnode) {
2 WeaklyAssertLockHeld(cs_main);
3 ...
4 });
because you can’t pass the fact that cs_main is still held through the un-annotated ForEachNode.
inside an overridden virtual method annotated EXCLUSIVE_LOCKS_REQUIRED called through a base method which is not annotated.
I think that is something we should go out of our way to avoid. Either the base method should be annotated as requiring the lock, or the overriding method should acquire the lock itself – with annotations, locking is part of the function signature, and you shouldn’t change the signature when overriding a method. We don’t have any code like this now, and I don’t think there’s any reason ever to, so I’m not sure it’s worth addressing it explicitly, but if we are, better to discourage it outright.
