Use the same ABI for static and shared libraries on non-Windows platforms #12983
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fowles
approved these changes
Jun 5, 2023
fowles
approved these changes
Jun 6, 2023
fowles
added
🅰️ safe for tests
fowles
approved these changes
Jun 6, 2023
fowles
added
🅰️ safe for tests
fowles
pushed a commit
to fowles/protobuf
that referenced
this pull request
Jun 7, 2023
…orms (protocolbuffers#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 protocolbuffers@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 protocolbuffers#12700 In the commit message it is mentionned that most likely we shall rather have a stable ABI. Finally in protocolbuffers#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 protocolbuffers#12983 COPYBARA_INTEGRATE_REVIEW=protocolbuffers#12983 from Romain-Geissler-1A:stable-abi-use-dll-non-windows dc23ff5 PiperOrigin-RevId: 538230923
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-cpp
that referenced
this pull request
Jun 21, 2023
### Motivation The current CMakeLists.txt does not work with the latest Protobuf (23.3.0). It's because currently the Module mode is used to find Protobuf, while the `FindProtobuf.cmake` is not updated to find the Abseil dependency. See protocolbuffers/protobuf#12292 (comment) ### Modifications For macOS, use the Config mode to find Protobuf. It's because in other systems, the Module mode works well. Besides, enable `PROTOBUF_USE_DLLS` as a workaround for protocolbuffers/protobuf#12983 is not released.
4 tasks
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-cpp
that referenced
this pull request
Jun 21, 2023
### Motivation The current CMakeLists.txt does not work with the latest Protobuf (23.3.0). It's because currently the Module mode is used to find Protobuf, while the `FindProtobuf.cmake` is not updated to find the Abseil dependency. See protocolbuffers/protobuf#12292 (comment) ### Modifications For macOS, use the Config mode to find Protobuf. It's because in other systems, the Module mode works well. Besides, enable `PROTOBUF_USE_DLLS` as a workaround for protocolbuffers/protobuf#12983 is not released.
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-cpp
that referenced
this pull request
Jun 25, 2023
### Motivation The current CMakeLists.txt does not work with the latest Protobuf (23.3.0). It's because currently the Module mode is used to find Protobuf, while the `FindProtobuf.cmake` is not updated to find the Abseil dependency. See protocolbuffers/protobuf#12292 (comment) ### Modifications For macOS, use the Config mode to find Protobuf. It's because in other systems, the Module mode works well. Besides, enable `PROTOBUF_USE_DLLS` as a workaround for protocolbuffers/protobuf#12983 is not released. Pin the default C++ standard to 17 for macOS so that users don't need to set the C++ standard manually.
shibd
pushed a commit
to apache/pulsar-client-cpp
that referenced
this pull request
Jun 26, 2023
### Motivation The current CMakeLists.txt does not work with the latest Protobuf (23.3.0). It's because currently the Module mode is used to find Protobuf, while the `FindProtobuf.cmake` is not updated to find the Abseil dependency. See protocolbuffers/protobuf#12292 (comment) ### Modifications For macOS, use the Config mode to find Protobuf. It's because in other systems, the Module mode works well. Besides, enable `PROTOBUF_USE_DLLS` as a workaround for protocolbuffers/protobuf#12983 is not released. Pin the default C++ standard to 17 for macOS so that users don't need to set the C++ standard manually.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
It seems that until last year, the logic behind
PROTOBUF_USE_DLLSwas for Windows (MSCV) only. It was changed to all platforms here in 5a0887fLast month, the generated pkg config files were updated to reflect the protobuf build-time value of
PROTOBUF_USE_DLLSas 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:
which denotes a mix of some .o or libs built
PROTOBUF_USE_DLLSdefined and some others build withPROTOBUF_USE_DLLSundefined, 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_localvariables, but experience has shown that:thread_local, leading to theGOOGLE_PROTOBUF_NO_THREADLOCALmacro later renamedPROTOBUF_NO_THREADLOCALwhich 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 allPROTOBUF_NO_THREADLOCALrelated code paths (this pull request doesn't do this).thread_localshall 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 ?