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 5a0887fc65
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 dc23ff50f6
PiperOrigin-RevId: 538230923
This additional if is necessary as of abseil 20230125.3 when abseil is consumed via add_subdirectory,
the abseil_dll target is named abseil_dll, while if abseil is consumed via find_package, the target is called `absl::abseil_dll` .
Once https://github.com/abseil/abseil-cpp/pull/1466 is merged and released in the minimum version of abseil required by protobuf, it is possible to always link `absl::abseil_dll` and `absl::abseil_test_dll` and remove the if.
You may wonder how linking worked at all before when `protobuf_ABSL_PROVIDER STREQUAL "package"`, as `abseil_dll` was not an imported target defined by `find_package(absl)`. The reason behind this is that if a name that is not an imported target is passed to `target_link_libraries`, it is just regarded as a C++ library name. So, in the end the `abseil_dll` library was correctly linked, simply all the transitive usage requirements defined by the `absl::abseil_dll` target were not propagated, that could lead to compilation errors if abseil was compiled with the `ABSL_PROPAGATE_CXX_STD` CMake option enabled.
Closes#12978
COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/12978 from traversaro:patch-1 39dd074281
PiperOrigin-RevId: 537990391
This is a macro on some (older) versions of GCC, and macOS, and Windows. Sigh. I moved the `#undef` block to a common section. I also took the opportunity to add a regression test for all these macros that need to be `#undef`'d.
Part of the work for googleapis/google-cloud-cpp#8125
Closes#12903
PiperOrigin-RevId: 535649278
Fix for unknown type `int32_t` in `src/google/protobuf/compiler/objectivec/text_format_decode_data.h`
Issue encountered when building with GCC 13 (MinGW-w64 on Windows 64-bit).
Error:
```
In file included from R:/winlibs-gcc13-64/protobuf-22.3/src/google/protobuf/compiler/objectivec/text_format_decode_data.cc:31:
R:/winlibs-gcc13-64/protobuf-22.3/src/google/protobuf/compiler/objectivec/text_format_decode_data.h:59:18: error: 'int32_t' has not been declared
59 | void AddString(int32_t key, const std::string& input_for_decode,
| ^~~~~~~
R:/winlibs-gcc13-64/protobuf-22.3/src/google/protobuf/compiler/objectivec/text_format_decode_data.h:68:21: error: 'int32_t' was not declared in this scope
68 | typedef std::pair<int32_t, std::string> DataEntry;
| ^~~~~~~
R:/winlibs-gcc13-64/protobuf-22.3/src/google/protobuf/compiler/objectivec/text_format_decode_data.h:40:1: note: 'int32_t' is defined in header '<cstdint>'; did you forget to '#include <cstdint>'?
39 | #include "google/protobuf/port_def.inc"
+++ |+#include <cstdint>
40 |
```
Closes#12554
COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/12554 from brechtsanders:patch-1 0ab04b43b6
PiperOrigin-RevId: 527667592
This turns the constexpr constructors into templates to silence errors when constexpr isn't valid. We are also switching to 12.2 for GCC/cmake tests to prevent regressions (9.5 and 13.1 are already tested by GCC/bazel tests).
Fixes#12807
PiperOrigin-RevId: 532258101
This PR replaces the descriptor name validation regex with a validation method. This change allows the `System.Text.RegularExpressions` engine to be trimmed away in published apps that do standard protobuf serialization.
There are some other usages of `Regex` in Google.Protobuf, but they in `JsonParser`. They are only included in a published app if `JsonParser` is used.
Another benefit is a slightly faster app startup time. The removed regex was compiled, which has a high-ish fixed cost.
cc @jskeet@jtattermuschCloses#12174
COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/12174 from JamesNK:jamesnk/remove-regex 9d065a3a71
PiperOrigin-RevId: 532210203
In both cases a `size_t` was being converted to a `uint32_t`. These headers are used from application code, or at least from generated code, and the application may be compiling with more warnings enabled than normal.
Closes#12762
COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/12762 from coryan:fix-avoid-warnings-with-MSVC 5ba6b5db1e
PiperOrigin-RevId: 531506224
This crash was due to the fact that we were passing `nullptr` as a `const
char*` parameter and relying on that implicitly converting into an empty
`absl::string_view`. `absl::string_view` supports that functionality, but
starting with C++17 its behavior changes since it's just a type alias for
`std::string_view`. `std::string_view` does not have any special conversion for
nullptr and so we were just getting crashes.
PiperOrigin-RevId: 530431663
Warnings in header files can be a problem for consumers that enable `/WX` (or `-Werror`). In this case, using `... & -align` produces a warning (C4146) with MSVC. The fix is to use equivalent expression `... & ~(align - 1)`, which was already used in the same file.
Fixes#12675Closes#12697
COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/12697 from coryan:fix-msvc-warnings-in-arena-align 835f3b489a
PiperOrigin-RevId: 530137165
On Wndows, `size_t` is 64-bits, and `int` is 32-bits. That makes conversions from `size_t` to `int` potentially lossy, and they generate warnings. In this case an `int` variable was assigned to `size_t` and then passed to functions consuming `int`. Seems simpler to use `auto` and avoid these problems altogether.
Closes#12701
COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/12701 from coryan:fix-warnings-repeated-field-warnings-in-msvc b1ec34de77
PiperOrigin-RevId: 530134611
When the protobuf libraries have been compiled as shared libraries the users of the library need to add `-DPROTOBUF_USE_DLLS` to their build line. Otherwise some symbols are missing.
Fixes#12699
FWIW, I am not sure this is an ideal fix. It may be better to fix the headers such that no macros change the ABI.
Closes#12700
COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/12700 from coryan:fix-define-protobuf-use-dlls-in-pkg-config-file 13c792eebd
PiperOrigin-RevId: 530116678
Note: gcc only supports docker images down to 9.5, and the 7.3 image is very old and problematic. A follow-up change might enable testing for GCC 7.3, which is our minimal supported version
PiperOrigin-RevId: 529885733
These more closely follow the standard practices of our users, where dependencies are pre-installed instead of using our provided sub-modules. This will prevent issues such as #12201 from reoccuring.
Additionally, this cl bumps our Abseil dependency to the latest release, and fixes a GTest issue that went previously unnoticed.
PiperOrigin-RevId: 529490402
Matt Fowles Kulukundis
Thomas Van Lenten
Romain Geissler @ Amadeus
Protobuf Team Bot
Silvio Traversaro
Carlos O'Ryan
Mike Kruskal
Protobuf Team Bot
Mike Kruskal
Brecht Sanders
Joshua Haberman
James Newton-King
Adam Cozzette
deannagarcia