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

build: simplify cc_configure and static link for libc++ #7329

Merged
merged 10 commits into from
Jul 11, 2019
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ build:clang-tsan --define ENVOY_CONFIG_TSAN=1
build:clang-tsan --copt -fsanitize=thread
build:clang-tsan --linkopt -fsanitize=thread
build:clang-tsan --linkopt -fuse-ld=lld
build:clang-tsan --linkopt -static-libsan
lizan marked this conversation as resolved.
Show resolved Hide resolved
build:clang-tsan --define tcmalloc=disabled
# Needed due to https://github.com/libevent/libevent/issues/777
build:clang-tsan --copt -DEVENT__DISABLE_DEBUG_MODE
Expand All @@ -60,6 +61,7 @@ build:libc++ --action_env=CC
build:libc++ --action_env=CXX
build:libc++ --action_env=CXXFLAGS=-stdlib=libc++
build:libc++ --action_env=PATH
build:libc++ --host_linkopt=-fuse-ld=lld
lizan marked this conversation as resolved.
Show resolved Hide resolved
build:libc++ --define force_libcpp=enabled

# Optimize build for binary size reduction.
Expand Down
52 changes: 1 addition & 51 deletions bazel/cc_configure.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -2,57 +2,8 @@ load("@bazel_tools//tools/cpp:cc_configure.bzl", _upstream_cc_autoconf_impl = "c
load("@bazel_tools//tools/cpp:lib_cc_configure.bzl", "get_cpu_value")
load("@bazel_tools//tools/cpp:unix_cc_configure.bzl", "find_cc")

# Stub for `repository_ctx.which()` that always succeeds. See comments in
# `_find_cxx` for details.
def _quiet_fake_which(program):
return struct(_envoy_fake_which = program)

# Stub for `repository_ctx.which()` that always fails. See comments in
# `_find_cxx` for details.
def _noisy_fake_which(program):
return None

# Find a good path for the C++ compiler, by hooking into Bazel's C compiler
# detection. Uses `$CXX` if found, otherwise defaults to `g++` because Bazel
# defaults to `gcc`.
def _find_cxx(repository_ctx):
# Bazel's `find_cc` helper uses the repository context to inspect `$CC`.
# Replace this value with `$CXX` if set.
environ_cxx = repository_ctx.os.environ.get("CXX", "g++")
fake_os = struct(
environ = {"CC": environ_cxx},
)

# We can't directly assign `repository_ctx.which` to a struct attribute
# because Skylark doesn't support bound method references. Instead, stub
# out `which()` using a two-pass approach:
#
# * The first pass uses a stub that always succeeds, passing back a special
# value containing the original parameter.
# * If we detect the special value, we know that `find_cc` found a compiler
# name but don't know if that name could be resolved to an executable path.
# So do the `which()` call ourselves.
# * If our `which()` failed, call `find_cc` again with a dummy which that
# always fails. The error raised by `find_cc` will be identical to what Bazel
# would generate for a missing C compiler.
#
# See https://github.com/bazelbuild/bazel/issues/4644 for more context.
real_cxx = find_cc(struct(
which = _quiet_fake_which,
os = fake_os,
), {})
if hasattr(real_cxx, "_envoy_fake_which"):
real_cxx = repository_ctx.which(real_cxx._envoy_fake_which)
if real_cxx == None:
find_cc(struct(
which = _noisy_fake_which,
os = fake_os,
), {})
return real_cxx

def _build_envoy_cc_wrapper(repository_ctx):
real_cc = find_cc(repository_ctx, {})
real_cxx = _find_cxx(repository_ctx)

# Copy our CC wrapper script into @local_config_cc, with the true paths
# to the C and C++ compiler injected in. The wrapper will use these paths
Expand All @@ -64,7 +15,6 @@ def _build_envoy_cc_wrapper(repository_ctx):
repository_ctx.template("extra_tools/envoy_cc_wrapper", repository_ctx.attr._envoy_cc_wrapper, {
"{ENVOY_REAL_CC}": repr(str(real_cc)),
"{ENVOY_CFLAGS}": repr(str(repository_ctx.os.environ.get("CFLAGS", ""))),
"{ENVOY_REAL_CXX}": repr(str(real_cxx)),
"{ENVOY_CXXFLAGS}": repr(str(repository_ctx.os.environ.get("CXXFLAGS", ""))),
})
return repository_ctx.path("extra_tools/envoy_cc_wrapper")
Expand Down Expand Up @@ -93,6 +43,7 @@ cc_autoconf = repository_rule(
"ABI_VERSION",
"BAZEL_COMPILER",
"BAZEL_HOST_SYSTEM",
"BAZEL_CXXOPTS",
lizan marked this conversation as resolved.
Show resolved Hide resolved
"BAZEL_LINKOPTS",
"BAZEL_PYTHON",
"BAZEL_SH",
Expand All @@ -108,7 +59,6 @@ cc_autoconf = repository_rule(
"USE_CLANG_CL",
"CC",
"CFLAGS",
"CXX",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When building with GCC, does the final link happen with gcc or g++?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gcc

"CXXFLAGS",
"CC_CONFIGURE_DEBUG",
"CC_TOOLCHAIN_NAME",
Expand Down
27 changes: 15 additions & 12 deletions bazel/cc_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
import sys
import tempfile

envoy_real_cc = {ENVOY_REAL_CC}
envoy_real_cxx = {ENVOY_REAL_CXX}
compiler = {ENVOY_REAL_CC}
envoy_cflags = {ENVOY_CFLAGS}
envoy_cxxflags = {ENVOY_CXXFLAGS}

Expand All @@ -25,7 +24,7 @@ def sanitize_flagfile(in_path, out_fd):
if line != "-lstdc++\n":
os.write(out_fd, line)
elif "-stdlib=libc++" in envoy_cxxflags:
os.write(out_fd, "-lc++\n")
os.write(out_fd, "-l:libc++.a\n-l:libc++abi.a\n")


# Is the arg a flag indicating that we're building for C++ (rather than C)?
Expand All @@ -37,11 +36,9 @@ def is_cpp_flag(arg):
def modify_driver_args(input_driver_flags):
# Detect if we're building for C++ or vanilla C.
if any(map(is_cpp_flag, input_driver_flags)):
compiler = envoy_real_cxx
# Append CXXFLAGS to all C++ targets (this is mostly for dependencies).
argv = shlex.split(envoy_cxxflags)
else:
compiler = envoy_real_cc
# Append CFLAGS to all C targets (this is mostly for dependencies).
argv = shlex.split(envoy_cflags)

Expand All @@ -50,9 +47,8 @@ def modify_driver_args(input_driver_flags):
# b) replace all occurrences of -lstdc++ with -lc++ (when linking against libc++).
if "-static-libstdc++" in input_driver_flags or "-stdlib=libc++" in envoy_cxxflags:
for arg in input_driver_flags:
if arg == "-lstdc++":
if "-stdlib=libc++" in envoy_cxxflags:
argv.append("-lc++")
if arg in ("-lstdc++", "-static-libstdc++"):
pass
elif arg.startswith("-Wl,@"):
# tempfile.mkstemp will write to the out-of-sandbox tempdir
# unless the user has explicitly set environment variables
Expand All @@ -67,6 +63,13 @@ def modify_driver_args(input_driver_flags):
else:
argv += input_driver_flags

# This flags should after all libraries
if "-static-libstdc++" in input_driver_flags:
argv.append("-l:libstdc++.a")
if "-lstdc++" in input_driver_flags and "-stdlib=libc++" in envoy_cxxflags:
argv.append("-l:libc++.a")
argv.append("-l:libc++abi.a")

# Bazel will add -fuse-ld=gold in some cases, gcc/clang will take the last -fuse-ld argument,
# so whenever we see lld once, add it to the end.
if "-fuse-ld=lld" in argv:
Expand All @@ -87,13 +90,13 @@ def modify_driver_args(input_driver_flags):
# See https://github.com/envoyproxy/envoy/issues/2987
argv.append("-Wno-maybe-uninitialized")

return compiler, argv
return argv


def main():
# Append CXXFLAGS to correctly detect include paths for either libstdc++ or libc++.
if sys.argv[1:5] == ["-E", "-xc++", "-", "-v"]:
os.execv(envoy_real_cxx, [envoy_real_cxx] + sys.argv[1:] + shlex.split(envoy_cxxflags))
os.execv(compiler, [compiler] + sys.argv[1:] + shlex.split(envoy_cxxflags))

if sys.argv[1].startswith("@"):
# Read flags from file
Expand All @@ -102,7 +105,7 @@ def main():
input_driver_flags = fd.read().splitlines()

# Compute new args
compiler, new_driver_args = modify_driver_args(input_driver_flags)
new_driver_args = modify_driver_args(input_driver_flags)

# Write args to temp file
(new_flagfile_fd, new_flagfile_path) = tempfile.mkstemp(dir="./", suffix=".linker-params")
Expand All @@ -116,7 +119,7 @@ def main():
else:
# TODO(https://github.com/bazelbuild/bazel/issues/7687): Remove this branch
# when Bazel 0.27 is released.
compiler, new_args = modify_driver_args(sys.argv[1:])
new_args = modify_driver_args(sys.argv[1:])
lizan marked this conversation as resolved.
Show resolved Hide resolved

os.execv(compiler, [compiler] + new_args)

Expand Down
4 changes: 1 addition & 3 deletions bazel/envoy_internal.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,7 @@ def envoy_select_force_libcpp(if_libcpp, default = None):

def envoy_static_link_libstdcpp_linkopts():
return envoy_select_force_libcpp(
# TODO(PiotrSikora): statically link libc++ once that's possible.
# See: https://reviews.llvm.org/D53238
["-stdlib=libc++"],
["-stdlib=libc++", "-l:libc++.a", "-l:libc++abi.a", "-static-libgcc"],
["-static-libstdc++", "-static-libgcc"],
)

Expand Down
2 changes: 1 addition & 1 deletion bazel/envoy_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def _envoy_test_linkopts():
# TODO(mattklein123): It's not great that we universally link against the following libs.
# In particular, -latomic and -lrt are not needed on all platforms. Make this more granular.
"//conditions:default": ["-pthread", "-lrt", "-ldl"],
}) + envoy_select_force_libcpp(["-lc++fs"], ["-lstdc++fs", "-latomic"])
}) + envoy_select_force_libcpp([], ["-lstdc++fs", "-latomic"])

