Implement type safety for TLS by tamird · Pull Request #26942 · grpc/grpc

tamird · 2021-08-08T22:27:04Z

I came across these type conversions while investigating https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=68325. It turns out that changing TLS from pthread to stdcpp (#26936) did resolve the UB on Fuchsia.

@ctiller @brunowonka

tamird · 2021-08-09T11:39:26Z

Oops, had some incorrect formatting. Craig, could I get another try run?

tamird · 2021-08-09T14:21:25Z

Some significant changes here to reduce the tls.h API surface. One more kokoro run please, Craig.

src/core/lib/iomgr/timer_generic.cc

src/core/lib/gpr/tls_stdcpp.h

ctiller · 2021-08-09T15:06:20Z

``` template <typename T, typename Ignored> struct Wrapper; template <typename T> struct Wrapper<T, absl::enable_if_t<sizeof(T) <= sizeof(intptr_t)>> { using Type = T; }; #define GPR_THREAD_LOCAL(type) thread_local typename Wrapper<type>::Type ```

ctiller · 2021-08-09T15:09:36Z

maybe better:

template <typename T>
struct Wrapper {
  static_assert(sizeof(T) <= sizeof(intptr_t));
  using Type = T;
};

tamird · 2021-08-09T16:10:23Z

Nice! Done, and made it apply uniformly to all toolchains (not just stdcpp).

src/core/lib/gpr/tls.h

tamird · 2021-08-09T19:29:17Z

Collapsed all the implementations into the same header to avoid having to include tls.h from each one. Otherwise clang-tidy is unhappy:

Error while processing /var/local/git/grpc/src/core/lib/gpr/tls_pthread.h.
src/core/lib/gpr/tls_pthread.h:33:27: error: no template named 'TlsTypeWrapper' [clang-diagnostic-error]
class pthreadTlsWrapper : TlsTypeWrapper<T> {
                          ^
src/core/lib/gpr/tls_pthread.h:39:53: error: use of undeclared identifier 'key_' [clang-diagnostic-error]
  void Init() { GPR_ASSERT(0 == pthread_key_create(&key_, nullptr)); }
                                                    ^
src/core/lib/gpr/tls_pthread.h:42:40: error: use of undeclared identifier 'key_' [clang-diagnostic-error]
    GPR_ASSERT(0 == pthread_key_delete(key_));
                                       ^
src/core/lib/gpr/tls_pthread.h:43:5: error: use of undeclared identifier 'key_' [clang-diagnostic-error]
    key_ = 0;

tamird · 2021-08-10T00:35:34Z

Damn it, compilation failures on Apple systems:

Mon Aug  9 13:57:28 PDT 2021 - In file included from /Volumes/BuildData/tmpfs/src/github/grpc/workspace_objc_macos_opt_native/src/core/lib/iomgr/exec_ctx.h:32:
Mon Aug  9 13:57:28 PDT 2021 - /Volumes/BuildData/tmpfs/src/github/grpc/workspace_objc_macos_opt_native/src/core/lib/gpr/tls.h:123:24: error: no viable conversion from 'int' to 'pthread_attr_t' (aka '_opaque_pthread_attr_t')
Mon Aug  9 13:57:28 PDT 2021 -   pthread_key_t key_ = 0;

Removed initialization of pthread_key_t. Another run, please?

tamird · 2021-08-10T01:34:34Z

More compilation failures, I apparently forgot to include headers needed for pthread when I combined the headers. Should be fixed now.

tamird · 2021-08-10T11:25:13Z

Fixed formatting.

ctiller · 2021-08-10T14:04:07Z

This is good stuff... Will give it one more thorough review after breakfast.

One thing to note/discuss @veblush @nicolasnoble @markdroth -- #26912 passed the portability suites without using the GPR TLS stuff (I'd forgotten about the facility when I drafted the code) -- which warrants the question, do we know that we still need this facility, or would it suffice to just use the C++11 mechanism for supported compilers?

When I built this mechanism we were still very much in C land where the ecosystem here is substantially more fractured.

markdroth · 2021-08-10T14:49:03Z

Assuming that we can use the C++ interface inside of core, that seems like a better option than this GPR API. The only reason I can think of why we might need the GPR API is if it's used by wrapped languages, so I'd suggest checking that.

tamird · 2021-08-10T14:56:47Z

I guess there was a reason at one time: 94c6766

@veblush

ctiller · 2021-08-10T15:21:03Z

Yeah I guess I remembered the iOS thing... was hoping that was old enough to be gone now (is a 2016 phone OS old enough for us to drop support for?)

tamird · 2021-08-10T16:28:10Z

Per the description in #24572:

Configuring port_platform to use PTHREAD_TLS instead of STDCPP_TLS when it's not available. This shouldn't happen to real devices but it can happen when running on the simulator. (e.g. iOS 9 32bits simulator)

So it isn't clear that this is really about phones. Anyway, removed all this stuff in #26960, let's see what the bots say there.

veblush · 2021-08-10T16:56:15Z

Our minimum supported iOS version is 9 so theoretically we should support iOS 9.0 on both 32 bits and 64 bits anyway. We bumped this version (#24282) so I don't think we can bump this again soon without strong reason. This affects all iOS applications relying on gRPC so it should be done carefully. So we should keep iOS 9 32 bits emulation support.

Also Google3 still depends on GPR_PTHREAD_TLS for both Android and iOS so we need to migrate them first. (I have CLs for them but they haven't passed the tests although I'm not sure those failures are valid)

tamird · 2021-08-10T16:58:31Z

Makes sense to me. I can send another PR after this one to retire GPR_GCC_TLS and GPR_MSVC_TLS if that sounds reasonable to folks.

tamird · 2021-08-10T17:33:59Z

Another question: should this move to src/core/lib/gprpp/ since this is a C++ API now?

markdroth · 2021-08-10T17:50:16Z

If this is now a C++ API, I would be in favor of moving the C++ API to src/core/lib/gprpp. If we still need the C99 API for wrapped languages, that part can be in src/core/lib/gpr, but it should just be a wrapper around the C++ API.

tamird · 2021-08-10T17:55:36Z

Wrapped languages do not use this API as far as I can tell. See these searches (all results are from this repo):

Shall I go ahead and move it?

src/core/lib/gpr/tls.h

src/core/lib/gprpp/tls.h

src/core/lib/gpr/tls.h

This is mostly free when compiler support is available, but requires careful templating when implemented using pthread. Significantly slimmed the tls.h interface; it now only defines the "TLS keyword" for each supported compiler, delegating enforcement of correct usage (i.e. must be static) to the compiler itself. Implemented implicit conversion for the pthread wrapper so it can be used (mostly) the same as native support. Notable exception to this is that static_cast<void*> is needed when printing a pointer stored in TLS as %p.

tamird · 2021-08-11T09:55:31Z

CI passed but formatting was off. Fixed now.

src/core/lib/gpr/tls.h

markdroth

LGTM.

I'll let Craig work with you to get this merged.

* Implement type safety for TLS This is mostly free when compiler support is available, but requires careful templating when implemented using pthread. Significantly slimmed the tls.h interface; it now only defines the "TLS keyword" for each supported compiler, delegating enforcement of correct usage (i.e. must be static) to the compiler itself. Implemented implicit conversion for the pthread wrapper so it can be used (mostly) the same as native support. Notable exception to this is that static_cast<void*> is needed when printing a pointer stored in TLS as %p. * Use GPR_THREAD_LOCAL macros consistently

ctiller added kokoro:force-run release notes: no Indicates if PR should not be in release notes labels Aug 9, 2021