Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protobuf's constant initialization is not actually constant on Windows #10159

Open
davidben opened this issue Jun 21, 2022 · 7 comments
Open
Assignees

Comments

@davidben
Copy link
Contributor

davidben commented Jun 21, 2022

What version of protobuf and what language are you using?
Version: main
Language: C++

What operating system (Linux, Windows, ...) and version?
Windows 10 20H2

What runtime / compiler are you using (e.g., python version or gcc version)
clang-cl from Visual Studio 2019

What did you do?
Steps to reproduce the behavior:

  1. Install Visual Studio 2019
  2. In the installer, include "C++ Clang tools for Windows"
  3. git clone https://github.com/protocolbuffers/protobuf.git
  4. cd protobuf
  5. git submodule update --init --recursive
  6. cmake -B build-shared -D BUILD_SHARED_LIBS=1 -T ClangCL
  7. cmake --build build-shared --parallel 16

What did you expect to see
The build should succeed

What did you see instead?
The build fails with errors like:

...\protobuf\src\google\protobuf\compiler\plugin.pb.cc(75,31): message : required by 'require_constant_initializatio
n' attribute here [...\protobuf\build-shared\libprotoc.vcxproj]
...\protobuf\src\google/protobuf/port_def.inc(656,30): message : expanded from macro 'PROTOBUF_CONSTINIT' [...\pr
otobuf\build-shared\libprotoc.vcxproj]
...\protobuf\src\google\protobuf\compiler\plugin.pb.cc(91,125): error : variable does not have a constant initialize
r [...\protobuf\build-shared\libprotoc.vcxproj]

Anything else we should know about your project / environment
This was an issue updating protobuf in Chromium. We've applied a workaround by patching out the PROTOBUF_CONSTINIT in some configurations, but the root problem is that protobuf's new constant initialization was not actually cross-platform, and relies on assumption that aren't actually universally true.

While, for the repro steps, I used clang-cl, this is only because protobuf has a pre-C++20 version of PROTOBUF_CONSTINIT for Clang, [[clang::require_constant_initialization]]. When protobuf moves to C++20, I expect it will have the same problem on MSVC.

This happens because the protobuf constant initialization assumes it can reference &fixed_address_empty_string. fixed_address_empty_string lives in the protobuf dll, so when referenced from another dll or exe, is __declspec(dllimport). But pointers to dllimport variables require a static initializer on Windows. See https://godbolt.org/z/5rn4xWrhE. As I understand it, Windows does not have a suitable relocation to express this.

As a result, the new constant initialization actually requires a per-message static initializer, which trips PROTOBUF_CONSTINIT and fails to build.

@davidben
Copy link
Contributor Author

When protobuf moves to C++20, I expect it will have the same problem on MSVC.

Yup! See https://godbolt.org/z/qTxb8vocb

@goldenbull
Copy link

I have the same problem. I have to use c++20 because std::format is only available in c++20. Currently I have to hack the header file google\protobuf\port_def.inc, change #define PROTOBUF_CONSTINIT constinit to #define PROTOBUF_CONSTINIT

Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

@github-actions github-actions bot added the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Jan 14, 2024
Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 28, 2024
@dmiller423
Copy link

Ran into this upon update to [4?.] 25.0, building w. clang-cl.
Also many invalid offsetof() warnings..

@justusranvier
Copy link

Protobuf compilation on Windows is completely broken and they closed the bug as "not planned".

Wonderful.

Not only that, but they intentionally set up the headers with this so you can't even manually apply a define to fix their broken logic:

#ifdef PROTOBUF_CONSTINIT
#error PROTOBUF_CONSTINIT was previously defined
#endif

@googleberg
Copy link
Member

This shouldn't have been autoclosed.

@googleberg googleberg reopened this Sep 17, 2024
@googleberg googleberg removed the inactive Denotes the issue/PR has not seen activity in the last 90 days. label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants