Skip to content

Commit efa1ec7

Browse files
Romain-Geissler-1Afowles
authored andcommitted
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 5a0887f 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 #12700 In the commit message it is mentionned that most likely we shall rather have a stable ABI. Finally in #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=#12983 from Romain-Geissler-1A:stable-abi-use-dll-non-windows dc23ff5 PiperOrigin-RevId: 538230923
1 parent 727ece5 commit efa1ec7

File tree

4 files changed

+11
-9
lines changed

4 files changed

+11
-9
lines changed

src/google/protobuf/arena.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -519,7 +519,7 @@ ThreadSafeArena::ThreadCache& ThreadSafeArena::thread_cache() {
519519
new internal::ThreadLocalStorage<ThreadCache>();
520520
return *thread_cache_->Get();
521521
}
522-
#elif defined(PROTOBUF_USE_DLLS)
522+
#elif defined(PROTOBUF_USE_DLLS) && defined(_MSC_VER)
523523
ThreadSafeArena::ThreadCache& ThreadSafeArena::thread_cache() {
524524
static PROTOBUF_THREAD_LOCAL ThreadCache thread_cache;
525525
return thread_cache;

src/google/protobuf/reflection_mode.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ namespace internal {
3939

4040
#if !defined(PROTOBUF_NO_THREADLOCAL)
4141

42-
#if defined(PROTOBUF_USE_DLLS)
42+
#if defined(PROTOBUF_USE_DLLS) && defined(_MSC_VER)
4343
ReflectionMode& ScopedReflectionMode::reflection_mode() {
4444
static PROTOBUF_THREAD_LOCAL ReflectionMode reflection_mode =
4545
ReflectionMode::kDefault;

src/google/protobuf/reflection_mode.h

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,18 +95,20 @@ class PROTOBUF_EXPORT ScopedReflectionMode final {
9595
private:
9696
#if !defined(PROTOBUF_NO_THREADLOCAL)
9797
const ReflectionMode previous_mode_;
98-
#if defined(PROTOBUF_USE_DLLS)
98+
#if defined(PROTOBUF_USE_DLLS) && defined(_MSC_VER)
99+
// Thread local variables cannot be exposed through MSVC DLL interface but we
100+
// can wrap them in static functions.
99101
static ReflectionMode& reflection_mode();
100102
#else
101103
PROTOBUF_CONSTINIT static PROTOBUF_THREAD_LOCAL ReflectionMode
102104
reflection_mode_;
103-
#endif // PROTOBUF_USE_DLLS
105+
#endif // PROTOBUF_USE_DLLS && _MSC_VER
104106
#endif // !PROTOBUF_NO_THREADLOCAL
105107
};
106108

107109
#if !defined(PROTOBUF_NO_THREADLOCAL)
108110

109-
#if defined(PROTOBUF_USE_DLLS)
111+
#if defined(PROTOBUF_USE_DLLS) && defined(_MSC_VER)
110112

111113
inline ScopedReflectionMode::ScopedReflectionMode(ReflectionMode mode)
112114
: previous_mode_(reflection_mode()) {
@@ -140,7 +142,7 @@ inline ReflectionMode ScopedReflectionMode::current_reflection_mode() {
140142
return reflection_mode_;
141143
}
142144

143-
#endif // PROTOBUF_USE_DLLS
145+
#endif // PROTOBUF_USE_DLLS && _MSC_VER
144146

145147
#else
146148

src/google/protobuf/thread_safe_arena.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -261,9 +261,9 @@ class PROTOBUF_EXPORT ThreadSafeArena {
261261
// iOS does not support __thread keyword so we use a custom thread local
262262
// storage class we implemented.
263263
static ThreadCache& thread_cache();
264-
#elif defined(PROTOBUF_USE_DLLS)
265-
// Thread local variables cannot be exposed through DLL interface but we can
266-
// wrap them in static functions.
264+
#elif defined(PROTOBUF_USE_DLLS) && defined(_MSC_VER)
265+
// Thread local variables cannot be exposed through MSVC DLL interface but we
266+
// can wrap them in static functions.
267267
static ThreadCache& thread_cache();
268268
#else
269269
PROTOBUF_CONSTINIT static PROTOBUF_THREAD_LOCAL ThreadCache thread_cache_;

0 commit comments

Comments
 (0)