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

linkopts flags are not passed when cpp toolchains are enabled #5291

Closed
nlopezgi opened this issue May 29, 2018 · 20 comments
Closed

linkopts flags are not passed when cpp toolchains are enabled #5291

nlopezgi opened this issue May 29, 2018 · 20 comments
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules type: bug

Comments

@nlopezgi
Copy link
Contributor

Description of the problem / feature request:

linkopts flags (and possibly other c related flags) are not passed when cpp toolchains are enabled (e.g., when flag --enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type is used)

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

  1. START a docker container with the sanitizer toolchain
    docker run
    -it
    -e USER_ID="$(id -u)"
    -v /temp/workspace:/temp/workspace
    -v /temp/build_output:/temp/build_output
    -w /temp/workspace
    --cap-add=SYS_PTRACE
    gcr.io/cloud-marketplace/google/rbe-debian8@sha256:0d5db936f8fa04638ca31e4fc117415068dca43dc343d605c0db2a15f433a327
    /bin/bash

  2. Install Bazel (+fix an issue in the container)
    export BAZEL_VERSION=0.13.0 && wget https://github.com/bazelbuild/bazel/releases/download/${BAZEL_VERSION}/bazel-${BAZEL_VERSION}-installer-linux-x86_64.sh -O /temp/bazel-installer.sh
    chmod +x /temp/bazel-installer.sh
    /temp/bazel-installer.sh
    update-ca-certificates -f
    apt-get update && apt-get install vim -y

  3. Clone abseil-cpp
    git clone https://github.com/abseil/abseil-cpp
    cd abseil-cpp

  4. Add the following content to the abseil-cpp WORKSPACE file:
    http_archive(
    name = 'bazel_toolchains_test_suite_repo',
    url = 'https://github.com/bazelbuild/bazel-toolchains/archive/4653c01284d8a4a536f8f9bb47b7d10f94c549e7.tar.gz',
    strip_prefix = 'bazel-toolchains-4653c01284d8a4a536f8f9bb47b7d10f94c549e7',
    sha256 = '1c4a532b396c698e6467a1548554571cb85fa091e472b05e398ebc836c315d77',
    )

register_execution_platforms(
'@bazel_toolchains_test_suite_repo//configs/ubuntu16_04_clang/1.0:rbe_ubuntu1604',
)

register_toolchains(
'@bazel_toolchains_test_suite_repo//configs/ubuntu16_04_clang/1.0/bazel_0.13.0/cpp:cc-toolchain-clang-x86_64-default',
)

  1. Run either a failing or passing command:

-This command fails:
bazel test --crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/default:toolchain --toolchain_resolution_debug --copt=-gmlt --strip=never --copt=-fsanitize=address --linkopt=-fsanitize=address --enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type --spawn_strategy=standalone -s //absl/debugging:disabled_leak_check_test

-This command works:
bazel test --crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/default:toolchain --toolchain_resolution_debug --copt=-gmlt --strip=never --copt=-fsanitize=address --linkopt=-fsanitize=address --spawn_strategy=standalone -s //absl/debugging:disabled_leak_check_test

What operating system are you running Bazel on?

Debian8
(specifically, this container: gcr.io/cloud-marketplace/google/rbe-debian8@sha256:0d5db936f8fa04638ca31e4fc117415068dca43dc343d605c0db2a15f433a327)

What's the output of bazel info release?

Using 0.13.0 from installer (see instructions for repro above)

Have you found anything relevant by searching the web?

Started thread with @katre - he confirmed "it's a known problem that there are many c++ flags that are resolved via the CppConfiguration, not the CcToolchain"

@katre katre added type: bug P2 We'll consider working on this in future. (Assignee optional) category: rules > C++ P1 I'll work on this now. (Assignee required) and removed P2 We'll consider working on this in future. (Assignee optional) labels May 29, 2018
@hlopko
Copy link
Member

hlopko commented May 30, 2018

Can I see the crosstool for the toolchain somewhere?

@nlopezgi
Copy link
Contributor Author

@nlopezgi
Copy link
Contributor Author

Note, I did try to replace use of linkopts flag for use of linker_flag in CROSSTOOL, and with linker_flag the builds work (CROSSTOOL with added linker_flag is not submitted atm).

@hlopko
Copy link
Member

hlopko commented Jun 1, 2018

Nick I think the issue is that toolchains currently see host configuration when created, not target configuration which is used with legacy cctoolchain selection. I believe that if you use --host_linkopt you will see them when using toolchains.

@hlopko hlopko self-assigned this Jun 1, 2018
@nlopezgi
Copy link
Contributor Author

nlopezgi commented Jun 4, 2018

Using host_linkopts does solve the problem. Thanks!

@nlopezgi nlopezgi closed this as completed Jun 4, 2018
@nlopezgi
Copy link
Contributor Author

nlopezgi commented Jun 4, 2018

