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

Problems with generated protos and DLLs on Windows #5849

Closed
coryan opened this issue Feb 15, 2021 · 5 comments
Closed

Problems with generated protos and DLLs on Windows #5849

coryan opened this issue Feb 15, 2021 · 5 comments
Labels
type: cleanup An internal cleanup or hygiene concern.

Comments

@coryan
Copy link
Member

coryan commented Feb 15, 2021

Status

Blocked by gRPC support. I (@coryan) prototyped this solution for the files generated by protoc. But there is no answer for gRPC.

Background

DLLs on Windows use a Windows specific extension to declare which symbols are exported from the library and which remain private to the library. The default is "private", and there is no way to change this default (well, more on this later). In contrast, Unix (and Linux) shared objects (aka dynamic libraries, aka shared libraries) export all symbols, though it is possible to hide specific symbols.

More often than not, these extensions are used through a pre-processor "export macro", which is defined to be __declspec(dllimport) when the library is used, and __declspec(dllexport) when the library is compiled.

CMake supports CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS which exports (almost) all symbols. As the documentation states

For global data symbols, __declspec(dllimport) must still be used when compiling against the code in the .dll.

Protobuf generates a number of these global data symbols that are used by proto libraries that depend on each other. For example, google/api/label.proto generates:

extern const ::PROTOBUF_NAMESPACE_ID::internal::DescriptorTable descriptor_table_google_2fapi_2flabel_2eproto;

and this is used in the code generated for google/api/metric.proto:

static const ::PROTOBUF_NAMESPACE_ID::internal::DescriptorTable*const descriptor_table_google_2fapi_2fmetric_2eproto_deps[3] = {
  &::descriptor_table_google_2fapi_2flabel_2eproto,
  &::descriptor_table_google_2fapi_2flaunch_5fstage_2eproto,
  &::descriptor_table_google_2fprotobuf_2fduration_2eproto,
};

If these two protos are in different DLLs, we have a problem, as the DLLs would then reference data symbols from each other.

Problem

We cannot generate DLLs with protos that reference each other.

Proposed Solution: Group the protos into a small number of DLLs

We would need to use the dllexport_decl option in the protoc compiler to generate protos with the right export macros:

https://github.com/protocolbuffers/protobuf/blob/a1f96fffc7d062037c52bcb83e9e841d325eeaf8/src/google/protobuf/compiler/cpp/cpp_options.h#L54

Then we would use -include (for GCC and Clang) and /FI (for MSVC) to force include headers that define these export macros. Note that these would be included by our libraries and by any users of our libraries.

https://groups.google.com/g/protobuf/c/PDR1bqRazts

The headers would need to be maintained by hand, we can have CMake generate them:

https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html

Alternatives considered

Use a single DLL with all the protos

Would not help if the protos are not usable by application developers outside the DLL. Which they would be, as we exposed protos for the admin APIs (and will expose them even more for generated libraries).

Keep the current target -> library mapping.

We have a lot of single-proto libraries, it seems wasteful to force include a header for each one, and pollute the command-line with dozens of flags.

Appendix: sample error messages from linking DLLs with protos

[build] metric.pb.cc.obj : error LNK2001: unresolved external symbol "struct google::protobuf::internal::DescriptorTable const descriptor_table_google_2fapi_2flabel_2eproto" (?descriptor_table_google_2fapi_2flabel_2eproto@@3UDescriptorTable@internal@protobuf@google@@B)
[build] metric.pb.cc.obj : error LNK2001: unresolved external symbol "struct google::protobuf::internal::DescriptorTable const descriptor_table_google_2fapi_2flaunch_5fstage_2eproto" (?descriptor_table_google_2fapi_2flaunch_5fstage_2eproto@@3UDescriptorTable@internal@protobuf@google@@B)
[build] metric.pb.cc.obj : error LNK2001: unresolved external symbol "struct google::protobuf::internal::SCCInfo<0> scc_info_LabelDescriptor_google_2fapi_2flabel_2eproto" (?scc_info_LabelDescriptor_google_2fapi_2flabel_2eproto@@3U?$SCCInfo@$0A@@internal@protobuf@google@@A)
[build] external\googleapis\google_cloud_cpp_api_metric_protos.dll : fatal error LNK1120: 3 unresolved externals
@coryan coryan added the type: cleanup An internal cleanup or hygiene concern. label Feb 15, 2021
@coryan
Copy link
Member Author

coryan commented Feb 16, 2021

Well, I just realized this does not matter, gRPC does not seem to support DLLs, so we are blocked:

grpc/grpc#937
grpc/grpc#15653
grpc/grpc#15333
grpc/grpc#16848

@Anton1123
Copy link

I'm seeing this issue when trying to use protobuf libraries. Importing a proto definition from a shared library into another proto definition contained in shared library fails.
It is mentioned that "We have change the code to support protobufs...". Can you explain / show what code change made fixed this for protobufs?

@coryan
Copy link
Member Author

coryan commented Oct 1, 2021

It is mentioned that "We have change the code to support protobufs...". Can you explain / show what code change made fixed this for protobufs?

I wish I could... I think I tested the ideas in https://groups.google.com/g/protobuf/c/PDR1bqRazts in a branch, and then discarded the branch during a spring cleaning, sorry.

I am going to edit the comment because it is widly inaccurate now.

@devjgm
Copy link
Contributor

devjgm commented May 5, 2022

gRPC does not support this and has no plans to support this at the moment. Closing.

@h-vetinari
Copy link

For reference, there's now a grpc issue for support of shared builds on windows, as well as a PR for rudimentary support (based on what we're actually shipping in conda-forge as of 1.55)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

No branches or pull requests

4 participants