# Envoy C++ fuzz test targets. These are not included in coverage runs.
def envoy_cc_fuzz_test(name, corpus, deps = [], tags = [], **kwargs):
Expand Down
2 changes: 2 additions & 0 deletions bazel/foreign_cc/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ envoy_cmake_external(
"ENABLE_LIB_ONLY": "on",
"CMAKE_BUILD_TYPE": "RelWithDebInfo",
"CMAKE_INSTALL_LIBDIR": "lib",
"CMAKE_CXX_COMPILER_FORCED": "on",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we set CMAKE_C_COMPILER_FORCED set as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't add it because it wasn't needed, but fine to add as well. no strong opinion on this.

# Disable ranlib because it is not handled by bazel, and therefore
# doesn't respect custom toolchains such as the Android NDK,
# see https://github.com/bazelbuild/rules_foreign_cc/issues/252
Expand All @@ -147,6 +148,7 @@ envoy_cmake_external(
"YAML_CPP_BUILD_TESTS": "off",
"YAML_CPP_BUILD_TOOLS": "off",
"CMAKE_BUILD_TYPE": "RelWithDebInfo",
"CMAKE_CXX_COMPILER_FORCED": "on",
# Disable ranlib because it is not handled by bazel, and therefore
# doesn't respect custom toolchains such as the Android NDK,
# see https://github.com/bazelbuild/rules_foreign_cc/issues/252
Expand Down
4 changes: 1 addition & 3 deletions test/exe/envoy_static_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ fi
if [[ ${DYNLIBS} =~ "libc++" ]]; then
echo "libc++ is dynamically linked:"
echo "${DYNLIBS}"
# TODO(PiotrSikora): enforce this once there is a way to statically link libc++.
# See: https://reviews.llvm.org/D53238
exit 0
exit 1
lizan marked this conversation as resolved.
Show resolved Hide resolved
fi

if [[ ${DYNLIBS} =~ "libstdc++" || ${DYNLIBS} =~ "libgcc" ]]; then
Expand Down