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

global std::once_flag variables don't work with Visual Studio #4773

Closed
xfxyjwf opened this issue Jun 16, 2018 · 11 comments · Fixed by #4882
Closed

global std::once_flag variables don't work with Visual Studio #4773

xfxyjwf opened this issue Jun 16, 2018 · 11 comments · Fixed by #4882
Assignees

Comments

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Jun 16, 2018

Symptom: statically built protoc binary check-fails.
#4409

[libprotobuf FATAL D:\Google-Protobuf\src\src\google\protobuf\generated_message_reflection.cc:2372] CHECK failed: file != NULL: 

Cause:
Visual studio compiler bug:
https://developercommunity.visualstudio.com/content/problem/262083/compiler-emits-dynamic-initializer-for-variable-wi.html

Similar bug: https://developercommunity.visualstudio.com/content/problem/1044/compiler-emits-dynamic-initializer-for-constexpr-v.html

The compiler generates dynamic-initialization code for global std::once_flag variables even though the constructor of once_flag is constexpr. As a result, the following code is broken:
https://github.com/google/protobuf/blob/master/src/google/protobuf/descriptor.cc#L1343-L1362

GOOGLE_PROTOBUF_DECLARE_ONCE(generated_pool_init_);
static void InitGeneratedPool() {
  generated_database_ = new EncodedDescriptorDatabase;
  generated_pool_ = new DescriptorPool(generated_database_);
  generated_pool_->InternalSetLazilyBuildDependencies();
}
void InitGeneratedPoolOnce() {
  ::google::protobuf::GoogleOnceInit(&generated_pool_init_, &InitGeneratedPool);
}
  1. InitGeneratedPoolOnce() is called before generated_pool_init_ is dynamically initialized. It will call InitGeneratedPool() and create EncodeDescriptorDatabase/DescriptorPool for the first time.
  2. dynamical initialization of generated_pool_init_ happens and it simply zeros the variable's content.
  3. InitGeneratedPoolOnce() is called again and it calls InitGeneratedPool() again which creates new EncodedDescriptorDatabase/DescriptorPool for the second time.

Anything registered with the descriptordatabase/pool created in (1) will get lost after (3), and as a result descriptors that should already be in the global pool appear missing and this triggers the following check failure:
https://github.com/google/protobuf/blob/master/src/google/protobuf/generated_message_reflection.cc#L2370-L2372

  const ::google::protobuf::FileDescriptor* file =
      ::google::protobuf::DescriptorPool::generated_pool()->FindFileByName(filename);
  GOOGLE_CHECK(file != NULL);

We can workaround the problem by moving the global once_flag into a static variable inside the init function:

void InitGeneratedPoolOnce() {
  static ::google::protobuf::ProtobufOnceType generated_pool_init_;
  ::google::protobuf::GoogleOnceInit(&generated_pool_init_, &InitGeneratedPool);
}

This way, the variable is guaranteed to be initialized before GoogleOnceInit() and it appears to work with Visual Studio 2017.

@xfxyjwf xfxyjwf self-assigned this Jun 16, 2018
@xfxyjwf
Copy link
Contributor Author

xfxyjwf commented Jun 16, 2018

See b/110301009

@nikhilpothuru
Copy link
Contributor

Hi, This is my first time trying to contribute to an open source project. I am looking to help out with any C++ or Python issues. It would be great to get some help starting off. I've gone through the Developer Guide and Protocol Buffer Basics: C++ to prepare. Thanks!

@xfxyjwf
Copy link
Contributor Author

xfxyjwf commented Jun 22, 2018

@nikhilpothuru Thanks for offering help! This issue here is probably not a great place to start though. I think it's probably better to start with some simple cleanups to get yourself more familiar with the protobuf build/development environment.

@nikhilpothuru
Copy link
Contributor

Hi! Thank you for the quick reply. Is there a specific cleanup issue you can point me to? I'm looking through the issues, but it is a bit overwhelming since I don't know where to start. Thanks!

@remoe
Copy link

remoe commented Jun 22, 2018

I've tested with VS 2017 (15.7.4) DEBUG and protobuf 3.6.0:

cmake parameters: -GVisual Studio 15 Win64 -DCMAKE_SYSTEM_VERSION=8.1 -DBUILD_SHARED_LIBS=ON

