Skip to content

Commit

Permalink
Fix rpath for symlinks in cc_shared_library
Browse files Browse the repository at this point in the history 
The rpath were set based on the path of the linker inputs relative to the output being created by the linking action. However, for shared objects after creating the linking action in most cases we also create a symlink. The dynamic loader takes into account the location of the symlink to resolve $ORIGIN in the rpath, not the target of the symlink. The RPATH was therefore wrong in most cases.

This misunderstanding of how the dynamic loader treats symlinks also influenced changes introduced in 95ae4db. All the features introduced in that change are not needed:
- No need to switch from RUNPATH to RPATH
- No need to stop RPATHs from being added to a shared library
- cc_test can avoid linking everything shared library (just like cc_binary)
- We don't ever need to link indirect shared libraries into a cc_binary except on Windows

This CL also fixes the name of the symlink for a shared library which was mangled so that the original name is preserved. This is important because if a distribution artifact is linked against the library with the mangled name, even if the shared library was installed in the system and the RPATH set correctly, the dynamic loader won't find it.

Fixes #18790

RELNOTES:none
PiperOrigin-RevId: 543768869
Change-Id: I48a2a6553e97cd611814051e731c874552d1de27
  • Loading branch information
@oquenchil
oquenchil authored and Copybara-Service committed Jun 27, 2023
1 parent dba9e43 commit 044a14c
Show file tree
Hide file tree
Showing 11 changed files with 144 additions and 171 deletions.
48 changes: 38 additions & 10 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingHelper.java
View file
Expand Up @@ -694,7 +694,8 @@ private boolean createDynamicLinkAction(
"-Wl,-soname="
+ SolibSymlinkAction.getDynamicLibrarySoname(
linkerOutput.getRootRelativePath(),
/* preserveName= */ false,
/* preserveName= */ dynamicLinkType
!= LinkTargetType.NODEPS_DYNAMIC_LIBRARY,
actionConstructionContext.getConfiguration().getMnemonic()));
}
}
Expand Down Expand Up @@ -799,6 +800,10 @@ private boolean createDynamicLinkAction(
if (dynamicLinkActionBuilder.getAllLtoBackendArtifacts() != null) {
ccLinkingOutputs.addAllLtoArtifacts(dynamicLinkActionBuilder.getAllLtoBackendArtifacts());
}
Artifact implLibraryLinkArtifact = getDynamicLibrarySolibSymlinkOutput(linkerOutput);
if (implLibraryLinkArtifact != null) {
dynamicLinkActionBuilder.setDynamicLibrarySolibSymlinkOutput(implLibraryLinkArtifact);
}
CppLinkAction dynamicLinkAction = dynamicLinkActionBuilder.build();
if (dynamicLinkType.isExecutable()) {
ccLinkingOutputs.setExecutable(linkerOutput);
Expand All @@ -824,14 +829,16 @@ private boolean createDynamicLinkAction(
}
libraryToLinkBuilder.setDynamicLibrary(dynamicLibrary.getArtifact());
} else {
Artifact implLibraryLinkArtifact =
SolibSymlinkAction.getDynamicLibrarySymlink(
/* actionRegistry= */ actionRegistry,
/* actionConstructionContext= */ actionConstructionContext,
ccToolchain.getSolibDirectory(),
dynamicLibrary.getArtifact(),
/* preserveName= */ false,
/* prefixConsumer= */ false);
if (dynamicLinkType == LinkTargetType.NODEPS_DYNAMIC_LIBRARY) {
implLibraryLinkArtifact =
SolibSymlinkAction.getDynamicLibrarySymlink(
/* actionRegistry= */ actionRegistry,
/* actionConstructionContext= */ actionConstructionContext,
ccToolchain.getSolibDirectory(),
dynamicLibrary.getArtifact(),
/* preserveName= */ false,
/* prefixConsumer= */ false);
}
libraryToLinkBuilder.setDynamicLibrary(implLibraryLinkArtifact);
libraryToLinkBuilder.setResolvedSymlinkDynamicLibrary(dynamicLibrary.getArtifact());

Expand All @@ -842,7 +849,8 @@ private boolean createDynamicLinkAction(
/* actionConstructionContext= */ actionConstructionContext,
ccToolchain.getSolibDirectory(),
interfaceLibrary.getArtifact(),
/* preserveName= */ false,
// Need to preserve name for transitive shared libraries that may be distributed.
/* preserveName= */ dynamicLinkType != LinkTargetType.NODEPS_DYNAMIC_LIBRARY,
/* prefixConsumer= */ false);
libraryToLinkBuilder.setInterfaceLibrary(libraryLinkArtifact);
libraryToLinkBuilder.setResolvedSymlinkInterfaceLibrary(interfaceLibrary.getArtifact());
Expand Down Expand Up @@ -1001,4 +1009,24 @@ private static List<LinkerInputs.LibraryToLink> convertLibraryToLinkListToLinker
}
return librariesToLinkBuilder.build();
}

