Change thread safety annotations in thread local store code

[krinkinmu]

Commit Message:

When I tried to build Envoy with Clang-18 I hit an issue that Clang thread safety analizer does not like the fact that we are returning a reference to a protected member (central_cache_) from centralCacheLockHeld method.

While I do think that the code is correct, when looking at the thread safety annotations I think we could do a little bit better.

Currently, centralCacheLockHeld is annotated with
ABLS_ASSERT_EXCLUSIVE_LOCK. My understanding is that this annotation should be used on functions that during runtime check that the right lock is held and fail if it's not the case. centralCacheLockHeld currently does not actually check that the lock is held - this seems somewhat misleading and I don't think that thread safety analysis should derive anything from this annotation TBH, as there is no runtime check present there.

We could add a runtime check to the function, but unfortunately it will not be enough to address Clang's warning (see
llvm/llvm-project#123512). Besides I think that we can do slightly better.

This change replaces ABLS_ASSERT_EXCLUSIVE_LOCK with ABSL_EXCLUSIVE_LOCKS_REQUIRED, to let Clang thread safety analysis know during compilation time that this function should be called under a lock.

That change triggered a few more warnings in various places that call into centralCacheLockHeld. In simple cases just adding ABSL_EXCLUSIVE_LOCKS_REQUIRED to the functions that call centralCacheLockHeld was enough.

There were a couple of more complicated cases that Clang could not figure out because it does not support aliasing (i.e., when the same mutex is known under different names, Clang cannot always figure out that different names refer to the same lock). To deal with those cases I added assertLocked method with ABSL_ASSERT_EXCLUSIVE_LOCK(scope->parent_.lock_) and ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_). So on the one hand to call this function Clang should be convinced that lock_ is held, and, on the other hand, it lets Clang know that after this function scope->parent_.lock_ is held.

Given that scope->parent_.lock_ and lock_ are two different names of the same lock, we can avoid doing a runtime check inside the assertLocked method, because if lock_ is held (and Clang is conviced of that) then it follows that scope->parent_.lock_ is also held, because it's the same lock.

All-in-all, I think relying on ABSL_EXCLUSIVE_LOCKS_REQUIRED is slightly better and it addresses the warning for me as well.

Additional Description: Related to the work in #37911
Risk Level: Low
Testing: Tested that things build after the change (plus played around with the code making thread safety analysis warnings to trigger)
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

+cc @phlax

[phlax]

just realized this is not TLS in the ssl sense - its Thread Local Store - reassiging ...

[krinkinmu]

just realized this is not TLS in the ssl sense - its Thread Local Store - reassiging ...

@phlax a good catch, I didn't think about it, changed the name to make it less confusing.

[jmarantz]

IMO the thread annotations are compile-time declarations so the compiler can enforce the intent at compile-time. I don't think you need to use runtime asserts for the thread annotations to make sense. At least that's the way I've been thinking about them.

But I understand the rejection of returning a reference to a guarded member var in a protected method. That's potentially a thread-safety violation from code outside the compilation unit. I wonder if we can resolve this with annotations somehow rather than adding asserts.

RE runtime asserts in this code, I'm wondering if you can say something about whether that compiles to nothing for release builds; it's possible some of this code is performance sensitive.

I'm happy to review this but I wanted to start this dialog first. Thanks!

/wait

[krinkinmu]

@jmarantz I can drop the asserts either completely or in release builds only, however, maybe we can consider a slight variation on top of that (in the spirit of resolving it without asserts), e.g,:

1. In ThreadLocalStoreImpl add a no-op function, let's call it sameLock - it will take ScopeImpl pointer (or scope->parent_.lock_)
2. Annotate sameLock function with ABSL_ASSERT_EXCLUSIVE_LOCK(scope->parent_.lock_) and ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) at the same time.

ABSL_EXCLUSIVE_LOCKS_REQUIRED annotation will ask compiler to check that lock_ is held before calling it and ABSL_ASSERT_EXCLUSIVE_LOCK will let compiler know that scope->parent_.lock is held after that function (and we can be sure of that because lock_ and scope->parent_.lock_ are actually the same thing).

IOW, I'm trying to create a gadget that will effectively let clang know that if lock_ is held it also implies that scope->parent_.lock_ held as well. This way it will be similar to dropping the runtime check (the function will be no-op) with an addition that clang will check that lock_ is held in compile time.

I hope that makes some sense, I will try this approach and see if it works.

[jmarantz]

Sure. I'm OK with the assert calls as you have them if we convince ourselves that during a release build with optimation they disappear completely.

[krinkinmu]

@jmarantz I think I didn't explain well what I wanted to do, anyways, I implemented the idea in #38113 (comment), PTAL.

[krinkinmu]

/retest weird bazel permission error

[krinkinmu]

/retest IO exception when caching successfully built artifacts

[jmarantz]

sweet. just one minor nit.

[jmarantz]

lgtm modulo the one nit.

/wait

[jmarantz]

/assign-from @envoyproxy/senior-maintainers

[repokitteh-read-only]

@envoyproxy/senior-maintainers assignee is @zuercher

🐱

Caused by: a #38113 (review) was submitted by @jmarantz.

see: more, trace.