From 773d232f528276338098578a28c19c742e3b4e7e Mon Sep 17 00:00:00 2001 From: yuzhy8701 <18453608+yuzhy8701@users.noreply.github.com> Date: Wed, 15 Feb 2023 06:28:30 -0800 Subject: [PATCH] Fix RPATHs for cc toolchain solib when sibling layout is used This should fix issue #16956 Includes the following changes: - Refactor logics to find runtime library search directories - Correctly compute RPATHs for toolchain solib. Closes #17276. PiperOrigin-RevId: 509815187 Change-Id: I7f2a2412aea6430fa712dd2836e18f9536757753 --- .../rules/cpp/LibrariesToLinkCollector.java | 430 ++++++++++++------ .../google/devtools/build/lib/rules/cpp/BUILD | 17 + .../cpp/LibrariesToLinkCollectorTest.java | 273 +++++++++++ 3 files changed, 581 insertions(+), 139 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollectorTest.java 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 a0052121121e84..abf3c1babf78be 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 @@ -15,6 +15,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; +import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -22,6 +23,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.RuleErrorConsumer; import com.google.devtools.build.lib.cmdline.LabelConstants; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; @@ -30,7 +32,9 @@ import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.rules.cpp.Link.LinkingMode; import com.google.devtools.build.lib.util.Pair; +import com.google.devtools.build.lib.vfs.OsPathPolicy; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import javax.annotation.Nullable; @@ -38,6 +42,9 @@ /** Class that goes over linker inputs and produces {@link LibraryToLinkValue}s */ public class LibrariesToLinkCollector { + private static final OsPathPolicy OS = OsPathPolicy.getFilePathOs(); + private static final Joiner PATH_JOINER = Joiner.on(PathFragment.SEPARATOR_CHAR); + private final boolean isNativeDeps; private final PathFragment toolchainLibrariesSolibDir; private final CppConfiguration cppConfiguration; @@ -50,11 +57,10 @@ public class LibrariesToLinkCollector { private final Artifact thinltoParamFile; private final FeatureConfiguration featureConfiguration; private final boolean needWholeArchive; - private final ImmutableList potentialExecRoots; - private final ImmutableList rpathRoots; private final boolean needToolchainLibrariesRpath; - private final Map ltoMap; private final RuleErrorConsumer ruleErrorConsumer; + private final Artifact output; + private final String workspaceName; public LibrariesToLinkCollector( boolean isNativeDeps, @@ -87,114 +93,13 @@ public LibrariesToLinkCollector( this.linkerInputs = linkerInputs; this.needWholeArchive = needWholeArchive; this.ruleErrorConsumer = ruleErrorConsumer; + this.output = output; + this.workspaceName = workspaceName; needToolchainLibrariesRpath = toolchainLibrariesSolibDir != null && (linkType.isDynamicLibrary() || (linkType == LinkTargetType.EXECUTABLE && linkingMode == LinkingMode.DYNAMIC)); - - // Calculate the correct relative value for the "-rpath" link option (which sets - // the search path for finding shared libraries). - if (isNativeDeps && cppConfiguration.shareNativeDeps()) { - // For shared native libraries, special symlinking is applied to ensure C++ - // toolchain libraries are available under $ORIGIN/_solib_[arch]. So we set the RPATH to find - // them. - // - // Note that we have to do this because $ORIGIN points to different paths for - // different targets. In other words, blaze-bin/d1/d2/d3/a_shareddeps.so and - // blaze-bin/d4/b_shareddeps.so have different path depths. The first could - // reference a standard blaze-bin/_solib_[arch] via $ORIGIN/../../../_solib[arch], - // 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. - potentialExecRoots = ImmutableList.of(); - rpathRoots = ImmutableList.of(ccToolchainProvider.getSolibDirectory() + "/"); - } else { - // 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. - // 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. - 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)); - // 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( - "../".repeat(output.getRunfilesPath().segmentCount() - 2) + workspaceName + "/"); - } - - potentialExecRoots = execRoots.build(); - rpathRoots = - potentialExecRoots.stream() - .map((execRoot) -> execRoot + ccToolchainProvider.getSolibDirectory() + "/") - .collect(toImmutableList()); - } - - ltoMap = generateLtoMap(); } /** @@ -236,6 +141,252 @@ public NestedSet getRuntimeLibrarySearchDirectories() { } } + private NestedSet collectToolchainRuntimeLibrarySearchDirectories( + ImmutableList potentialSolibParents) { + NestedSetBuilder runtimeLibrarySearchDirectories = NestedSetBuilder.linkOrder(); + if (!needToolchainLibrariesRpath) { + return runtimeLibrarySearchDirectories.build(); + } + + String toolchainLibrariesSolibName = toolchainLibrariesSolibDir.getBaseName(); + if (!(isNativeDeps && cppConfiguration.shareNativeDeps())) { + for (String potentialExecRoot : findToolchainSolibParents(potentialSolibParents)) { + runtimeLibrarySearchDirectories.add(potentialExecRoot + toolchainLibrariesSolibName + "/"); + } + } + if (isNativeDeps) { + runtimeLibrarySearchDirectories.add("../" + toolchainLibrariesSolibName + "/"); + runtimeLibrarySearchDirectories.add("."); + } + runtimeLibrarySearchDirectories.add(toolchainLibrariesSolibName + "/"); + + return runtimeLibrarySearchDirectories.build(); + } + + private ImmutableList findPotentialSolibParents() { + // 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. + // 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. + boolean isExternal = + output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX); + boolean usesLegacyRepositoryLayout = output.getRoot().isLegacy(); + ImmutableList.Builder solibParents = ImmutableList.builder(); + // Handles cases 1, 3, 4, 5, and 7. + solibParents.add("../".repeat(output.getRootRelativePath().segmentCount() - 1)); + // Handle cases 2 and 6. + String solibRepositoryName; + if (isExternal && !usesLegacyRepositoryLayout) { + // Case 6b + solibRepositoryName = output.getRunfilesPath().getSegment(1); + } else { + // Cases 2 and 6a + solibRepositoryName = workspaceName; + } + solibParents.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. + solibParents.add( + "../".repeat(output.getRunfilesPath().segmentCount() - 2) + workspaceName + "/"); + } + + return solibParents.build(); + } + + private ImmutableList findToolchainSolibParents( + ImmutableList potentialSolibParents) { + boolean usesLegacyRepositoryLayout = output.getRoot().isLegacy(); + // When -experimental_sibling_repository_layout is not enabled, the toolchain solib sits next to + // the solib_ directory - so that it shares the same parents. + if (usesLegacyRepositoryLayout) { + return potentialSolibParents; + } + + // When -experimental_sibling_repository_layout is enabled, the toolchain solib is located in + // these 2 places: + // 1. The `bin` directory of the repository where the toolchain target is declared (this is the + // parent directory of `toolchainLibrariesSolibDir`). + // 2. In `target.runfiles/` + // + // And the following factors affect what $ORIGIN is resolved to: + // * 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; + // + // 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: + // 2. main, direct, regular file: + // $ORIGIN: $EXECROOT/pkg + // solib root: $EXECROOT/pkg/file.runfiles/ + // 3. main, runfiles, symlink: + // $ORIGIN: $EXECROOT/pkg + // solib root: + // 4. main, runfiles, regular file: + // $ORIGIN: other_target.runfiles/main_repo/pkg + // solib root: other_target.runfiles/ + // 5. external, direct, symlink: + // $ORIGIN: $EXECROOT/../other_repo/pkg + // solib root: + // 6. external, direct, regular file: + // $ORIGIN: $EXECROOT/../other_repo/pkg + // solib root: $EXECROOT/../other_repo/pkg/file.runfiles/ + // 7. external, runfiles, symlink: + // $ORIGIN: $EXECROOT/../other_repo/pkg + // solib root: + // 8. external, runfiles, regular file: + // $ORIGIN: other_target.runfiles/some_repo/pkg + // solib root: other_target.runfiles/ + // + // For cases 1, 3, 5, 7, we need to compute the relative path from the output artifact to + // toolchain repo's bin directory. For 2 and 6, we walk down into `file.runfiles/`. For 4 and 8, we need to compute the relative path from the output runfile to + // under runfiles. + ImmutableList.Builder solibParents = ImmutableList.builder(); + + // Cases 1, 3, 5, 7 + PathFragment toolchainBinExecPath = toolchainLibrariesSolibDir.getParentDirectory(); + PathFragment binaryOriginExecPath = output.getExecPath().getParentDirectory(); + solibParents.add( + getRelative(binaryOriginExecPath, toolchainBinExecPath).getPathString() + + PathFragment.SEPARATOR_CHAR); + + // Cases 2 and 6 + String toolchainRunfilesRepoName = + getRunfilesRepoName(ccToolchainProvider.getCcToolchainLabel().getRepository()); + solibParents.add( + PATH_JOINER.join(output.getFilename() + ".runfiles", toolchainRunfilesRepoName) + + PathFragment.SEPARATOR_CHAR); + + // Cases 4 and 8 + String binaryRepoName = getRunfilesRepoName(output.getOwnerLabel().getRepository()); + PathFragment toolchainBinRunfilesPath = PathFragment.create(toolchainRunfilesRepoName); + PathFragment binaryOriginRunfilesPath = + PathFragment.create(binaryRepoName) + .getRelative(output.getRepositoryRelativePath()) + .getParentDirectory(); + solibParents.add( + getRelative(binaryOriginRunfilesPath, toolchainBinRunfilesPath).getPathString() + + PathFragment.SEPARATOR_CHAR); + + return solibParents.build(); + } + + private String getRunfilesRepoName(RepositoryName repo) { + if (repo.isMain()) { + return workspaceName; + } + return repo.getName(); + } + + /** + * Returns the relative {@link PathFragment} from "from" to "to". + * + *