@Nullable
private Artifact getDynamicLibrarySolibSymlinkOutput(Artifact linkerOutputArtifact) {
if (dynamicLinkType != LinkTargetType.DYNAMIC_LIBRARY
|| neverlink
|| featureConfiguration.isEnabled(CppRuleClasses.COPY_DYNAMIC_LIBRARIES_TO_BINARY)) {
return null;
}
return SolibSymlinkAction.getDynamicLibrarySymlink(
/* actionRegistry= */ actionRegistry,
/* actionConstructionContext= */ actionConstructionContext,
ccToolchain.getSolibDirectory(),
linkerOutputArtifact,
// For transitive shared libraries we want to preserve the name of the original library so
// that distribution artifacts can be linked against it and not against the mangled name.
// This makes it possible to find the library on the system if the RPATH has been set
// properly.
/* preserveName= */ true,
/* prefixConsumer= */ false);
}
}
23 changes: 14 additions & 9 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
View file
Expand Up @@ -147,6 +147,10 @@ public Artifact create(
private boolean isStampingEnabled;
private final Map<String, String> executionInfo = new LinkedHashMap<>();

// We have to add the dynamicLibrarySolibOutput to the CppLinkActionBuilder so that it knows how
// to set up the RPATH properly with respect to the symlink itself and not the original library.
private Artifact dynamicLibrarySolibSymlinkOutput;

/**
* Creates a builder that builds {@link CppLinkAction}s.
*
Expand Down Expand Up @@ -814,7 +818,8 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
nonExpandedLinkerInputs,
needWholeArchive,
ruleErrorConsumer,
((RuleContext) actionConstructionContext).getWorkspaceName());
((RuleContext) actionConstructionContext).getWorkspaceName(),
dynamicLibrarySolibSymlinkOutput);
CollectedLibrariesToLink collectedLibrariesToLink =
librariesToLinkCollector.collectLibrariesToLink();

Expand All @@ -831,13 +836,6 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
userLinkFlags.addAll(cppConfiguration.getLtoIndexOptions());
}

NestedSet<String> runtimeLibrarySearchDirectories =
collectedLibrariesToLink.getRuntimeLibrarySearchDirectories();
if (linkType.getActionName().equals(CppActionNames.CPP_LINK_DYNAMIC_LIBRARY)
&& featureConfiguration.isEnabled(
CppRuleClasses.EXCLUDE_BAZEL_RPATHS_IN_TRANSITIVE_LIBS_FEATURE_NAME)) {
runtimeLibrarySearchDirectories = null;
}
variables =
LinkBuildVariables.setupVariables(
((RuleContext) actionConstructionContext).getStarlarkThread(),
Expand Down Expand Up @@ -865,7 +863,7 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
ltoOutputRootPrefix,
defFile != null ? defFile.getExecPathString() : null,
fdoContext,
runtimeLibrarySearchDirectories,
collectedLibrariesToLink.getRuntimeLibrarySearchDirectories(),
collectedLibrariesToLink.getLibrariesToLink(),
collectedLibrariesToLink.getLibrarySearchDirectories(),
/* addIfsoRelatedVariables= */ true);
Expand Down Expand Up @@ -1563,4 +1561,11 @@ public CppLinkActionBuilder addExecutionInfo(Map<String, String> executionInfo)
this.executionInfo.putAll(executionInfo);
return this;
}