Hi Marcel, I've been doing some more thorough testing of the solution you suggested. It seems to work fine as long as i use the same host and target configuration (i.e., sanitizer config for both host and target), but not when I actually need to use the sanitizer configuration only for target configuration. Specifically, for asan, tsan and ubsan, the solution worked, as I am using the same crosstool for host and target. However, for msan builds, I'm using a different crosstool for host (--host_crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/default:toolchain) and another for target (--crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/msan:toolchain). The main difference between these two configs is that the host config does not use libc++.

Debugging further, it seems like if I add the --host_linkopt=-fsanitize=memory it gets applied both for host and target configuration. That is, it seems the --linkopt=-fsanitize=memory is not having any effect, and the reason why --host_linkopt=-fsanitize=xxx works for other sanitizers is just because I'm using the same config for target and host.

To repro you can follow the instructions above, but instead of checking out abseil, you need to check out grpc:
3. Clone grpc:
git clone https://github.com/abseil/abseil-cpp project_src
git submodule update --init

follow step 4 as indicated above

  1. Run the following commands:
  • Your suggestion works for a single test target (using the same config for host and target):

bazel test --crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/msan:toolchain --extra_execution_platforms=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0:rbe_debian8 --action_env=BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 --extra_toolchains=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/cpp:cc-toolchain-clang-x86_64-msan --copt=-fsanitize=memory --cxxopt=-stdlib=libc++ --copt=-gmlt --copt=-fsanitize-memory-track-origins --host_linkopt=-fsanitize=memory --linkopt=-fsanitize=memory --strip=never --test_output=errors --action_env=LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH --enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type -s //test/cpp/microbenchmarks:noop-benchmark

  • But there are build failures when genrules that use host binaries are executed:

bazel test --crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/msan:toolchain --extra_execution_platforms=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0:rbe_debian8 --action_env=BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 --extra_toolchains=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/cpp:cc-toolchain-clang-x86_64-msan --copt=-fsanitize=memory --cxxopt=-stdlib=libc++ --copt=-gmlt --copt=-fsanitize-memory-track-origins --host_linkopt=-fsanitize=memory --linkopt=-fsanitize=memory --strip=never --test_output=errors --action_env=LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH --enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type -s @com_google_protobuf//:generate_js_well_known_types_embed

Error message:
ERROR: /root/.cache/bazel/_bazel_root/7e958634aed2e0b9513fa7cce861a282/external/com_google_protobuf/BUILD:262:1: Executing genrule @com_google_protobuf//:generate_js_well_known_types_embed failed (Exit 127): bash failed: error executing command
(cd /root/.cache/bazel/_bazel_root/7e958634aed2e0b9513fa7cce861a282/execroot/com_github_grpc_grpc &&
exec env -
PATH=/bin:/usr/bin
/bin/bash -c 'source external/bazel_tools/tools/genrule/genrule-setup.sh; bazel-out/host/bin/external/com_google_protobuf/js_embed external/com_google_protobuf/src/google/protobuf/compiler/js/well_known_types/any.js external/com_google_protobuf/src/google/protobuf/compiler/js/well_known_types/struct.js external/com_google_protobuf/src/google/protobuf/compiler/js/well_known_types/timestamp.js > bazel-out/host/genfiles/external/com_google_protobuf/src/google/protobuf/compiler/js/well_known_types_embed.cc')
bazel-out/host/bin/external/com_google_protobuf/js_embed: error while loading shared libraries: libc++.so.1: cannot open shared object file: No such file or directory

  • If I use the distinct configuration for host and target (i.e., added the host_crosstool_top):
    bazel --host_jvm_args=-Dbazel.DigestFunction=SHA256 --nomaster_bazelrc test --crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/msan:toolchain --host_crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/default:toolchain --extra_execution_platforms=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0:rbe_debian8 --action_env=BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 --extra_toolchains=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/cpp:cc-toolchain-clang-x86_64-msan --copt=-fsanitize=memory --cxxopt=-stdlib=libc++ --copt=-gmlt --copt=-fsanitize-memory-track-origins --host_linkopt=-fsanitize=memory --linkopt=-fsanitize=memory --strip=never --test_output=errors --action_env=LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH --enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type -s //test/cpp/microbenchmarks:noop-benchmark

  • This alternative also fails (with host crosstool top && w/o --host_linkopt=-fsanitize=memory):
    bazel --host_jvm_args=-Dbazel.DigestFunction=SHA256 --nomaster_bazelrc test --crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/msan:toolchain --host_crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/default:toolchain --extra_execution_platforms=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0:rbe_debian8 --action_env=BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 --extra_toolchains=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/cpp:cc-toolchain-clang-x86_64-msan --copt=-fsanitize=memory --cxxopt=-stdlib=libc++ --copt=-gmlt --copt=-fsanitize-memory-track-origins --linkopt=-fsanitize=memory --strip=never --test_output=errors --action_env=LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH --enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type -s //test/cpp/microbenchmarks:noop-benchmark

  • Works for all targets (does not use enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type, uses distinct host vs target crosstool, does not explicitly add host_linkopts):

