From efa1ec75cf152991a64cedf9c4a1ec5cb826ee34 Mon Sep 17 00:00:00 2001 From: "Romain Geissler @ Amadeus" Date: Tue, 6 Jun 2023 10:49:55 -0700 Subject: [PATCH 1/2] Use the same ABI for static and shared libraries on non-Windows platforms (#12983) Hi, It seems that until last year, the logic behind `PROTOBUF_USE_DLLS` was for Windows (MSCV) only. It was changed to all platforms here in https://github.com/protocolbuffers/protobuf/commit/5a0887fc6529596eff5c0f72febc602a9d494cc2 Last month, the generated pkg config files were updated to reflect the protobuf build-time value of `PROTOBUF_USE_DLLS` as it was indeed noted that it changes the ABI. This was done in https://github.com/protocolbuffers/protobuf/pull/12700 In the commit message it is mentionned that most likely we shall rather have a stable ABI. Finally in https://github.com/protocolbuffers/protobuf/issues/12746 which at some point mentions https://issuetracker.google.com/issues/283987730#comment7 where a Google employee hits the linker issue: ``` undefined reference to `google::protobuf::internal::ThreadSafeArena::thread_cache_' ``` which denotes a mix of some .o or libs built `PROTOBUF_USE_DLLS` defined and some others build with `PROTOBUF_USE_DLLS` undefined, resulting in ABI incompatibilities. I also hit this issue while trying to include protobuf in a corporate environment using it's own proprietary build system in which it is expected that .a and .so use a compatible ABI. From my own understanding, ideally we should always use `thread_local` variables, but experience has shown that: - old iOS (iOS < 9) didn't seem to accept `thread_local`, leading to the `GOOGLE_PROTOBUF_NO_THREADLOCAL` macro later renamed `PROTOBUF_NO_THREADLOCAL` which allowed to disable this, but it is not set anywhere in the protobuf code base. Also I doubt you still want to support such old iOS now, so maybe you should consider removing all `PROTOBUF_NO_THREADLOCAL` related code paths (this pull request doesn't do this). - MSVC's DLL interface doesn't seem to accept exporting thread local variables (at least from what I understood, I know absolutely nothing about the Windows ecosystem), yet we can "hide" a thread local variable in a static function using a thread local variable. However in that case the access to TLS variable is not inlined, leading to worse performances, this hack shall be done only for Windows (actually when using MSVC) *AND* we build a shared library. - In all other cases, a classical `thread_local` shall be used, no matter if we build a static or a shared library. In particular on Linux which I guess is the target Google cares the more about for its own production. This pull request achieves this. Am I right in my conclusion ? Closes #12983 COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/12983 from Romain-Geissler-1A:stable-abi-use-dll-non-windows dc23ff50f67cf0c8e45900a78700d1fc3e8bec39 PiperOrigin-RevId: 538230923 --- src/google/protobuf/arena.cc | 2 +- src/google/protobuf/reflection_mode.cc | 2 +- src/google/protobuf/reflection_mode.h | 10 ++++++---- src/google/protobuf/thread_safe_arena.h | 6 +++--- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/google/protobuf/arena.cc b/src/google/protobuf/arena.cc index 51afdbdafe5d2..6577a3669217a 100644 --- a/src/google/protobuf/arena.cc +++ b/src/google/protobuf/arena.cc @@ -519,7 +519,7 @@ ThreadSafeArena::ThreadCache& ThreadSafeArena::thread_cache() { new internal::ThreadLocalStorage(); return *thread_cache_->Get(); } -#elif defined(PROTOBUF_USE_DLLS) +#elif defined(PROTOBUF_USE_DLLS) && defined(_MSC_VER) ThreadSafeArena::ThreadCache& ThreadSafeArena::thread_cache() { static PROTOBUF_THREAD_LOCAL ThreadCache thread_cache; return thread_cache; diff --git a/src/google/protobuf/reflection_mode.cc b/src/google/protobuf/reflection_mode.cc index 7611c7b69cdda..2f6eec286eabe 100644 --- a/src/google/protobuf/reflection_mode.cc +++ b/src/google/protobuf/reflection_mode.cc @@ -39,7 +39,7 @@ namespace internal { #if !defined(PROTOBUF_NO_THREADLOCAL) -#if defined(PROTOBUF_USE_DLLS) +#if defined(PROTOBUF_USE_DLLS) && defined(_MSC_VER) ReflectionMode& ScopedReflectionMode::reflection_mode() { static PROTOBUF_THREAD_LOCAL ReflectionMode reflection_mode = ReflectionMode::kDefault; diff --git a/src/google/protobuf/reflection_mode.h b/src/google/protobuf/reflection_mode.h index e9ff3408f0cfc..5e1f237a3500c 100644 --- a/src/google/protobuf/reflection_mode.h +++ b/src/google/protobuf/reflection_mode.h @@ -95,18 +95,20 @@ class PROTOBUF_EXPORT ScopedReflectionMode final { private: #if !defined(PROTOBUF_NO_THREADLOCAL) const ReflectionMode previous_mode_; -#if defined(PROTOBUF_USE_DLLS) +#if defined(PROTOBUF_USE_DLLS) && defined(_MSC_VER) + // Thread local variables cannot be exposed through MSVC DLL interface but we + // can wrap them in static functions. static ReflectionMode& reflection_mode(); #else PROTOBUF_CONSTINIT static PROTOBUF_THREAD_LOCAL ReflectionMode reflection_mode_; -#endif // PROTOBUF_USE_DLLS +#endif // PROTOBUF_USE_DLLS && _MSC_VER #endif // !PROTOBUF_NO_THREADLOCAL }; #if !defined(PROTOBUF_NO_THREADLOCAL) -#if defined(PROTOBUF_USE_DLLS) +#if defined(PROTOBUF_USE_DLLS) && defined(_MSC_VER) inline ScopedReflectionMode::ScopedReflectionMode(ReflectionMode mode) : previous_mode_(reflection_mode()) { @@ -140,7 +142,7 @@ inline ReflectionMode ScopedReflectionMode::current_reflection_mode() { return reflection_mode_; } -#endif // PROTOBUF_USE_DLLS +#endif // PROTOBUF_USE_DLLS && _MSC_VER #else diff --git a/src/google/protobuf/thread_safe_arena.h b/src/google/protobuf/thread_safe_arena.h index 5a527bc63724e..5b5b4e6eb69c2 100644 --- a/src/google/protobuf/thread_safe_arena.h +++ b/src/google/protobuf/thread_safe_arena.h @@ -261,9 +261,9 @@ class PROTOBUF_EXPORT ThreadSafeArena { // iOS does not support __thread keyword so we use a custom thread local // storage class we implemented. static ThreadCache& thread_cache(); -#elif defined(PROTOBUF_USE_DLLS) - // Thread local variables cannot be exposed through DLL interface but we can - // wrap them in static functions. +#elif defined(PROTOBUF_USE_DLLS) && defined(_MSC_VER) + // Thread local variables cannot be exposed through MSVC DLL interface but we + // can wrap them in static functions. static ThreadCache& thread_cache(); #else PROTOBUF_CONSTINIT static PROTOBUF_THREAD_LOCAL ThreadCache thread_cache_; From adf892293a5d7bdc6a2a130650d70ecf035f4869 Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Mon, 5 Jun 2023 10:00:39 -0700 Subject: [PATCH 2/2] Sync up the min os versions between the C++ and ObjC CocoaPods support. Fixes #12944 PiperOrigin-RevId: 537907016 --- Protobuf-C++.podspec | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Protobuf-C++.podspec b/Protobuf-C++.podspec index bc3ba77394411..78b92d83996ba 100644 --- a/Protobuf-C++.podspec +++ b/Protobuf-C++.podspec @@ -25,10 +25,10 @@ Pod::Spec.new do |s| s.header_mappings_dir = 'src' - s.ios.deployment_target = '7.0' - s.osx.deployment_target = '10.9' - s.tvos.deployment_target = '9.0' - s.watchos.deployment_target = '2.0' + s.ios.deployment_target = '12.0' + s.osx.deployment_target = '10.13' + s.tvos.deployment_target = '12.0' + s.watchos.deployment_target = '6.0' s.pod_target_xcconfig = { # Do not let src/google/protobuf/stubs/time.h override system API