@CanIgnoreReturnValue
public CppLinkActionBuilder setDynamicLibrarySolibSymlinkOutput(
Artifact dynamicLibrarySolibSymlinkOutput) {
this.dynamicLibrarySolibSymlinkOutput = dynamicLibrarySolibSymlinkOutput;
return this;
}
}
31 changes: 0 additions & 31 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
View file
Expand Up @@ -517,37 +517,6 @@ public static ToolchainTypeRequirement ccToolchainTypeRequirement(RuleDefinition
*/
public static final String LEGACY_IS_CC_TEST_FEATURE_NAME = "legacy_is_cc_test";

/**
* By default Bazel will be embed runtime search directories (RPATHS) in transitive shared
* libraries, however, for Linux they are wrong in most cases since the runfiles directory where
* the transitive shared library will live will not be known at the time of linking. The runfiles
* directory is decided by dependent rules, we don't know where those dependents will live. For
* Mac (where it works differently by searching from the library's path instead of the main
* binary's) the loader paths (not rpaths) are correct so the paths will work.
*
* <p>This feature controls whether Bazel will embed those rpaths into the transitive shared
* library.
*/
public static final String EXCLUDE_BAZEL_RPATHS_IN_TRANSITIVE_LIBS_FEATURE_NAME =
"exclude_bazel_rpaths_in_transitive_libs";

/**
* With this feature enabled cc_binary will link all its dynamic_deps, even the ones it depends on
* transitively, linking indirect deps might be necessary because if the RPATHs haven't been set
* up properly in those dynamic_deps then the loader won't be able to find those libraries unless
* they are also linked. For a production binary this is probably not the desired behavior and you
* can switch it off by disabling this feature, for the binary to work you have to make sure that
* the RPATHs in all shared libraries are set up properly though. The default toolchains have this
* behavior switched off for cc_binaries by default. The behavior for cc_tests with dynamic_deps
* on all platforms and Windows cc_binaries is hardcoded to always link every transitive library.
*
* <p>This feature controls the behavior for shared libraries depended on via dynamic_deps and
* doesn't control the behavior of the dynamic dependencies created by cc_libraries and used for
* cc_tests or cc_binaries(linkstatic=0).
*/
public static final String LINK_INDIRECT_DYNAMIC_DEPS_IN_BINARY_FEATURE_NAME =
"link_indirect_dynamic_deps_in_binary";

/** Ancestor for all rules that do include scanning. */
public static final class CcIncludeScanningRule implements RuleDefinition {
private final boolean addGrepIncludes;
Expand Down
158 changes: 84 additions & 74 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
View file
Expand Up @@ -61,6 +61,7 @@ public class LibrariesToLinkCollector {
private final RuleErrorConsumer ruleErrorConsumer;
private final Artifact output;
private final String workspaceName;
private final Artifact dynamicLibrarySolibSymlinkOutput;

public LibrariesToLinkCollector(
boolean isNativeDeps,
Expand All @@ -79,7 +80,8 @@ public LibrariesToLinkCollector(
Iterable<LinkerInput> linkerInputs,
boolean needWholeArchive,
RuleErrorConsumer ruleErrorConsumer,
String workspaceName) {
String workspaceName,
Artifact dynamicLibrarySolibSymlinkOutput) {
this.isNativeDeps = isNativeDeps;
this.cppConfiguration = cppConfiguration;
this.ccToolchainProvider = toolchain;
Expand All @@ -95,6 +97,7 @@ public LibrariesToLinkCollector(
this.ruleErrorConsumer = ruleErrorConsumer;
this.output = output;
this.workspaceName = workspaceName;
this.dynamicLibrarySolibSymlinkOutput = dynamicLibrarySolibSymlinkOutput;

needToolchainLibrariesRpath =
toolchainLibrariesSolibDir != null
Expand Down Expand Up @@ -164,81 +167,88 @@ private NestedSet<String> collectToolchainRuntimeLibrarySearchDirectories(
}

private ImmutableList<String> 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<String> 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;
ImmutableList.Builder<Artifact> outputs = ImmutableList.builder();
outputs.add(output);
if (dynamicLibrarySolibSymlinkOutput != null) {
outputs.add(dynamicLibrarySolibSymlinkOutput);
}
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 + "/");
for (Artifact output : outputs.build()) {
// 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();
// 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();
Expand Down

0 comments on commit 044a14c

Please sign in to comment.