Skip to content

Commit

Permalink
Rework the Bazel workaround's relationship with linkstatic
Browse files Browse the repository at this point in the history
Due to a longstanding Bazel flaw
(bazelbuild/bazel#22041), we need to split all
our mixed C/C++ targets in two. Ideally this split would behave as if
the Bazel flaw were fixed, with the split-out library statically linked
with the other source files. Accordingly, the helper macro sets
linkstatic = True.

It turns out linkstatic = True does not work this way. Bazel interprets
linkstatic such that, if A(test, linkshared) -> B(library) -> C(library,
linkstatic), C will be statically linked into A, not B. This is probably
to avoid diamond dependency problems but means it is not possible for a
cc_library split to be transparent, only cc_binary and cc_test.

So what is happening is that every cc_test that links libcrypto is
getting mlkem.cc statically linked into it, separate from the rest of
libcrypto! That means we're getting the worst of both worlds: worse
cache hit rate for tests that link libcrypto AND our C/C++ bits are
still not contained in the same library.

linkstatic = True on the helper is still valuable in cc_test and
cc_binary, but otherwise inherit the outer value.

Change-Id: I1089c58c6ddaa90c89efd8cdcebd88169b0236c8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71508
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
  • Loading branch information
davidben authored and Boringssl LUCI CQ committed Sep 23, 2024
1 parent 505c2c6 commit 62d8540
Showing 1 changed file with 28 additions and 13 deletions.
41 changes: 28 additions & 13 deletions util/util.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,24 @@ boringssl_copts_c = boringssl_copts_common + select({
"//conditions:default": ["-std=c11"] + gcc_copts_c,
})

def linkstatic_kwargs(linkstatic):
# Although Bazel's documentation says linkstatic defaults to True or False
# for the various target types, this is not true. The defaults differ by
# platform non-Windows and True on Windows. There is now way to request the
# default except to omit the parameter, so we must use kwargs.
kwargs = {}
if linkstatic != None:
kwargs["linkstatic"] = linkstatic
return kwargs

def handle_mixed_c_cxx(
name,
copts,
deps,
internal_hdrs,
includes,
linkopts,
linkstatic,
srcs,
testonly,
alwayslink):
Expand All @@ -116,13 +127,11 @@ def handle_mixed_c_cxx(
srcs = srcs_cxx + internal_hdrs,
copts = copts + boringssl_copts_cxx,
includes = includes,
# This target only exists to be linked into the main library, so
# always link it statically.
linkstatic = True,
linkopts = linkopts,
deps = deps,
testonly = testonly,
alwayslink = alwayslink,
**linkstatic_kwargs(linkstatic),
)

# Build the remainder as a C-only target.
Expand Down Expand Up @@ -168,16 +177,6 @@ def handle_asm_srcs(asm_srcs):
"//conditions:default": asm_srcs,
})

def linkstatic_kwargs(linkstatic):
# Although Bazel's documentation says linkstatic defaults to True or False
# for the various target types, this is not true. The defaults differ by
# platform non-Windows and True on Windows. There is now way to request the
# default except to omit the parameter, so we must use kwargs.
kwargs = {}
if linkstatic != None:
kwargs["linkstatic"] = linkstatic
return kwargs

def bssl_cc_library(
name,
asm_srcs = [],
Expand All @@ -199,6 +198,14 @@ def bssl_cc_library(
internal_hdrs = hdrs + internal_hdrs,
includes = includes,
linkopts = linkopts,
# Ideally we would set linkstatic = True to statically link the helper
# library into main cc_library. But Bazel interprets linkstatic such
# that, if A(test, linkshared) -> B(library) -> C(library, linkstatic),
# C will be statically linked into A, not B. This is probably to avoid
# diamond dependency problems but means linkstatic does not help us make
# this function transparent. Instead, just pass along the linkstatic
# nature of the main library.
linkstatic = linkstatic,
srcs = srcs,
testonly = testonly,
alwayslink = alwayslink,
Expand Down Expand Up @@ -250,6 +257,10 @@ def bssl_cc_binary(
deps = deps,
internal_hdrs = [],
includes = includes,
# If it weren't for https://github.com/bazelbuild/bazel/issues/22041,
# the split library be part of `srcs` and linked statically. Set
# linkstatic to match.
linkstatic = True,
linkopts = linkopts,
srcs = srcs,
testonly = testonly,
Expand Down Expand Up @@ -288,6 +299,10 @@ def bssl_cc_test(
deps = deps,
internal_hdrs = [],
includes = includes,
# If it weren't for https://github.com/bazelbuild/bazel/issues/22041,
# the split library be part of `srcs` and linked statically. Set
# linkstatic to match.
linkstatic = True,
linkopts = linkopts,
srcs = srcs,
testonly = True,
Expand Down

0 comments on commit 62d8540

Please sign in to comment.