From e3dcfa54baec45a6b247143106f7ab689df424cd Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 17 Oct 2022 06:16:40 -0700 Subject: [PATCH] Roll forward of https://github.com/bazelbuild/bazel/commit/c7aa39224eec36ced303cd4370170c60a2d1d79c: Fix dynamic library lookup with remotely executed tools When a cc_binary tool is executed directly (e.g. in a genrule) with remote execution, its dynamic dependencies are only available from the solib directory in the runfiles directory. This commit adds an additional rpath entry for this directory. Since target names may contain commas and the newly added rpaths contain target names, the `-Xlinker` compiler is now used everywhere instead of the `-Wl` flag, which doesn't support commas in linker arguments. Stacked on #16214 Closes #16215. PiperOrigin-RevId: 481621970 Change-Id: I5969a15ef0c828477f893216b6b702a3bad6b6fa --- .../build/lib/rules/cpp/CppActionConfigs.java | 28 ++-- .../rules/cpp/LibrariesToLinkCollector.java | 21 ++- .../util/mock/osx_cc_toolchain_config.bzl | 5 +- .../cpp/CcImportBaseConfiguredTargetTest.java | 8 +- .../cpp/CcLibraryConfiguredTargetTest.java | 28 +++- .../lib/rules/cpp/CppLinkActionTest.java | 4 +- .../lib/rules/objc/ObjcRuleTestCase.java | 5 +- .../bazel/remote/remote_execution_test.sh | 126 ++++++++++++++++++ tools/cpp/osx_cc_wrapper.sh.tpl | 2 +- tools/cpp/unix_cc_toolchain_config.bzl | 15 ++- tools/osx/crosstool/cc_toolchain_config.bzl | 5 +- 11 files changed, 217 insertions(+), 30 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java index aa8658f1f1c31f..1ade3e2dfc0103 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java @@ -549,18 +549,25 @@ public static ImmutableList getLegacyFeatures( " flag_group {", " expand_if_true: 'is_cc_test'", // TODO(b/27153401): This should probably be @loader_path on osx. - " flag: ", - " '-Wl,-rpath,$EXEC_ORIGIN/%{runtime_library_search_directories}'", + " flag: '-Xlinker'", + " flag: '-rpath'", + " flag: '-Xlinker'", + " flag: '$EXEC_ORIGIN/%{runtime_library_search_directories}'", " }", " flag_group {", " expand_if_false: 'is_cc_test'", ifLinux( platform, - " flag: '-Wl,-rpath,$ORIGIN/" - + "%{runtime_library_search_directories}'"), + " flag: '-Xlinker'", + " flag: '-rpath'", + " flag: '-Xlinker'", + " flag: '$ORIGIN/" + "%{runtime_library_search_directories}'"), ifMac( platform, - " flag: '-Wl,-rpath,@loader_path/" + " flag: '-Xlinker'", + " flag: '-rpath'", + " flag: '-Xlinker'", + " flag: '@loader_path/" + "%{runtime_library_search_directories}'"), " }", " }", @@ -579,11 +586,16 @@ public static ImmutableList getLegacyFeatures( " flag_group {", ifLinux( platform, - " flag: '-Wl,-rpath,$ORIGIN/" - + "%{runtime_library_search_directories}'"), + " flag: '-Xlinker'", + " flag: '-rpath'", + " flag: '-Xlinker'", + " flag: '$ORIGIN/" + "%{runtime_library_search_directories}'"), ifMac( platform, - " flag: '-Wl,-rpath,@loader_path/" + " flag: '-Xlinker'", + " flag: '-rpath'", + " flag: '-Xlinker'", + " flag: '@loader_path/" + "%{runtime_library_search_directories}'"), " }", " }", diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java index 5e38bc46e4375c..a0052121121e84 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java @@ -162,16 +162,25 @@ public LibrariesToLinkCollector( // solib root: other_target.runfiles/some_repo // // Cases 1, 3, 4, 5, 7, and 8b are covered by an rpath that walks up the root relative path. + // Cases 2 and 6 covered by walking into file.runfiles/main_repo. // Case 8a is covered by walking up some_repo/pkg and then into main_repo. - // Cases 2 and 6 are currently not covered as they would require an rpath containing the - // binary filename, which may contain commas that would clash with the `-Wl` argument used to - // pass the rpath to the linker. - // TODO(#14600): Fix this by using `-Xlinker` instead of `-Wl`. + boolean isExternal = + output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX); + boolean usesLegacyRepositoryLayout = output.getRoot().isLegacy(); ImmutableList.Builder execRoots = ImmutableList.builder(); // Handles cases 1, 3, 4, 5, and 7. execRoots.add("../".repeat(output.getRootRelativePath().segmentCount() - 1)); - if (output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX) - && output.getRoot().isLegacy()) { + // Handle cases 2 and 6. + String solibRepositoryName; + if (isExternal && !usesLegacyRepositoryLayout) { + // Case 6b + solibRepositoryName = output.getRunfilesPath().getSegment(1); + } else { + // Cases 2 and 6a + solibRepositoryName = workspaceName; + } + execRoots.add(output.getFilename() + ".runfiles/" + solibRepositoryName + "/"); + if (isExternal && usesLegacyRepositoryLayout) { // Handles case 8a. The runfiles path is of the form ../some_repo/pkg/file and we need to // walk up some_repo/pkg and then down into main_repo. execRoots.add( diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl b/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl index 339c0b8e9eb38c..b3015cb2740214 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl +++ b/src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl @@ -7922,7 +7922,10 @@ def _impl(ctx): flag_groups = [ flag_group( flags = [ - "-Wl,-rpath,$ORIGIN/%{runtime_library_search_directories}", + "-Xlinker", + "-rpath", + "-Xlinker", + "@loader_path/%{runtime_library_search_directories}", ], iterate_over = "runtime_library_search_directories", expand_if_available = "runtime_library_search_directories", diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java index 59c7819f7479b8..b8029da6fbb4bd 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java @@ -444,7 +444,9 @@ public void testCcImportWithSharedLibraryAddsRpathEntry() throws Exception { Artifact mainBin = getBinArtifact("bin", main); CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin); List linkArgv = action.getLinkCommandLine().arguments(); - assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua"); + assertThat(linkArgv) + .containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua") + .inOrder(); } @Test @@ -525,6 +527,8 @@ public void testCcImportWithSharedLibraryWithTransitionAddsRpathEntry() throws E Artifact mainBin = getBinArtifact("bin", main); CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin); List linkArgv = action.getLinkCommandLine().arguments(); - assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua"); + assertThat(linkArgv) + .containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua") + .inOrder(); } } diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java index 3667288d65c123..45e019c1915c14 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java @@ -2124,7 +2124,7 @@ public void testRpathIsNotAddedWhenThereAreNoSoDeps() throws Exception { Artifact mainBin = getBinArtifact("main", main); CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin); assertThat(Joiner.on(" ").join(action.getLinkCommandLine().arguments())) - .doesNotContain("-Wl,-rpath"); + .doesNotContain("-Xlinker -rpath"); } @Test @@ -2157,12 +2157,21 @@ public void testRpathAndLinkPathsWithoutTransitions() throws Exception { Artifact mainBin = getBinArtifact("main", main); CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin); List linkArgv = action.getLinkCommandLine().arguments(); - assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/"); + assertThat(linkArgv) + .containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/") + .inOrder(); + assertThat(linkArgv) + .containsAtLeast( + "-Xlinker", + "-rpath", + "-Xlinker", + "$ORIGIN/main.runfiles/" + TestConstants.WORKSPACE_NAME + "/_solib_k8/") + .inOrder(); assertThat(linkArgv) .contains("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild/bin/_solib_k8"); assertThat(linkArgv).contains("-lno-transition_Slibdep1"); assertThat(Joiner.on(" ").join(linkArgv)) - .doesNotContain("-Wl,-rpath,$ORIGIN/../_solib_k8/../../../k8-fastbuild-ST-"); + .doesNotContain("-Xlinker -rpath -Xlinker $ORIGIN/../_solib_k8/../../../k8-fastbuild-ST-"); assertThat(Joiner.on(" ").join(linkArgv)) .doesNotContain("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild-ST-"); assertThat(Joiner.on(" ").join(linkArgv)).doesNotContain("-lST-"); @@ -2217,9 +2226,18 @@ public void testRpathRootIsAddedEvenWithTransitionedDepsOnly() throws Exception Artifact mainBin = getBinArtifact("main", main); CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin); List linkArgv = action.getLinkCommandLine().arguments(); - assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/"); + assertThat(linkArgv) + .containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/") + .inOrder(); + assertThat(linkArgv) + .containsAtLeast( + "-Xlinker", + "-rpath", + "-Xlinker", + "$ORIGIN/main.runfiles/" + TestConstants.WORKSPACE_NAME + "/_solib_k8/") + .inOrder(); assertThat(Joiner.on(" ").join(linkArgv)) - .contains("-Wl,-rpath,$ORIGIN/../../../k8-fastbuild-ST-"); + .contains("-Xlinker -rpath -Xlinker $ORIGIN/../../../k8-fastbuild-ST-"); assertThat(Joiner.on(" ").join(linkArgv)) .contains("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild-ST-"); assertThat(Joiner.on(" ").join(linkArgv)).containsMatch("-lST-[0-9a-f]+_transition_Slibdep2"); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java index d8c8423f75d682..08ead2a154cca8 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java @@ -260,7 +260,9 @@ public void testLibOptsAndLibSrcsAreInCorrectOrder() throws Exception { ".* -L[^ ]*some-dir(?= ).* -L[^ ]*some-other-dir(?= ).* " + "-lbar -l:qux.so(?= ).* -ldl -lutil .*"); assertThat(Joiner.on(" ").join(arguments)) - .matches(".* -Wl,-rpath[^ ]*some-dir(?= ).* -Wl,-rpath[^ ]*some-other-dir .*"); + .matches( + ".* -Xlinker -rpath -Xlinker [^ ]*some-dir(?= ).* -Xlinker -rpath -Xlinker [^" + + " ]*some-other-dir .*"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java index 05d9091f09142f..0fcd6bead3acb2 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java @@ -521,7 +521,8 @@ protected static void addAppleBinaryStarlarkRule(Scratch scratch) throws Excepti " if binary_type == 'dylib':", " linkopts.append('-dynamiclib')", " elif binary_type == 'loadable_bundle':", - " linkopts.extend(['-bundle', '-Wl,-rpath,@loader_path/Frameworks'])", + " linkopts.extend(['-bundle', '-Xlinker', '-rpath', '-Xlinker'," + + " '@loader_path/Frameworks'])", " if ctx.attr.bundle_loader:", " bundle_loader = ctx.attr.bundle_loader", " bundle_loader_file = bundle_loader[apple_common.AppleExecutableBinary].binary", @@ -782,7 +783,7 @@ protected void checkBundleLoaderIsCorrectlyPassedToTheLinker(RuleType ruleType) assertThat(Joiner.on(" ").join(linkAction.getArguments())) .contains("-bundle_loader " + getBinArtifact("bin_lipobin", binTarget).getExecPath()); assertThat(Joiner.on(" ").join(linkAction.getArguments())) - .contains("-Wl,-rpath,@loader_path/Frameworks"); + .contains("-Xlinker -rpath -Xlinker @loader_path/Frameworks"); } protected Action lipoLibAction(String libLabel) throws Exception { diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh index 6f3fa424a64caf..b9c0cbf933b703 100755 --- a/src/test/shell/bazel/remote/remote_execution_test.sh +++ b/src/test/shell/bazel/remote/remote_execution_test.sh @@ -3062,4 +3062,130 @@ function test_unresolved_symlink_spawn_absolute() { do_test_unresolved_symlink spawn /non/existent } +function setup_cc_binary_tool_with_dynamic_deps() { + local repo=$1 + + cat >> WORKSPACE <<'EOF' +local_repository( + name = "other_repo", + path = "other_repo", +) +EOF + + mkdir -p $repo + touch $repo/WORKSPACE + + mkdir -p $repo/lib + # Use a comma in the target name as that is known to be problematic whith -Wl, + # which is commonly used to pass rpaths to the linker. + cat > $repo/lib/BUILD <<'EOF' +cc_binary( + name = "l,ib", + srcs = ["lib.cpp"], + linkshared = True, + linkstatic = True, +) + +cc_import( + name = "dynamic_l,ib", + shared_library = ":l,ib", + hdrs = ["lib.h"], + visibility = ["//visibility:public"], +) +EOF + cat > $repo/lib/lib.h <<'EOF' +void print_greeting(); +EOF + cat > $repo/lib/lib.cpp <<'EOF' +#include +void print_greeting() { + printf("Hello, world!\n"); +} +EOF + + mkdir -p $repo/pkg + cat > $repo/pkg/BUILD <<'EOF' +cc_binary( + name = "tool", + srcs = ["tool.cpp"], + deps = ["//lib:dynamic_l,ib"], +) + +genrule( + name = "rule", + outs = ["out"], + cmd = "$(location :tool) > $@", + tools = [":tool"], +) +EOF + cat > $repo/pkg/tool.cpp <<'EOF' +#include "lib/lib.h" +int main() { + print_greeting(); +} +EOF +} + +function test_cc_binary_tool_with_dynamic_deps() { + if [[ "$PLATFORM" == "darwin" ]]; then + # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting + # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of + # action executors in order to select the appropriate Xcode toolchain. + return 0 + fi + + setup_cc_binary_tool_with_dynamic_deps . + + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + //pkg:rule >& $TEST_log || fail "Build should succeed" +} + +function test_cc_binary_tool_with_dynamic_deps_sibling_repository_layout() { + if [[ "$PLATFORM" == "darwin" ]]; then + # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting + # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of + # action executors in order to select the appropriate Xcode toolchain. + return 0 + fi + + setup_cc_binary_tool_with_dynamic_deps . + + bazel build \ + --experimental_sibling_repository_layout \ + --remote_executor=grpc://localhost:${worker_port} \ + //pkg:rule >& $TEST_log || fail "Build should succeed" +} + +function test_external_cc_binary_tool_with_dynamic_deps() { + if [[ "$PLATFORM" == "darwin" ]]; then + # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting + # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of + # action executors in order to select the appropriate Xcode toolchain. + return 0 + fi + + setup_cc_binary_tool_with_dynamic_deps other_repo + + bazel build \ + --remote_executor=grpc://localhost:${worker_port} \ + @other_repo//pkg:rule >& $TEST_log || fail "Build should succeed" +} + +function test_external_cc_binary_tool_with_dynamic_deps_sibling_repository_layout() { + if [[ "$PLATFORM" == "darwin" ]]; then + # TODO(b/37355380): This test is disabled due to RemoteWorker not supporting + # setting SDKROOT and DEVELOPER_DIR appropriately, as is required of + # action executors in order to select the appropriate Xcode toolchain. + return 0 + fi + + setup_cc_binary_tool_with_dynamic_deps other_repo + + bazel build \ + --experimental_sibling_repository_layout \ + --remote_executor=grpc://localhost:${worker_port} \ + @other_repo//pkg:rule >& $TEST_log || fail "Build should succeed" +} + run_suite "Remote execution and remote cache tests" diff --git a/tools/cpp/osx_cc_wrapper.sh.tpl b/tools/cpp/osx_cc_wrapper.sh.tpl index 25ed2ece8e8b4e..ab58d1f1df4fd5 100644 --- a/tools/cpp/osx_cc_wrapper.sh.tpl +++ b/tools/cpp/osx_cc_wrapper.sh.tpl @@ -42,7 +42,7 @@ function parse_option() { LIBS="${BASH_REMATCH[1]} $LIBS" elif [[ "$opt" =~ ^-L(.*)$ ]]; then LIB_DIRS="${BASH_REMATCH[1]} $LIB_DIRS" - elif [[ "$opt" =~ ^-Wl,-rpath,\@loader_path/(.*)$ ]]; then + elif [[ "$opt" =~ ^\@loader_path/(.*)$ ]]; then RPATHS="${BASH_REMATCH[1]} ${RPATHS}" elif [[ "$opt" = "-o" ]]; then # output is coming diff --git a/tools/cpp/unix_cc_toolchain_config.bzl b/tools/cpp/unix_cc_toolchain_config.bzl index 5768a746578804..b951b09c24eed0 100644 --- a/tools/cpp/unix_cc_toolchain_config.bzl +++ b/tools/cpp/unix_cc_toolchain_config.bzl @@ -486,13 +486,19 @@ def _impl(ctx): flag_groups = [ flag_group( flags = [ - "-Wl,-rpath,$EXEC_ORIGIN/%{runtime_library_search_directories}", + "-Xlinker", + "-rpath", + "-Xlinker", + "$EXEC_ORIGIN/%{runtime_library_search_directories}", ], expand_if_true = "is_cc_test", ), flag_group( flags = [ - "-Wl,-rpath,$ORIGIN/%{runtime_library_search_directories}", + "-Xlinker", + "-rpath", + "-Xlinker", + "$ORIGIN/%{runtime_library_search_directories}", ], expand_if_false = "is_cc_test", ), @@ -513,7 +519,10 @@ def _impl(ctx): flag_groups = [ flag_group( flags = [ - "-Wl,-rpath,$ORIGIN/%{runtime_library_search_directories}", + "-Xlinker", + "-rpath", + "-Xlinker", + "$ORIGIN/%{runtime_library_search_directories}", ], ), ], diff --git a/tools/osx/crosstool/cc_toolchain_config.bzl b/tools/osx/crosstool/cc_toolchain_config.bzl index a3d6eeb30c9135..276c3ceb6797d7 100644 --- a/tools/osx/crosstool/cc_toolchain_config.bzl +++ b/tools/osx/crosstool/cc_toolchain_config.bzl @@ -777,7 +777,10 @@ def _impl(ctx): flag_groups = [ flag_group( flags = [ - "-Wl,-rpath,@loader_path/%{runtime_library_search_directories}", + "-Xlinker", + "-rpath", + "-Xlinker", + "@loader_path/%{runtime_library_search_directories}", ], iterate_over = "runtime_library_search_directories", expand_if_available = "runtime_library_search_directories",