Skip to content

Commit

Permalink
Work around a workaround of a workaround of a Bazel bug
Browse files Browse the repository at this point in the history
This fixes an issue where :ssl_test in the Bazel build ran no tests.
(The CMake build was fine.)

Bazel has no way to handle C and C++ sources need different flags
(bazelbuild/bazel#22041), so our Bazel rules
transparently split mixed C/C++ targets in two.

Bazel silently turns all static libraries into shared libraries for
tests. This means that, even if we set linkstatic = True on the
split-out library, the two halves don't end up in the same shared
object. This is because if A(test) -> B(lib) -> C(lib) and C is
linkstatic, C is statically linked into A, not B. This is probably to
avoid diamond dependency issues. From what I can tell, there is no clean
way to say "B can be made into a shared library but please statically
link C into it, because C will never be referenced except by way of B".

(This means our use of linkshared is wrong. The next CL will try to redo
that.)

This Bazel behavior does not recognize that static and shared libraries
have very, very different symbol handling. In particular, our assembly
files needed to be in the same shared object as OPENSSL_ia32cap_P, so
all this required another workaround in
https://boringssl-review.googlesource.com/c/boringssl/+/70690

This, in turn, triggered this latest issue:

GoogleTest relies on static initializers of individual object files to
register tests. This does not work with linker dead code elimination and
needs --whole-archive, or alwayslink in Bazel parlance. The most recent
Bazel workaround required the C++ sources be the ones that were
extracted, so they lost the --whole-archive behavior. As a result,
:ssl_test silently ran no tests!

Work around this latest Bazel-induced problem by setting alwayslink on
cc_test helpers.

All this would go away if Bazel just fixed
bazelbuild/bazel#22041

(We can probably revert
https://boringssl-review.googlesource.com/c/boringssl/+/70690 now, but
either way we should probably set alwayslink=True on the helpers so that
the build is not sensitive to which were extracted.)

Change-Id: I10745e1bcfe91ac929f154e66093b29723efc576
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71507
Reviewed-by: Bob Beck <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: Bob Beck <[email protected]>
  • Loading branch information
davidben authored and Boringssl LUCI CQ committed Sep 23, 2024
1 parent a4eb021 commit 505c2c6
Showing 1 changed file with 12 additions and 2 deletions.
14 changes: 12 additions & 2 deletions util/util.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ def handle_mixed_c_cxx(
includes,
linkopts,
srcs,
testonly):
testonly,
alwayslink):
"""
Works around https://github.com/bazelbuild/bazel/issues/22041. Determines
whether a target contains C, C++, or both. If the target is multi-language,
Expand Down Expand Up @@ -121,6 +122,7 @@ def handle_mixed_c_cxx(
linkopts = linkopts,
deps = deps,
testonly = testonly,
alwayslink = alwayslink,
)

# Build the remainder as a C-only target.
Expand Down Expand Up @@ -188,6 +190,7 @@ def bssl_cc_library(
linkstatic = None,
srcs = [],
testonly = False,
alwayslink = False,
visibility = []):
copts, deps, srcs = handle_mixed_c_cxx(
name = name,
Expand All @@ -198,6 +201,7 @@ def bssl_cc_library(
linkopts = linkopts,
srcs = srcs,
testonly = testonly,
alwayslink = alwayslink,
)

# BoringSSL's notion of internal headers are slightly different from
Expand All @@ -217,6 +221,7 @@ def bssl_cc_library(
linkopts = linkopts,
deps = deps,
testonly = testonly,
alwayslink = alwayslink,
**linkstatic_kwargs(linkstatic)
)

Expand Down Expand Up @@ -248,6 +253,9 @@ def bssl_cc_binary(
linkopts = linkopts,
srcs = srcs,
testonly = testonly,
# TODO(davidben): Should this be alwayslink = True? How does Bazel treat
# the real cc_binary.srcs?
alwayslink = False,
)

cc_binary(
Expand All @@ -268,7 +276,6 @@ def bssl_cc_test(
asm_srcs = [],
data = [],
size = "medium",
internal_hdrs = [],
copts = [],
includes = [],
linkopts = [],
Expand All @@ -284,6 +291,9 @@ def bssl_cc_test(
linkopts = linkopts,
srcs = srcs,
testonly = True,
# If any sources get extracted, they must always be linked, otherwise
# tests will be dropped.
alwayslink = True,
)

cc_test(
Expand Down

0 comments on commit 505c2c6

Please sign in to comment.