bazel --host_jvm_args=-Dbazel.DigestFunction=SHA256 --nomaster_bazelrc test --crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/msan:toolchain --host_crosstool_top=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/default:toolchain --extra_execution_platforms=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0:rbe_debian8 --action_env=BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 --extra_toolchains=@bazel_toolchains_test_suite_repo//configs/debian8_clang/0.3.0/bazel_0.13.0/cpp:cc-toolchain-clang-x86_64-msan --copt=-fsanitize=memory --cxxopt=-stdlib=libc++ --copt=-gmlt --copt=-fsanitize-memory-track-origins --linkopt=-fsanitize=memory --strip=never --test_output=errors --action_env=LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH -s //test/cpp/microbenchmarks:noop-benchmark

@nlopezgi nlopezgi reopened this Jun 4, 2018
@hlopko
Copy link
Member

hlopko commented Jun 8, 2018

Hi Nick,
I'm sorry I didn't explicitly mention that the host linkopts was only a workaround. The root of the problem is that platforms are evaluated with host configuration, and we need them to be evaluated in target configuration. @katre is working on it, but it's not trivial. Pinging @katre to comment on timeline.

@katre
Copy link
Member

katre commented Jun 11, 2018

Nick and I discussed this in person recently, here's the summary:

I've sent a design out for review on the topic of Platforms and Configurations. I'm not linking it here because there were many excellent points raised in the initial review, and I will be revising it heavily, but one of the things I expect to get out of that effort are richer dependency transitions for toolchains. Once that is in place (and it should take a few months, not be immediate), we should be able to correctly associate the linkopts flag with the cc_toolchain dependencies in a sensible way.

@nlopezgi
Copy link
Contributor Author

The timeline should be fine as long as we can guarantee the sanitizer builds that we have working now will keep working. I'm particularly concerned about changes like the one that caused #5133 that made us switch to use platform flags (i.e., using bazel with no gcc now requires you add the platform flags) without full support for the platforms path which is the one that spurred this issue (i.e., we were fine with not using any platform flags and stick to linkopts flags until #5133).

@hlopko
Copy link
Member

hlopko commented Nov 30, 2018

Hi John,

is this already fixed? TLDR: target platform saw linkopts from the host configuration, because platforms were analyzed in the host configuration.

@katre
Copy link
Member

katre commented Nov 30, 2018

Sadly not: I have plans for fixing configuration transitions for toolchains, but they aren't prioritized currently. I'm hoping to get back to that next quarter.

@hlopko
Copy link
Member

hlopko commented Nov 30, 2018

Pardon my ignorance, does it mean that we cannot reasonably use linkopts with platforms currently?

@katre
Copy link
Member

katre commented Nov 30, 2018

This doesn't just affect linkopts, it affects any case where the host_foo flag is different from the foo flag.

The core problem here is that it's not obvious which version should be used: the common case is that the toolchain is executing on something more like the host platform than the target, but that's not always the case.

Can the linkopts be set on the cc_toolchain itself? That seems superior to using a flag in this case.

@hlopko
Copy link
Member

hlopko commented Nov 30, 2018

You ask good questions. I think we can actually delete --host_copt/--host_linkopt and friends, and have them unified. They're not used much. Let me investigate on Monday.

@katre
Copy link
Member

katre commented Jan 22, 2019

Picking this back up: We can probably remove most of the host_ flags and convert them to attributes on cc_toolchain and related targets, which is arguably a better choice anyway (it puts configuration info into the build tree, rather than on the command line).

The major holdout is --host_compilation_mode, which I am investigating.

@hlopko
Copy link
Member

hlopko commented Apr 5, 2019

Hi @nlopezgi (and @xingao267)

so in 0.25, we rewrote a bunch of C++ rules logic to work properly also with platforms, even when cc_toolchain is analyzed in the host configuration. Android and Apple split transitions are not yet migrated though.

What we could really use now is testing that everything works well in Bazel, and also in bazel-toolchains. Do you have C++ builds that you can try switching to platforms?

Incompatible change covering this effort is #7260, please report there.

As for this concrete issue, I believe it's fixed by #6826. Please complain if it's not true.

@hlopko hlopko closed this as completed Apr 5, 2019
@nlopezgi
Copy link
Contributor Author

nlopezgi commented Apr 5, 2019

Setting up tests that use enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type with 0.25.0rc1, will report back on results.

@katre
Copy link
Member

katre commented Apr 5, 2019

That flag is being phased out. Please use --incompatible_enable_cc_toolchain_resolution.

@nlopezgi
Copy link
Contributor Author

nlopezgi commented Apr 5, 2019

thanks for the clarification. Testing then with --incompatible_enable_cc_toolchain_resolution

@nlopezgi
Copy link
Contributor Author

nlopezgi commented Apr 5, 2019

Ran tests with 0.25.0rc1 with both enabled_toolchain_types=@bazel_tools//tools/cpp:toolchain_type and --incompatible_enable_cc_toolchain_resolution. Ran tests for abseil asan builds successfully. Tests do fail with any version prior to 0.25.0rc1. So this lgtm. Will report if we see any other issues related to this. Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

No branches or pull requests

3 participants