Refine the conditions for the MSVC constinit workaround #25086
Conversation
This file contains hidden or 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
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 such that: 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. (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: 850876823
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
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 such that:
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.
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.
(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.)