Skip to content

Commit

Permalink
Fix local execution of external dynamically linked cc_* targets
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@fmeum
fmeum committed Sep 3, 2022
1 parent c4a1ab8 commit 952c901
Show file tree
Hide file tree
Showing 2 changed files with 145 additions and 27 deletions.
106 changes: 79 additions & 27 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -106,22 +107,67 @@ 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;
// The runtime location of the solib directory relative to the binary depends on three
// independent factors:
//
// * whether the binary is contained in the main repository or an external repository;
// * whether the binary is executed directly or from the runfiles of another target;
// * 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.
//
// The rpaths emitted into the binary thus have to cover the following eight 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
// 5. external, direct, symlink:
// $ORIGIN: $EXECROOT/external/other_repo/pkg
// solib root: $EXECROOT
// 6. external, direct, regular file:
// $ORIGIN: $EXECROOT/external/other_repo/pkg
// solib root: $EXECROOT/external/other_repo/pkg/file.runfiles/main_repo
// 7. external, runfiles, symlink:
// $ORIGIN: $EXECROOT/external/other_repo/pkg
// solib root: $EXECROOT
// 8. external, runfiles, regular file:
// $ORIGIN: other_target.runfiles/some_repo/pkg
// solib root: other_target.runfiles/main_repo
//
// Cases 1, 3, 4, 5, and 7 are covered by an rpath that walks up the runfiles path.
// Case 8 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`.
PathFragment runfilesPath = output.getRunfilesPath();
ImmutableList.Builder<String> execRoots = ImmutableList.builder();
// Handles cases 1, 3, 4, 5, and 7.
execRoots.add("../".repeat(runfilesPath.segmentCount() - 1));
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 = "../".repeat(runfilesPath.segmentCount() - 2) + workspaceName + "/";
} else {
// runfilesPath is of the form pkg/file, walk back pkg to reach the main workspace.
runfilesExecRoot = "../".repeat(runfilesPath.segmentCount() - 1);
// Handles cases 8.
execRoots.add("../".repeat(runfilesPath.segmentCount() - 2) + workspaceName + "/");
}
rpathRoot = runfilesExecRoot + ccToolchainProvider.getSolibDirectory() + "/";

potentialExecRoots = execRoots.build();
rpathRoots = potentialExecRoots.stream()
.map((execRoot) -> execRoot + ccToolchainProvider.getSolibDirectory() + "/")
.collect(toImmutableList());
}

ltoMap = generateLtoMap();
Expand Down Expand Up @@ -196,10 +242,10 @@ public CollectedLibrariesToLink collectLibrariesToLink() {
// directory. In other words, given blaze-bin/my/package/binary, rpathRoot would be
// "../../_solib_[arch]".
if (needToolchainLibrariesRpath) {
runtimeLibrarySearchDirectories.add(
"../".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
Expand Down Expand Up @@ -231,7 +277,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) {
Expand Down Expand Up @@ -346,17 +394,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());
}
}
}

Expand Down
66 changes: 66 additions & 0 deletions src/test/shell/bazel/cc_integration_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1506,4 +1506,70 @@ EOF
expect_log "\"PWD\": \"/proc/self/cwd\""
}

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_local() {
external_cc_test_setup

bazel test \
--test_output=errors \
--strategy=local \
@other_repo//test >& $TEST_log || fail "Test should pass"
}

run_suite "cc_integration_test"

0 comments on commit 952c901

Please sign in to comment.