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 bazelbuild#16214

Closes bazelbuild#16215.

PiperOrigin-RevId: 474792702
Change-Id: I2bfa1fd77be83d7bfe54ba591af5cb0ad0e630fc

src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java

@@ -549,18 +549,25 @@ public static ImmutableList<CToolchain.Feature> 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<CToolchain.Feature> 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}'"),
                         "    }",
                         "  }",

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<String> 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(

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",

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<String> 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<String> 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();
   }
 }

src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java

@@ -2122,7 +2122,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
@@ -2155,12 +2155,21 @@ public void testRpathAndLinkPathsWithoutTransitions() throws Exception {
     Artifact mainBin = getBinArtifact("main", main);
     CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin);
     List<String> 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-");
@@ -2215,9 +2224,18 @@ public void testRpathRootIsAddedEvenWithTransitionedDepsOnly() throws Exception
     Artifact mainBin = getBinArtifact("main", main);
     CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin);
     List<String> 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");

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

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 {

src/test/shell/bazel/remote/remote_execution_test.sh

@@ -3037,4 +3037,130 @@ EOF
   [[ $(cat bazel-bin/pkg/b.txt) == non/existent ]] || fail "expected symlink target to be 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 <cstdio>
+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"

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

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}",
                                 ],
                             ),
                         ],

tools/osx/crosstool/cc_toolchain_config.bzl

@@ -793,7 +793,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",