Fix local execution of external dynamically linked cc_* targets (#16253)

fmeum · ShreeM01 · web-flow · commit 1e25152906b6 · 2022-09-12T08:24:27.000-07:00
The fix for remote execution of external dynamically linked cc_* targets in 95a5bfd regressed local execution of such targets. This is fixed by emitting two rpaths in the case of an external target. Fixes #16008 (comment) Closes #16214. PiperOrigin-RevId: 473706330 Change-Id: I500f8b1f79e4d994323651fbb5964a730f1136e3 Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
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
@@ -13,6 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.lib.rules.cpp;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
@@ -40,7 +42,6 @@ public class LibrariesToLinkCollector {
   private final PathFragment toolchainLibrariesSolibDir;
   private final CppConfiguration cppConfiguration;
   private final CcToolchainProvider ccToolchainProvider;
-  private final Artifact outputArtifact;
   private final boolean isLtoIndexing;
   private final PathFragment solibDir;
   private final Iterable<? extends LinkerInput> linkerInputs;
@@ -49,7 +50,8 @@ public class LibrariesToLinkCollector {
   private final Artifact thinltoParamFile;
   private final FeatureConfiguration featureConfiguration;
   private final boolean needWholeArchive;
-  private final String rpathRoot;
+  private final ImmutableList<String> potentialExecRoots;
+  private final ImmutableList<String> rpathRoots;
   private final boolean needToolchainLibrariesRpath;
   private final Map<Artifact, Artifact> ltoMap;
   private final RuleErrorConsumer ruleErrorConsumer;
@@ -76,7 +78,6 @@ public LibrariesToLinkCollector(
     this.cppConfiguration = cppConfiguration;
     this.ccToolchainProvider = toolchain;
     this.toolchainLibrariesSolibDir = toolchainLibrariesSolibDir;
-    this.outputArtifact = output;
     this.solibDir = solibDir;
     this.isLtoIndexing = isLtoIndexing;
     this.allLtoArtifacts = allLtoArtifacts;
@@ -106,22 +107,83 @@ public LibrariesToLinkCollector(
       // and the second could use $ORIGIN/../_solib_[arch]. But since this is a shared
       // artifact, both are symlinks to the same place, so
       // there's no *one* RPATH setting that fits all targets involved in the sharing.
-      rpathRoot = ccToolchainProvider.getSolibDirectory() + "/";
+      potentialExecRoots = ImmutableList.of();
+      rpathRoots = ImmutableList.of(ccToolchainProvider.getSolibDirectory() + "/");
     } else {
-      // When executed from within a runfiles directory, the binary lies under a path such as
-      // target.runfiles/some_repo/pkg/file, whereas the solib directory is located under
-      // target.runfiles/main_repo.
-      PathFragment runfilesPath = outputArtifact.getRunfilesPath();
-      String runfilesExecRoot;
-      if (runfilesPath.startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX)) {
-        // runfilesPath is of the form ../some_repo/pkg/file, walk back some_repo/pkg and then
-        // descend into the main workspace.
-        runfilesExecRoot = Strings.repeat("../", runfilesPath.segmentCount() - 2) + workspaceName + "/";
-      } else {
-        // runfilesPath is of the form pkg/file, walk back pkg to reach the main workspace.
-        runfilesExecRoot = Strings.repeat("../", runfilesPath.segmentCount() - 1);
+      // The runtime location of the solib directory relative to the binary depends on four factors:
+      //
+      // * whether the binary is contained in the main repository or an external repository;
+      // * whether the binary is executed directly or from a runfiles tree;
+      // * whether the binary is staged as a symlink (sandboxed execution; local execution if the
+      //   binary is in the runfiles of another target) or a regular file (remote execution) - the
+      //   dynamic linker follows sandbox and runfiles symlinks into its location under the
+      //   unsandboxed execroot, which thus becomes the effective $ORIGIN;
+      // * whether --experimental_sibling_repository_layout is enabled or not.
+      //
+      // The rpaths emitted into the binary thus have to cover the following cases (assuming that
+      // the binary target is located in the pkg `pkg` and has name `file`) for the directory used
+      // as $ORIGIN by the dynamic linker and the directory containing the solib directories:
+      //
+      // 1. main, direct, symlink:
+      //    $ORIGIN:    $EXECROOT/pkg
+      //    solib root: $EXECROOT
+      // 2. main, direct, regular file:
+      //    $ORIGIN:    $EXECROOT/pkg
+      //    solib root: $EXECROOT/pkg/file.runfiles/main_repo
+      // 3. main, runfiles, symlink:
+      //    $ORIGIN:    $EXECROOT/pkg
+      //    solib root: $EXECROOT
+      // 4. main, runfiles, regular file:
+      //    $ORIGIN:    other_target.runfiles/main_repo/pkg
+      //    solib root: other_target.runfiles/main_repo
+      // 5a. external, direct, symlink:
+      //    $ORIGIN:    $EXECROOT/external/other_repo/pkg
+      //    solib root: $EXECROOT
+      // 5b. external, direct, symlink, with --experimental_sibling_repository_layout:
+      //    $ORIGIN:    $EXECROOT/../other_repo/pkg
+      //    solib root: $EXECROOT/../other_repo
+      // 6a. external, direct, regular file:
+      //    $ORIGIN:    $EXECROOT/external/other_repo/pkg
+      //    solib root: $EXECROOT/external/other_repo/pkg/file.runfiles/main_repo
+      // 6b. external, direct, regular file, with --experimental_sibling_repository_layout:
+      //    $ORIGIN:    $EXECROOT/../other_repo/pkg
+      //    solib root: $EXECROOT/../other_repo/pkg/file.runfiles/other_repo
+      // 7a. external, runfiles, symlink:
+      //    $ORIGIN:    $EXECROOT/external/other_repo/pkg
+      //    solib root: $EXECROOT
+      // 7b. external, runfiles, symlink, with --experimental_sibling_repository_layout:
+      //    $ORIGIN:    $EXECROOT/../other_repo/pkg
+      //    solib root: $EXECROOT/../other_repo
+      // 8a. external, runfiles, regular file:
+      //    $ORIGIN:    other_target.runfiles/some_repo/pkg
+      //    solib root: other_target.runfiles/main_repo
+      // 8b. external, runfiles, regular file, with --experimental_sibling_repository_layout:
+      //    $ORIGIN:    other_target.runfiles/some_repo/pkg
+      //    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.
+      // 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`.
+      ImmutableList.Builder<String> execRoots = ImmutableList.builder();
+      // Handles cases 1, 3, 4, 5, and 7.
+      execRoots.add(Strings.repeat("../", output.getRootRelativePath().segmentCount() - 1));
+      if (output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX)
+          && output.getRoot().isLegacy()) {
+        // 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(
+            Strings.repeat("../", output.getRunfilesPath().segmentCount() - 2) + workspaceName
+                + "/");
       }
-      rpathRoot = runfilesExecRoot + ccToolchainProvider.getSolibDirectory() + "/";
+
+      potentialExecRoots = execRoots.build();
+      rpathRoots =
+          potentialExecRoots.stream()
+              .map((execRoot) -> execRoot + ccToolchainProvider.getSolibDirectory() + "/")
+              .collect(toImmutableList());
     }
 
     ltoMap = generateLtoMap();
@@ -196,10 +258,10 @@ public CollectedLibrariesToLink collectLibrariesToLink() {
       // directory. In other words, given blaze-bin/my/package/binary, rpathRoot would be
       // "../../_solib_[arch]".
       if (needToolchainLibrariesRpath) {
-        runtimeLibrarySearchDirectories.add(
-            Strings.repeat("../", outputArtifact.getRootRelativePath().segmentCount() - 1)
-                + toolchainLibrariesSolibName
-                + "/");
+        for (String potentialExecRoot : potentialExecRoots) {
+          runtimeLibrarySearchDirectories.add(
+              potentialExecRoot + toolchainLibrariesSolibName + "/");
+        }
       }
       if (isNativeDeps) {
         // We also retain the $ORIGIN/ path to solibs that are in _solib_<arch>, as opposed to
@@ -231,7 +293,9 @@ public CollectedLibrariesToLink collectLibrariesToLink() {
     NestedSetBuilder<String> allRuntimeLibrarySearchDirectories = NestedSetBuilder.linkOrder();
     // rpath ordering matters for performance; first add the one where most libraries are found.
     if (includeSolibDir) {
-      allRuntimeLibrarySearchDirectories.add(rpathRoot);
+      for (String rpathRoot : rpathRoots) {
+        allRuntimeLibrarySearchDirectories.add(rpathRoot);
+      }
     }
     allRuntimeLibrarySearchDirectories.addAll(rpathRootsForExplicitSoDeps.build());
     if (includeToolchainLibrariesSolibDir) {
@@ -346,17 +410,21 @@ private void addDynamicInputLinkOptions(
       // When all dynamic deps are built in transitioned configurations, the default solib dir is
       // not created. While resolving paths, the dynamic linker stops at the first directory that
       // does not exist, even when followed by "../". We thus have to normalize the relative path.
-      String relativePathToRoot =
-          rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString();
-      String normalizedPathToRoot = PathFragment.create(relativePathToRoot).getPathString();
-      rpathRootsForExplicitSoDeps.add(normalizedPathToRoot);
+      for (String rpathRoot : rpathRoots) {
+        String relativePathToRoot =
+            rpathRoot + dotdots + libDir.relativeTo(commonParent).getPathString();
+        String normalizedPathToRoot = PathFragment.create(relativePathToRoot).getPathString();
+        rpathRootsForExplicitSoDeps.add(normalizedPathToRoot);
+      }
 
       // Unless running locally, libraries will be available under the root relative path, so we
       // should add that to the rpath as well.
       if (inputArtifact.getRootRelativePathString().startsWith("_solib_")) {
         PathFragment artifactPathUnderSolib = inputArtifact.getRootRelativePath().subFragment(1);
-        rpathRootsForExplicitSoDeps.add(
-            rpathRoot + artifactPathUnderSolib.getParentDirectory().getPathString());
+        for (String rpathRoot : rpathRoots) {
+          rpathRootsForExplicitSoDeps.add(
+              rpathRoot + artifactPathUnderSolib.getParentDirectory().getPathString());
+        }
       }
     }
 
diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh
@@ -1382,4 +1382,92 @@ EOF
       fail "bazel build should've succeeded with --features=compiler_param_file"
 }
 
+function external_cc_test_setup() {
+  cat >> WORKSPACE <<'EOF'
+local_repository(
+  name = "other_repo",
+  path = "other_repo",
+)
+EOF
+
+  mkdir -p other_repo
+  touch other_repo/WORKSPACE
+
+  mkdir -p other_repo/lib
+  cat > other_repo/lib/BUILD <<'EOF'
+cc_library(
+  name = "lib",
+  srcs = ["lib.cpp"],
+  hdrs = ["lib.h"],
+  visibility = ["//visibility:public"],
+)
+EOF
+  cat > other_repo/lib/lib.h <<'EOF'
+void print_greeting();
+EOF
+  cat > other_repo/lib/lib.cpp <<'EOF'
+#include <cstdio>
+void print_greeting() {
+  printf("Hello, world!\n");
+}
+EOF
+
+  mkdir -p other_repo/test
+  cat > other_repo/test/BUILD <<'EOF'
+cc_test(
+  name = "test",
+  srcs = ["test.cpp"],
+  deps = ["//lib"],
+)
+EOF
+  cat > other_repo/test/test.cpp <<'EOF'
+#include "lib/lib.h"
+int main() {
+  print_greeting();
+}
+EOF
+}
+
+function test_external_cc_test_sandboxed() {
+  [ "$PLATFORM" != "windows" ] || return 0
+
+  external_cc_test_setup
+
+  bazel test \
+      --test_output=errors \
+      --strategy=sandboxed \
+      @other_repo//test >& $TEST_log || fail "Test should pass"
+}
+
+function test_external_cc_test_sandboxed_sibling_repository_layout() {
+  [ "$PLATFORM" != "windows" ] || return 0
+
+  external_cc_test_setup
+
+  bazel test \
+      --test_output=errors \
+      --strategy=sandboxed \
+      --experimental_sibling_repository_layout \
+      @other_repo//test >& $TEST_log || fail "Test should pass"
+}
+
+function test_external_cc_test_local() {
+  external_cc_test_setup
+
+  bazel test \
+      --test_output=errors \
+      --strategy=local \
+      @other_repo//test >& $TEST_log || fail "Test should pass"
+}
+
+function test_external_cc_test_local_sibling_repository_layout() {
+  external_cc_test_setup
+
+  bazel test \
+      --test_output=errors \
+      --strategy=local \
+      --experimental_sibling_repository_layout \
+      @other_repo//test >& $TEST_log || fail "Test should pass"
+}
+
 run_suite "cc_integration_test"
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -3811,14 +3811,7 @@ EOF
   expect_log "Executing genrule .* failed: (Exit 1):"
 }
 
-function test_external_cc_test() {
-  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
-
+function setup_external_cc_test() {
   cat >> WORKSPACE <<'EOF'
 local_repository(
   name = "other_repo",
@@ -3862,10 +3855,38 @@ int main() {
   print_greeting();
 }
 EOF
+}
+
+function test_external_cc_test() {
+  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_external_cc_test
+
+  bazel test \
+      --test_output=errors \
+      --remote_executor=grpc://localhost:${worker_port} \
+      @other_repo//test >& $TEST_log || fail "Test should pass"
+}
+
+function test_external_cc_test_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_external_cc_test
 
   bazel test \
       --test_output=errors \
       --remote_executor=grpc://localhost:${worker_port} \
+      --experimental_sibling_repository_layout \
       @other_repo//test >& $TEST_log || fail "Test should pass"
 }
 