Example 1: + * getRelative({@link PathFragment}.create("foo"), {@link PathFragment}.create("foo/bar/wiz")) + * returns "bar/wiz". + * + *

Example 2: + * getRelative({@link PathFragment}.create("foo/bar/wiz"), + * {@link PathFragment}.create("foo/wiz")) + * returns "../../wiz". + * + *

The following requirements / assumptions are made: 1) paths must be both relative; 2) they + * are assumed to be relative to the same location; 3) when the {@code from} path starts with + * {@code ..} prefixes, the prefix length must not exceed {@code ..} prefixes of the {@code to} + * path. + */ + static PathFragment getRelative(PathFragment from, PathFragment to) { + if (from.isAbsolute() || to.isAbsolute()) { + throw new IllegalArgumentException("Paths must be both relative."); + } + + final ImmutableList fromSegments = from.splitToListOfSegments(); + final ImmutableList toSegments = to.splitToListOfSegments(); + final int fromSegCount = fromSegments.size(); + final int toSegCount = toSegments.size(); + + int commonSegCount = 0; + while (commonSegCount < fromSegCount + && commonSegCount < toSegCount + && OS.equals(fromSegments.get(commonSegCount), toSegments.get(commonSegCount))) { + commonSegCount++; + } + + if (commonSegCount < fromSegCount && fromSegments.get(commonSegCount).equals("..")) { + throw new IllegalArgumentException( + "Unable to compute relative path from \"" + + from.getPathString() + + "\" to \"" + + to.getPathString() + + "\": too many leading \"..\" segments in from path."); + } + PathFragment relativePath = + PathFragment.create( + PATH_JOINER.join(Collections.nCopies(fromSegCount - commonSegCount, ".."))); + if (commonSegCount < toSegCount) { + relativePath = relativePath.getRelative(to.subFragment(commonSegCount, toSegCount)); + } + return relativePath; + } + /** * When linking a shared library fully or mostly static then we need to link in *all* dependent * files, not just what the shared library needs for its own code. This is done by wrapping all @@ -247,48 +398,43 @@ public NestedSet getRuntimeLibrarySearchDirectories() { */ public CollectedLibrariesToLink collectLibrariesToLink() { NestedSetBuilder librarySearchDirectories = NestedSetBuilder.linkOrder(); - NestedSetBuilder runtimeLibrarySearchDirectories = NestedSetBuilder.linkOrder(); ImmutableSet.Builder rpathRootsForExplicitSoDeps = ImmutableSet.builder(); NestedSetBuilder expandedLinkerInputsBuilder = NestedSetBuilder.linkOrder(); // List of command line parameters that need to be placed *outside* of // --whole-archive ... --no-whole-archive. SequenceBuilder librariesToLink = new SequenceBuilder(); - String toolchainLibrariesSolibName = - toolchainLibrariesSolibDir != null ? toolchainLibrariesSolibDir.getBaseName() : null; + ImmutableList potentialSolibParents; + ImmutableList rpathRoots; + // Calculate the correct relative value for the "-rpath" link option (which sets + // the search path for finding shared libraries). if (isNativeDeps && cppConfiguration.shareNativeDeps()) { - if (needToolchainLibrariesRpath) { - runtimeLibrarySearchDirectories.add("../" + toolchainLibrariesSolibName + "/"); - } + // For shared native libraries, special symlinking is applied to ensure C++ + // toolchain libraries are available under $ORIGIN/_solib_[arch]. So we set the RPATH to find + // them. + // + // Note that we have to do this because $ORIGIN points to different paths for + // different targets. In other words, blaze-bin/d1/d2/d3/a_shareddeps.so and + // blaze-bin/d4/b_shareddeps.so have different path depths. The first could + // reference a standard blaze-bin/_solib_[arch] via $ORIGIN/../../../_solib[arch], + // 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. + potentialSolibParents = ImmutableList.of(); + rpathRoots = ImmutableList.of(ccToolchainProvider.getSolibDirectory() + "/"); } else { - // For all other links, calculate the relative path from the output file to _solib_[arch] - // (the directory where all shared libraries are stored, which resides under the blaze-bin - // directory. In other words, given blaze-bin/my/package/binary, rpathRoot would be - // "../../_solib_[arch]". - if (needToolchainLibrariesRpath) { - for (String potentialExecRoot : potentialExecRoots) { - runtimeLibrarySearchDirectories.add( - potentialExecRoot + toolchainLibrariesSolibName + "/"); - } - } - if (isNativeDeps) { - // We also retain the $ORIGIN/ path to solibs that are in _solib_, as opposed to - // the package directory) - if (needToolchainLibrariesRpath) { - runtimeLibrarySearchDirectories.add("../" + toolchainLibrariesSolibName + "/"); - } - } - } - - if (needToolchainLibrariesRpath) { - if (isNativeDeps) { - runtimeLibrarySearchDirectories.add("."); - } - runtimeLibrarySearchDirectories.add(toolchainLibrariesSolibName + "/"); + potentialSolibParents = findPotentialSolibParents(); + rpathRoots = + potentialSolibParents.stream() + .map((execRoot) -> execRoot + ccToolchainProvider.getSolibDirectory() + "/") + .collect(toImmutableList()); } + Map ltoMap = generateLtoMap(); Pair includeSolibsPair = addLinkerInputs( + rpathRoots, + ltoMap, librarySearchDirectories, rpathRootsForExplicitSoDeps, librariesToLink, @@ -307,7 +453,8 @@ public CollectedLibrariesToLink collectLibrariesToLink() { } allRuntimeLibrarySearchDirectories.addAll(rpathRootsForExplicitSoDeps.build()); if (includeToolchainLibrariesSolibDir) { - allRuntimeLibrarySearchDirectories.addTransitive(runtimeLibrarySearchDirectories.build()); + allRuntimeLibrarySearchDirectories.addTransitive( + collectToolchainRuntimeLibrarySearchDirectories(potentialSolibParents)); } return new CollectedLibrariesToLink( @@ -318,6 +465,8 @@ public CollectedLibrariesToLink collectLibrariesToLink() { } private Pair addLinkerInputs( + ImmutableList rpathRoots, + Map ltoMap, NestedSetBuilder librarySearchDirectories, ImmutableSet.Builder rpathEntries, SequenceBuilder librariesToLink, @@ -366,9 +515,10 @@ private Pair addLinkerInputs( librariesToLink, expandedLinkerInputsBuilder, librarySearchDirectories, + rpathRoots, rpathEntries); } else { - addStaticInputLinkOptions(input, librariesToLink, expandedLinkerInputsBuilder); + addStaticInputLinkOptions(input, ltoMap, librariesToLink, expandedLinkerInputsBuilder); } } return Pair.of(includeSolibDir, includeToolchainLibrariesSolibDir); @@ -384,6 +534,7 @@ private void addDynamicInputLinkOptions( SequenceBuilder librariesToLink, NestedSetBuilder expandedLinkerInputsBuilder, NestedSetBuilder librarySearchDirectories, + ImmutableList rpathRoots, ImmutableSet.Builder rpathRootsForExplicitSoDeps) { Preconditions.checkState( input.getArtifactCategory() == ArtifactCategory.DYNAMIC_LIBRARY @@ -470,6 +621,7 @@ private void addDynamicInputLinkOptions( */ private void addStaticInputLinkOptions( LinkerInput input, + Map ltoMap, SequenceBuilder librariesToLink, NestedSetBuilder expandedLinkerInputsBuilder) { ArtifactCategory artifactCategory = input.getArtifactCategory(); @@ -528,7 +680,7 @@ private void addStaticInputLinkOptions( LinkerInputs.simpleLinkerInput( member, ArtifactCategory.OBJECT_FILE, - /* disableWholeArchive = */ false, + /* disableWholeArchive= */ false, member.getRootRelativePathString())); } ImmutableList nonLtoArchiveMembers = nonLtoArchiveMembersBuilder.build(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD index e25406fff02bc6..36172597190522 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/BUILD @@ -713,3 +713,20 @@ java_test( "//third_party:truth", ], ) + +java_test( + name = "LibrariesToLinkCollectorTest", + srcs = ["LibrariesToLinkCollectorTest.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", + "//src/main/java/com/google/devtools/build/lib/rules/cpp", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/test/java/com/google/devtools/build/lib/analysis/util", + "//src/test/java/com/google/devtools/build/lib/packages:testutil", + "//src/test/java/com/google/devtools/build/lib/testutil:TestConstants", + "//third_party:junit4", + "//third_party:truth", + ], +) diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollectorTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollectorTest.java new file mode 100644 index 00000000000000..f29ec5044d9071 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollectorTest.java @@ -0,0 +1,273 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.rules.cpp; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.rules.cpp.LibrariesToLinkCollector.getRelative; +import static org.junit.Assert.assertThrows; + +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.packages.util.Crosstool.CcToolchainConfig; +import com.google.devtools.build.lib.packages.util.ResourceLoader; +import com.google.devtools.build.lib.testutil.TestConstants; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class LibrariesToLinkCollectorTest extends BuildViewTestCase { + + @Test + public void getRalitive_returnsRelativePaths() { + assertThat(getRelative(PathFragment.create("foo"), PathFragment.create("foo/bar/baz"))) + .isEqualTo(PathFragment.create("bar/baz")); + assertThat(getRelative(PathFragment.create("foo/bar"), PathFragment.create("foo/bar/baz"))) + .isEqualTo(PathFragment.create("baz")); + assertThat(getRelative(PathFragment.create(""), PathFragment.create("foo"))) + .isEqualTo(PathFragment.create("foo")); + assertThat(getRelative(PathFragment.create(""), PathFragment.create("foo/bar"))) + .isEqualTo(PathFragment.create("foo/bar")); + assertThat( + getRelative(PathFragment.create("foo/bar"), PathFragment.create("foo/bar")) + .getPathString()) + .isEmpty(); + + assertThat(getRelative(PathFragment.create("foo/bar/baz"), PathFragment.create("foo"))) + .isEqualTo(PathFragment.create("../..")); + assertThat(getRelative(PathFragment.create("foo/bar/baz"), PathFragment.create("foo/bar"))) + .isEqualTo(PathFragment.create("..")); + assertThat(getRelative(PathFragment.create("foo"), PathFragment.create(""))) + .isEqualTo(PathFragment.create("..")); + assertThat(getRelative(PathFragment.create("foo/bar"), PathFragment.create(""))) + .isEqualTo(PathFragment.create("../..")); + assertThat(getRelative(PathFragment.create("foo/baz"), PathFragment.create("foo/bar"))) + .isEqualTo(PathFragment.create("../bar")); + assertThat(getRelative(PathFragment.create("bar"), PathFragment.create("foo"))) + .isEqualTo(PathFragment.create("../foo")); + + assertThat(getRelative(PathFragment.create("foo"), PathFragment.create("../foo"))) + .isEqualTo(PathFragment.create("../../foo")); + assertThat(getRelative(PathFragment.create(".."), PathFragment.create("../../foo"))) + .isEqualTo(PathFragment.create("../foo")); + assertThat(getRelative(PathFragment.create("../bar"), PathFragment.create("../../foo"))) + .isEqualTo(PathFragment.create("../../foo")); + } + + @Test + public void getRelative_throwsOnInvalidCases() { + assertThrows( + IllegalArgumentException.class, + () -> getRelative(PathFragment.create("/bar"), PathFragment.create("/foo"))); + assertThrows( + IllegalArgumentException.class, + () -> getRelative(PathFragment.create(".."), PathFragment.create(""))); + assertThrows( + IllegalArgumentException.class, + () -> getRelative(PathFragment.create("../../bar"), PathFragment.create("../foo"))); + } + + /* TODO: Add an integration test (maybe in cc_integration_test.sh) when a modular toolchain config + is available.*/ + @Test + public void dynamicLink_siblingLayout_externalBinary_rpath() throws Exception { + FileSystemUtils.appendIsoLatin1( + scratch.resolve("WORKSPACE"), "local_repository(name = 'src', path = 'src')"); + invalidatePackages(); + + scratch.file("src/WORKSPACE"); + scratch.file( + "src/test/BUILD", + "cc_binary(", + " name = 'foo',", + " srcs = ['some-dir/bar.so', 'some-other-dir/qux.so'],", + ")"); + scratch.file("src/test/some-dir/bar.so"); + scratch.file("src/test/some-other-dir/qux.so"); + + analysisMock.ccSupport().setupCcToolchainConfig(mockToolsConfig, CcToolchainConfig.builder()); + mockToolsConfig.create( + "toolchain/cc_toolchain_config.bzl", + ResourceLoader.readFromResources( + "com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl")); + scratch.file( + "toolchain/BUILD", + "load(':cc_toolchain_config.bzl', 'cc_toolchain_config')", + "filegroup(", + " name = 'empty',", + ")", + "filegroup(", + " name = 'static_runtime',", + " srcs = ['librt.a'],", + ")", + "filegroup(", + " name = 'dynamic_runtime',", + " srcs = ['librt.so'],", + ")", + "filegroup(", + " name = 'files',", + " srcs = ['librt.a', 'librt.so'],", + ")", + "cc_toolchain(", + " name = 'c_toolchain',", + " toolchain_identifier = 'toolchain-identifier-k8',", + " toolchain_config = ':k8-compiler_config',", + " all_files = ':files',", + " ar_files = ':empty',", + " as_files = ':empty',", + " compiler_files = ':empty',", + " dwp_files = ':empty',", + " linker_files = ':empty',", + " strip_files = ':empty',", + " objcopy_files = ':empty',", + " dynamic_runtime_lib = ':dynamic_runtime',", + " static_runtime_lib = ':static_runtime',", + ")", + CcToolchainConfig.builder() + .withFeatures( + CppRuleClasses.STATIC_LINK_CPP_RUNTIMES, + CppRuleClasses.SUPPORTS_DYNAMIC_LINKER, + "runtime_library_search_directories") + .build() + .getCcToolchainConfigRule(), + "toolchain(", + " name = 'toolchain',", + " toolchain_type = '" + TestConstants.TOOLS_REPOSITORY + "//tools/cpp:toolchain_type',", + " toolchain = ':c_toolchain',", + ")"); + scratch.file("toolchain/librt.a"); + scratch.file("toolchain/librt.so"); + analysisMock.ccSupport().setupCcToolchainConfig(mockToolsConfig, CcToolchainConfig.builder()); + + setBuildLanguageOptions("--experimental_sibling_repository_layout"); + useConfiguration( + "--extra_toolchains=//toolchain:toolchain", + "--dynamic_mode=fully", + "--incompatible_enable_cc_toolchain_resolution"); + + ConfiguredTarget target = getConfiguredTarget("@src//test:foo"); + assertThat(target).isNotNull(); + Artifact binary = getExecutable(target); + CppLinkAction linkAction = (CppLinkAction) getGeneratingAction(binary); + assertThat(linkAction).isNotNull(); + + String workspace = getTarget("//toolchain:toolchain").getPackage().getWorkspaceName(); + List linkArgs = linkAction.getArguments(); + assertThat(linkArgs) + .contains( + "--runtime_library=../../../../k8-fastbuild/bin/_solib__toolchain_Cc_Utoolchain/"); + assertThat(linkArgs) + .contains( + "--runtime_library=foo.runfiles/" + workspace + "/_solib__toolchain_Cc_Utoolchain/"); + assertThat(linkArgs) + .contains("--runtime_library=../../" + workspace + "/_solib__toolchain_Cc_Utoolchain/"); + } + + @Test + public void dynamicLink_siblingLayout_externalToolchain_rpath() throws Exception { + FileSystemUtils.appendIsoLatin1( + scratch.resolve("WORKSPACE"), "local_repository(name = 'toolchain', path = 'toolchain')"); + invalidatePackages(); + + scratch.file( + "src/test/BUILD", + "cc_binary(", + " name = 'foo',", + " srcs = ['some-dir/bar.so', 'some-other-dir/qux.so'],", + ")"); + scratch.file("src/test/some-dir/bar.so"); + scratch.file("src/test/some-other-dir/qux.so"); + + // The cc_toolchain_config.bzl cannot be placed in the external "toolchain" repo (b/269187186) + mockToolsConfig.create( + "cc_toolchain_config.bzl", + ResourceLoader.readFromResources( + "com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl")); + scratch.file("BUILD"); + + scratch.file("toolchain/WORKSPACE"); + scratch.file( + "toolchain/BUILD", + "load('@//:cc_toolchain_config.bzl', 'cc_toolchain_config')", + "filegroup(", + " name = 'empty',", + ")", + "filegroup(", + " name = 'static_runtime',", + " srcs = ['librt.a'],", + ")", + "filegroup(", + " name = 'dynamic_runtime',", + " srcs = ['librt.so'],", + ")", + "filegroup(", + " name = 'files',", + " srcs = ['librt.a', 'librt.so'],", + ")", + "cc_toolchain(", + " name = 'c_toolchain',", + " toolchain_identifier = 'toolchain-identifier-k8',", + " toolchain_config = ':k8-compiler_config',", + " all_files = ':files',", + " ar_files = ':empty',", + " as_files = ':empty',", + " compiler_files = ':empty',", + " dwp_files = ':empty',", + " linker_files = ':empty',", + " strip_files = ':empty',", + " objcopy_files = ':empty',", + " dynamic_runtime_lib = ':dynamic_runtime',", + " static_runtime_lib = ':static_runtime',", + ")", + CcToolchainConfig.builder() + .withFeatures( + CppRuleClasses.STATIC_LINK_CPP_RUNTIMES, + CppRuleClasses.SUPPORTS_DYNAMIC_LINKER, + "runtime_library_search_directories") + .build() + .getCcToolchainConfigRule(), + "toolchain(", + " name = 'toolchain',", + " toolchain_type = '" + TestConstants.TOOLS_REPOSITORY + "//tools/cpp:toolchain_type',", + " toolchain = ':c_toolchain',", + ")"); + scratch.file("toolchain/librt.a"); + scratch.file("toolchain/librt.so"); + analysisMock.ccSupport().setupCcToolchainConfig(mockToolsConfig, CcToolchainConfig.builder()); + + setBuildLanguageOptions("--experimental_sibling_repository_layout"); + useConfiguration( + "--extra_toolchains=@toolchain//:toolchain", + "--dynamic_mode=fully", + "--incompatible_enable_cc_toolchain_resolution"); + + ConfiguredTarget target = getConfiguredTarget("//src/test:foo"); + assertThat(target).isNotNull(); + Artifact binary = getExecutable(target); + CppLinkAction linkAction = (CppLinkAction) getGeneratingAction(binary); + assertThat(linkAction).isNotNull(); + + List linkArgs = linkAction.getArguments(); + assertThat(linkArgs) + .contains( + "--runtime_library=../../../../toolchain/k8-fastbuild/bin/_solib___Cc_Utoolchain/"); + assertThat(linkArgs) + .contains("--runtime_library=foo.runfiles/toolchain/_solib___Cc_Utoolchain/"); + assertThat(linkArgs).contains("--runtime_library=../../../toolchain/_solib___Cc_Utoolchain/"); + } +}