The protoc executable crashes on tests:

   8>Done Building Project "D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\_build\test_plugin.vcxproj" (default targets).
    14>d:\apps\vs2017\Common7\IDE\VC\VCTargets\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 3. [D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\
       _build\lite-arena-test.vcxproj]
    14>Done Building Project "D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\_build\lite-arena-test.vcxproj" (default targets) -- FAILED.
     9>d:\apps\vs2017\Common7\IDE\VC\VCTargets\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 3. [D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\
       _build\tests.vcxproj]
     9>Done Building Project "D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\_build\tests.vcxproj" (default targets) -- FAILED.
     3>Project "D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\_build\ALL_BUILD.vcxproj" (3) is building "D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\_build\lite
       -test.vcxproj" (6) on node 1 (default targets).
     6>InitializeBuildStatus:
         Touching "lite-test.dir\Debug\lite-test.tlog\unsuccessfulbuild".
       CustomBuild:
         Generating D:/sdk/_build/win_x86_64/protobuf-3.6.0-debug/protobuf.git/src/google/protobuf/map_lite_unittest.pb.cc
         [libprotobuf FATAL d:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\src\google\protobuf\message.cc:346] File appears to be in generated pool but wasn't registered: googl
         e/protobuf/descriptor.proto
     6>d:\apps\vs2017\Common7\IDE\VC\VCTargets\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 3. [D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\
       _build\lite-test.vcxproj]

   8>Done Building Project "D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\_build\test_plugin.vcxproj" (default targets).
    14>d:\apps\vs2017\Common7\IDE\VC\VCTargets\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 3. [D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\
       _build\lite-arena-test.vcxproj]
    14>Done Building Project "D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\_build\lite-arena-test.vcxproj" (default targets) -- FAILED.
     9>d:\apps\vs2017\Common7\IDE\VC\VCTargets\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 3. [D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\
       _build\tests.vcxproj]
     9>Done Building Project "D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\_build\tests.vcxproj" (default targets) -- FAILED.
     3>Project "D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\_build\ALL_BUILD.vcxproj" (3) is building "D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\_build\lite
       -test.vcxproj" (6) on node 1 (default targets).
     6>InitializeBuildStatus:
         Touching "lite-test.dir\Debug\lite-test.tlog\unsuccessfulbuild".
       CustomBuild:
         Generating D:/sdk/_build/win_x86_64/protobuf-3.6.0-debug/protobuf.git/src/google/protobuf/map_lite_unittest.pb.cc
         [libprotobuf FATAL d:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\src\google\protobuf\message.cc:346] File appears to be in generated pool but wasn't registered: googl
         e/protobuf/descriptor.proto
     6>d:\apps\vs2017\Common7\IDE\VC\VCTargets\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 3. [D:\sdk\_build\win_x86_64\protobuf-3.6.0-debug\protobuf.git\
       _build\lite-test.vcxproj]

@Renshk
Copy link

Renshk commented Jun 27, 2018

This way, the initialization of generated_pool_init_ is not thread-safe in the InitGeneratedPoolOnce function.

@Renshk
Copy link

Renshk commented Jun 27, 2018

GeneratedMessageFactory::singleton function have the same issue.

@xfxyjwf
Copy link
Contributor Author

xfxyjwf commented Jun 27, 2018

@Renshk Why is it not thread-safe? I'm pretty sure C++11 guarantees static local variable to be initialized thread-safely.

@wheezil
Copy link

wheezil commented Jun 27, 2018

Greetings... I ran into this error (well, actually #4679) trying to build protobuf libraries for Windows using VS2017. I found that copying the win32 distribution of protoc.exe over the built one allowed the build to complete successfully. However, nmake check tests fail with similar errors.

Given that I have VS2017 and would like to proceed with a project, what is the best advice for working around the problem, until either VS2017 or protobuf arrives at a solution?

EDIT: does going the vcpkg install route avoid this entirely? Is it an issue with generated code or only with the library build?

Thanks
john

@xfxyjwf
Copy link
Contributor Author

xfxyjwf commented Jun 27, 2018

@wheezil The bug only happens in debug build so you can workaround it by using the release build version of protobuf. The pre-built binaries published by us also do not have this problem because they are release builds.

@wheezil
Copy link

wheezil commented Jun 27, 2018

Thanks! I will just go with the pre-built binaries.

acozzette added a commit to acozzette/protobuf that referenced this issue Jul 6, 2018
It appears that Visual Studio does not work well with std::once_flag
because it has a bug causing it to initialize that during dynamic
initialization instead of constant initialization. This change works
around the problem by using function static initializers instead.

@gerben-s originally wrote this change for the Google-internal codebase
but I am just cherry-picking it here.

This fixes protocolbuffers#4773.
coryan added a commit to googleapis/google-cloud-cpp that referenced this issue Jul 31, 2018
Today was not a good day.  The Windows build broke over the weekend (maybe
late last week, but I did not notice until the weekend).

This morning I could not even troubleshoot the problem because `strawberryperl.com`
was down, and we use that (indirectly) to build openssl on Windows:

microsoft/vcpkg#3973

I tried to do other stuff, without success. Late in the morning the site was back up.
At first, I thought it was just a matter of adapting to the newer protobuf (3.6.0) that
we got as part of a regular vcpkg update:

microsoft/vcpkg@c95b6bf

Well, the changes I made in our `cmake/*` files (included in this PR) did fix the build,
and look like useful changes.  But then the unit tests would not pass because of:

protocolbuffers/protobuf#4773

Which is "Not Good"[tm].  I decided to revert the vcpkg version for now, that got the
tests to compile and pass on my Windows machine.  The programming gods wanted
to toy with this mortal today. Because the `.appveyor.yml` file was not properly clearing
the cache, and the builds on appveyor.com still used the protobuf 3.6 artifacts already
cached there.

BTW, this is probably why the builds did not break immediately: vcpkg reused the cached
versions even other stuff had changed.  Our builds continued to work after that commit
made it through.

I think all the necessary changes are in now.  I will commute home and fix anything else
that is broken when I get there.  May the programming gods be kind to you, oh faithful
reviewer.

* Use targets in add_custom_command.

CMake automatically expands targets to the correct generator
expression in add_custom_command, no need for generator
expressions.

* Set missing paths for PROTOBUF_IMPORT_DIRS.

Depending on your version of CMake you get PROTOBUF_IMPORT_DIRS,
or Protobuf_IMPORT_DIRS, or nothing:

https://cmake.org/cmake/help/v3.10/module/FindProtobuf.html
https://cmake.org/cmake/help/v3.5/module/FindProtobuf.html

Make an educated guess in the file dealing with such nonsense.

* Pin vcpkg to an older version.

The newer version includes protobuf-3.6.0 which is broken on
Windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants