ODR violation when building protobuf with bazel as a dependency of grpc

[jtattermusch]

When running gRPC bazel build with an upgraded protobuf dependency (to 22.x), ASAN reports and ODR violation.
Indeed, it seems that wrappers.pb.cc (one of the WKT) gets included twice - once as com_google_protobuf/src/google/protobuf/_virtual_imports/wrappers_proto/google/protobuf/wrappers.pb.cc and another time as com_google_protobuf/src/google/protobuf/wkt/google/protobuf/wrappers.pb.cc

ODR in ASAN (Looks like we are including wrappers.pb.cc in the build twice?)
Executing tests from //test/cpp/end2end:channelz_service_test@poller=poll
-----------------------------------------------------------------------------
=================================================================
==15==ERROR: AddressSanitizer: odr-violation (0x7fdffd443da0):
  [1] size=160 'vtable for google::protobuf::DoubleValue' bazel-out/k8-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_imports/wrappers_proto/google/protobuf/wrappers.pb.cc
  [2] size=160 'vtable for google::protobuf::DoubleValue' bazel-out/k8-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/wkt/google/protobuf/wrappers.pb.cc

error details:
https://source.cloud.google.com/results/invocations/87f26bb4-8652-484c-96a5-125bd2e15da7/targets/%2F%2Ftest%2Fcpp%2Futil:error_details_test@poller%3Depoll1/log

Looks like this could be a problem with protobuf's bazel build (TBH I don't understand where the file from under the _virtual_imports tree is coming from).

[jtattermusch]

@deannagarcia @mkruskal-google this is one of the issues that block grpc's upgrade to 22.x

[deannagarcia]

I have never seen this issue and I would guess it has more to do with how grpc uses protobuf (this file/line looks particularly suspect: https://github.com/grpc/grpc/blob/451b2303588965f7110526e4ae57f88ca4abfce7/bazel/protobuf.bzl#L270).

Can you link me to where this is being run/can you create a simpler reproduction case?

[jtattermusch]

To reproduce:

• clone branch https://github.com/jtattermusch/grpc/tree/repro_protobuf_issue_12314
• you might need to start port server (run tools/run_tests/start_port_server.py from repo root)
• bazel test --config asan //test/cpp/end2end:channelz_service_test@poller=poll --test_output streamed

That gave me the same error as on the CI

==12==ERROR: AddressSanitizer: odr-violation (0x7fc2c7393ec0):
  [1] size=160 'vtable for google::protobuf::DoubleValue' bazel-out/k8-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/_virtual_imports/wrappers_proto/google/protobuf/wrappers.pb.cc
  [2] size=160 'vtable for google::protobuf::DoubleValue' bazel-out/k8-fastbuild/bin/external/com_google_protobuf/src/google/protobuf/wkt/google/protobuf/wrappers.pb.cc
These globals were registered at these points:
  [1]:
    #0 0x5596e37e998a in __asan_register_globals (/usr/local/google/home/jtattermusch/.cache/bazel/_bazel_jtattermusch/473402ebb46aca5e4ea9eb310c6f9e93/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/bin/test/cpp/end2end/channelz_service_test@poller=poll+0x7b98a) (BuildId: 693a58804a5f88d72f5e6a80b096be9cffbaa9a6)
    #1 0x7fc2c738b72e in asan.module_ctor wrappers.pb.cc
    #2 0x7fc2c77bdabd in call_init elf/./elf/dl-init.c:70:3
    #3 0x7fc2c77bdabd in call_init elf/./elf/dl-init.c:26:1

  [2]:
    #0 0x5596e37e998a in __asan_register_globals (/usr/local/google/home/jtattermusch/.cache/bazel/_bazel_jtattermusch/473402ebb46aca5e4ea9eb310c6f9e93/execroot/com_github_grpc_grpc/bazel-out/k8-fastbuild/bin/test/cpp/end2end/channelz_service_test@poller=poll+0x7b98a) (BuildId: 693a58804a5f88d72f5e6a80b096be9cffbaa9a6)
    #1 0x7fc2bf7c84ae in asan.module_ctor wrappers.pb.cc
    #2 0x7fc2c77bdabd in call_init elf/./elf/dl-init.c:70:3
    #3 0x7fc2c77bdabd in call_init elf/./elf/dl-init.c:26:1

[deannagarcia]

I think this is still too grpc-specific for us to help debug, especially with the custom server and bazel rules. Do you ever see the odr violation anywhere else? Can you point us to a proto rule that is making either of these files?

[perezd]

@jtattermusch can you help me understand what these virtual imports are needed for? I don't really understand why this is an issue on our side? Can you help me understand the need for all this? I am seeing this protobuf.bzl file is the grpc repo not ours.

[jtattermusch]

Ok, I looked at the situation more in depths are here are my findings:

• One of the targets that ultimately ends up depending on two different copies of wrappers.pb.cc is //src/proto/grpc/channelz:channelz_proto (see https://github.com/grpc/grpc/blob/47ea5062cb0f5167ae7f36438750888aea63a9fe/src/proto/grpc/channelz/BUILD#L25)
• By analyzing the dependencies, I found out that it depends both on @com_google_protobuf//src/google/protobuf:wkt_cc_proto and @com_google_protobuf//:wrappers_proto

AFAICT, the @com_google_protobuf//:wrappers_proto is necessary since without it, compilation fails with src/proto/grpc/channelz/channelz.proto:29:1: Import "google/protobuf/wrappers.proto" was not found or had errors.

Is seems that the @com_google_protobuf//src/google/protobuf:wkt_cc_proto dependency comes the dependency chain @com_google_protobuf//:protobuf -> @com_google_protobuf//src/google/protobuf:protobuf -> @com_google_protobuf//src/google/protobuf:wkt_cc_proto and AFAICT there's nothing wrong with depending on @com_google_protobuf//:protobuf (I'd expect that exactly what one would depend on?)

The wkt_cc_proto target was added here: https://github.com/protocolbuffers/protobuf/pull/10576/files
(and seems new in 22.x+)

Questions:
Is adding a dependency on @com_google_protobuf//:wrappers_proto the correct approach to allow a *_proto_library target to reference the wrapper WKT? (It worked for us until now and is done automatically by grpc_proto_library if the well_known_protos = True.

Should grpc_proto_library not depend on @com_google_protobuf//:protobuf? (and what should it depend on?) I think we're in fact not adding that dependency ourselves, but we're getting it through the native.cc_proto_library:
https://github.com/grpc/grpc/blob/451b2303588965f7110526e4ae57f88ca4abfce7/bazel/cc_grpc_library.bzl#L87

[deannagarcia]

I see the issue, and the wkt_cc_proto target you link was our attempt to fix that in our repository. We added a //src/google/protobuf:protobuf_nowkt to use instead of //:protobuf. I'm confused why you are just seeing this issue now, given that the .pb.cc files for wkt were in that target before.

I think the easiest workaround is for you to use proto_lang_toolchain like we were using before. You can blacklist the wkt protos to stop the extra definition from being pulled in.

We are taking over these rules from bazel long term, so this is an issue we can hopefully look to resolve in the future.

[deannagarcia]

Update: this was fixed in #12419.