Skip to content

Commit 38927bf

Browse files
Refine the conditions for the MSVC constinit workaround
As I understand it, the underlying limitation comes from Windows lacking a runtime relocation that can express pointers to symbols in a different dll. In particular, it seems the generated TcParseTable objects reference libprotobuf symbols under _pbi::TcParser. When libprotobuf is built as a dll, this breaks. See #10159. There is already a workaround that suppresses all of PROTOBUF_CONSTINIT on MSVC. This is, as far as I can tell, both too broad and too narrow. It is too narrow by excluding clang, when the limitation is Windows-wide, not specific to MSVC. It is too broad by including non-PROTOBUF_USE_DLLS builds, when statically-linked libprotobuf is, as I understand it, fine. This CL changes the condition to check for PROTOBUF_USE_DLLS && _WIN32, to reflect this being a Windows limitation, not a compiler limitation. As a result: 1. clang-cl + PROTOBUF_USE_DLLS now suppresses constinit. This is the configuration that is broken and should now build. This should be low risk. Turning off constinit should only make the compiler accept more things. 2. MSVC + non-PROTOBUF_USE_DLLS + C++20 restores constinit. If the above is correct, non-PROTOBUF_USE_DLLS never needed the workaround. This avoids regressing constinit on staticly-linked Windows. There is some risk here if I'm wrong and we needed the suppression more broadly. _WIN32 would also cover MinGW and Cygwin, which were written differently. The MinGW suppression was added in #13240 and cited shared libraries, so it seems this is really the same error and now covered by the fixed condition. The Cygwin suppression was aded in #9562 back in 2022, and it is unclear whether that was being built as a shared library. (The error about std::mutex is also odd but it seems to be an issue with constexpr and not constinit? That may be interesting for 7f431bb, but doesn't seem to be related to constinit specifically.) Regardless, there is now a GCC-wide suppression of constinit, so it is *at least* redundant with it, so I've removed it for now. Though if the GCC suppression is removed, we may later learn that Cygwin had a non-dll issue. Finally, after 7f431bb, some of the PROTOBUF_CONSTINIT logic was a little odd. The version checks, for instance, are now all no-ops because the fallback case is the same. I've cleaned up that code a bit, but those aren't expected to change behavior. (It's still arguably a workaround because constinit itself is supported by Windows. protobuf just doesn't generate constinit code in this configuration. A real fix might be to rework TcParseTable to avoid function pointers into libprotobuf? Possibly there are other issues to fix too? Not sure. Or perhaps to just declare this build configuration is only half-supported and has to suffer a static initializer.) PiperOrigin-RevId: 855254095
1 parent 1bc2c63 commit 38927bf

File tree

1 file changed

+13
-18
lines changed

1 file changed

+13
-18
lines changed

src/google/protobuf/port_def.inc

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -416,35 +416,30 @@ static_assert(PROTOBUF_ABSL_MIN(20230125, 3),
416416
#error PROTOBUF_CONSTINIT was previously defined
417417
#endif
418418

419-
// Lexan sets both MSV_VER and clang, so handle it with the clang path.
420-
#if defined(_MSC_VER) && !defined(__clang__)
421-
// MSVC 17 currently seems to raise an error about constant-initialized pointers.
422-
# if PROTOBUF_MSC_VER_MIN(1930)
419+
#if defined(PROTOBUF_USE_DLLS) && (defined(_WIN32) || defined(__CYGWIN__))
420+
// On Windows, `constinit` structures cannot include pointers to dllimport
421+
// symbols, which are defined in another dll. If libprotobuf is built as a dll,
422+
// generated parse tables breaks this. See
423+
// https://github.com/protocolbuffers/protobuf/issues/10159 and
424+
// https://github.com/protocolbuffers/protobuf/pull/13240. Work around this by
425+
// suppressing `constinit`.
423426
#define PROTOBUF_CONSTINIT
424-
# endif
425427
#elif defined(__GNUC__) && !defined(__clang__)
426428
// GCC doesn't support constinit aggregate initialization of absl::Cord.
427-
# if PROTOBUF_GNUC_MIN(12, 2)
428429
#define PROTOBUF_CONSTINIT
429-
# endif
430-
#else
431-
# if defined(__cpp_constinit) && !defined(__CYGWIN__)
430+
#elif defined(__cpp_constinit)
432431
#define PROTOBUF_CONSTINIT constinit
433-
# define PROTOBUF_CONSTINIT_DEFAULT_INSTANCES
432+
#define PROTOBUF_CONSTINIT_DEFAULT_INSTANCES
434433
// Some older Clang versions incorrectly raise an error about
435434
// constant-initializing weak default instance pointers. Versions 12.0 and
436435
// higher seem to work, except that XCode 12.5.1 shows the error even though it
437436
// uses Clang 12.0.5.
438-
#elif !defined(__CYGWIN__) && !defined(__MINGW32__) && \
439-
ABSL_HAVE_CPP_ATTRIBUTE(clang::require_constant_initialization) && \
440-
((defined(__APPLE__) && PROTOBUF_CLANG_MIN(13, 0)) || \
437+
#elif ABSL_HAVE_CPP_ATTRIBUTE(clang::require_constant_initialization) && \
438+
((defined(__APPLE__) && PROTOBUF_CLANG_MIN(13, 0)) || \
441439
(!defined(__APPLE__) && PROTOBUF_CLANG_MIN(12, 0)))
442440
#define PROTOBUF_CONSTINIT [[clang::require_constant_initialization]]
443-
# define PROTOBUF_CONSTINIT_DEFAULT_INSTANCES
444-
# endif
445-
#endif
446-
447-
#ifndef PROTOBUF_CONSTINIT
441+
#define PROTOBUF_CONSTINIT_DEFAULT_INSTANCES
442+
#else
448443
#define PROTOBUF_CONSTINIT
449444
#endif
450445

0 commit comments

Comments
 (0)