Skip to content

Commit 044a14c

Browse files
oquenchilcopybara-github
oquenchil
authored and
copybara-github
committed
Fix rpath for symlinks in cc_shared_library
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
1 parent dba9e43 commit 044a14c

File tree

11 files changed

+144
-171
lines changed

11 files changed

+144
-171
lines changed

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

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -694,7 +694,8 @@ private boolean createDynamicLinkAction(
694694
"-Wl,-soname="
695695
+ SolibSymlinkAction.getDynamicLibrarySoname(
696696
linkerOutput.getRootRelativePath(),
697-
/* preserveName= */ false,
697+
/* preserveName= */ dynamicLinkType
698+
!= LinkTargetType.NODEPS_DYNAMIC_LIBRARY,
698699
actionConstructionContext.getConfiguration().getMnemonic()));
699700
}
700701
}
@@ -799,6 +800,10 @@ private boolean createDynamicLinkAction(
799800
if (dynamicLinkActionBuilder.getAllLtoBackendArtifacts() != null) {
800801
ccLinkingOutputs.addAllLtoArtifacts(dynamicLinkActionBuilder.getAllLtoBackendArtifacts());
801802
}
803+
Artifact implLibraryLinkArtifact = getDynamicLibrarySolibSymlinkOutput(linkerOutput);
804+
if (implLibraryLinkArtifact != null) {
805+
dynamicLinkActionBuilder.setDynamicLibrarySolibSymlinkOutput(implLibraryLinkArtifact);
806+
}
802807
CppLinkAction dynamicLinkAction = dynamicLinkActionBuilder.build();
803808
if (dynamicLinkType.isExecutable()) {
804809
ccLinkingOutputs.setExecutable(linkerOutput);
@@ -824,14 +829,16 @@ private boolean createDynamicLinkAction(
824829
}
825830
libraryToLinkBuilder.setDynamicLibrary(dynamicLibrary.getArtifact());
826831
} else {
827-
Artifact implLibraryLinkArtifact =
828-
SolibSymlinkAction.getDynamicLibrarySymlink(
829-
/* actionRegistry= */ actionRegistry,
830-
/* actionConstructionContext= */ actionConstructionContext,
831-
ccToolchain.getSolibDirectory(),
832-
dynamicLibrary.getArtifact(),
833-
/* preserveName= */ false,
834-
/* prefixConsumer= */ false);
832+
if (dynamicLinkType == LinkTargetType.NODEPS_DYNAMIC_LIBRARY) {
833+
implLibraryLinkArtifact =
834+
SolibSymlinkAction.getDynamicLibrarySymlink(
835+
/* actionRegistry= */ actionRegistry,
836+
/* actionConstructionContext= */ actionConstructionContext,
837+
ccToolchain.getSolibDirectory(),
838+
dynamicLibrary.getArtifact(),
839+
/* preserveName= */ false,
840+
/* prefixConsumer= */ false);
841+
}
835842
libraryToLinkBuilder.setDynamicLibrary(implLibraryLinkArtifact);
836843
libraryToLinkBuilder.setResolvedSymlinkDynamicLibrary(dynamicLibrary.getArtifact());
837844

@@ -842,7 +849,8 @@ private boolean createDynamicLinkAction(
842849
/* actionConstructionContext= */ actionConstructionContext,
843850
ccToolchain.getSolibDirectory(),
844851
interfaceLibrary.getArtifact(),
845-
/* preserveName= */ false,
852+
// Need to preserve name for transitive shared libraries that may be distributed.
853+
/* preserveName= */ dynamicLinkType != LinkTargetType.NODEPS_DYNAMIC_LIBRARY,
846854
/* prefixConsumer= */ false);
847855
libraryToLinkBuilder.setInterfaceLibrary(libraryLinkArtifact);
848856
libraryToLinkBuilder.setResolvedSymlinkInterfaceLibrary(interfaceLibrary.getArtifact());
@@ -1001,4 +1009,24 @@ private static List<LinkerInputs.LibraryToLink> convertLibraryToLinkListToLinker
10011009
}
10021010
return librariesToLinkBuilder.build();
10031011
}
1012+
1013+
@Nullable
1014+
private Artifact getDynamicLibrarySolibSymlinkOutput(Artifact linkerOutputArtifact) {
1015+
if (dynamicLinkType != LinkTargetType.DYNAMIC_LIBRARY
1016+
|| neverlink
1017+
|| featureConfiguration.isEnabled(CppRuleClasses.COPY_DYNAMIC_LIBRARIES_TO_BINARY)) {
1018+
return null;
1019+
}
1020+
return SolibSymlinkAction.getDynamicLibrarySymlink(
1021+
/* actionRegistry= */ actionRegistry,
1022+
/* actionConstructionContext= */ actionConstructionContext,
1023+
ccToolchain.getSolibDirectory(),
1024+
linkerOutputArtifact,
1025+
// For transitive shared libraries we want to preserve the name of the original library so
1026+
// that distribution artifacts can be linked against it and not against the mangled name.
1027+
// This makes it possible to find the library on the system if the RPATH has been set
1028+
// properly.
1029+
/* preserveName= */ true,
1030+
/* prefixConsumer= */ false);
1031+
}
10041032
}

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

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,10 @@ public Artifact create(
147147
private boolean isStampingEnabled;
148148
private final Map<String, String> executionInfo = new LinkedHashMap<>();
149149

150+
// We have to add the dynamicLibrarySolibOutput to the CppLinkActionBuilder so that it knows how
151+
// to set up the RPATH properly with respect to the symlink itself and not the original library.
152+
private Artifact dynamicLibrarySolibSymlinkOutput;
153+
150154
/**
151155
* Creates a builder that builds {@link CppLinkAction}s.
152156
*
@@ -814,7 +818,8 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
814818
nonExpandedLinkerInputs,
815819
needWholeArchive,
816820
ruleErrorConsumer,
817-
((RuleContext) actionConstructionContext).getWorkspaceName());
821+
((RuleContext) actionConstructionContext).getWorkspaceName(),
822+
dynamicLibrarySolibSymlinkOutput);
818823
CollectedLibrariesToLink collectedLibrariesToLink =
819824
librariesToLinkCollector.collectLibrariesToLink();
820825

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

834-
NestedSet<String> runtimeLibrarySearchDirectories =
835-
collectedLibrariesToLink.getRuntimeLibrarySearchDirectories();
836-
if (linkType.getActionName().equals(CppActionNames.CPP_LINK_DYNAMIC_LIBRARY)
837-
&& featureConfiguration.isEnabled(
838-
CppRuleClasses.EXCLUDE_BAZEL_RPATHS_IN_TRANSITIVE_LIBS_FEATURE_NAME)) {
839-
runtimeLibrarySearchDirectories = null;
840-
}
841839
variables =
842840
LinkBuildVariables.setupVariables(
843841
((RuleContext) actionConstructionContext).getStarlarkThread(),
@@ -865,7 +863,7 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
865863
ltoOutputRootPrefix,
866864
defFile != null ? defFile.getExecPathString() : null,
867865
fdoContext,
868-
runtimeLibrarySearchDirectories,
866+
collectedLibrariesToLink.getRuntimeLibrarySearchDirectories(),
869867
collectedLibrariesToLink.getLibrariesToLink(),
870868
collectedLibrariesToLink.getLibrarySearchDirectories(),
871869
/* addIfsoRelatedVariables= */ true);
@@ -1563,4 +1561,11 @@ public CppLinkActionBuilder addExecutionInfo(Map<String, String> executionInfo)
15631561
this.executionInfo.putAll(executionInfo);
15641562
return this;
15651563
}
1564+
1565+
@CanIgnoreReturnValue
1566+
public CppLinkActionBuilder setDynamicLibrarySolibSymlinkOutput(
1567+
Artifact dynamicLibrarySolibSymlinkOutput) {
1568+
this.dynamicLibrarySolibSymlinkOutput = dynamicLibrarySolibSymlinkOutput;
1569+
return this;
1570+
}
15661571
}

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

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -517,37 +517,6 @@ public static ToolchainTypeRequirement ccToolchainTypeRequirement(RuleDefinition
517517
*/
518518
public static final String LEGACY_IS_CC_TEST_FEATURE_NAME = "legacy_is_cc_test";
519519

520-
/**
521-
* By default Bazel will be embed runtime search directories (RPATHS) in transitive shared
522-
* libraries, however, for Linux they are wrong in most cases since the runfiles directory where
523-
* the transitive shared library will live will not be known at the time of linking. The runfiles
524-
* directory is decided by dependent rules, we don't know where those dependents will live. For
525-
* Mac (where it works differently by searching from the library's path instead of the main
526-
* binary's) the loader paths (not rpaths) are correct so the paths will work.
527-
*
528-
* <p>This feature controls whether Bazel will embed those rpaths into the transitive shared
529-
* library.
530-
*/
531-
public static final String EXCLUDE_BAZEL_RPATHS_IN_TRANSITIVE_LIBS_FEATURE_NAME =
532-
"exclude_bazel_rpaths_in_transitive_libs";
533-
534-
/**
535-
* With this feature enabled cc_binary will link all its dynamic_deps, even the ones it depends on
536-
* transitively, linking indirect deps might be necessary because if the RPATHs haven't been set
537-
* up properly in those dynamic_deps then the loader won't be able to find those libraries unless
538-
* they are also linked. For a production binary this is probably not the desired behavior and you
539-
* can switch it off by disabling this feature, for the binary to work you have to make sure that
540-
* the RPATHs in all shared libraries are set up properly though. The default toolchains have this
541-
* behavior switched off for cc_binaries by default. The behavior for cc_tests with dynamic_deps
542-
* on all platforms and Windows cc_binaries is hardcoded to always link every transitive library.
543-
*
544-
* <p>This feature controls the behavior for shared libraries depended on via dynamic_deps and
545-
* doesn't control the behavior of the dynamic dependencies created by cc_libraries and used for
546-
* cc_tests or cc_binaries(linkstatic=0).
547-
*/
548-
public static final String LINK_INDIRECT_DYNAMIC_DEPS_IN_BINARY_FEATURE_NAME =
549-
"link_indirect_dynamic_deps_in_binary";
550-
551520
/** Ancestor for all rules that do include scanning. */
552521
public static final class CcIncludeScanningRule implements RuleDefinition {
553522
private final boolean addGrepIncludes;

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

Lines changed: 84 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ public class LibrariesToLinkCollector {
6161
private final RuleErrorConsumer ruleErrorConsumer;
6262
private final Artifact output;
6363
private final String workspaceName;
64+
private final Artifact dynamicLibrarySolibSymlinkOutput;
6465

6566
public LibrariesToLinkCollector(
6667
boolean isNativeDeps,
@@ -79,7 +80,8 @@ public LibrariesToLinkCollector(
7980
Iterable<LinkerInput> linkerInputs,
8081
boolean needWholeArchive,
8182
RuleErrorConsumer ruleErrorConsumer,
82-
String workspaceName) {
83+
String workspaceName,
84+
Artifact dynamicLibrarySolibSymlinkOutput) {
8385
this.isNativeDeps = isNativeDeps;
8486
this.cppConfiguration = cppConfiguration;
8587
this.ccToolchainProvider = toolchain;
@@ -95,6 +97,7 @@ public LibrariesToLinkCollector(
9597
this.ruleErrorConsumer = ruleErrorConsumer;
9698
this.output = output;
9799
this.workspaceName = workspaceName;
100+
this.dynamicLibrarySolibSymlinkOutput = dynamicLibrarySolibSymlinkOutput;
98101

99102
needToolchainLibrariesRpath =
100103
toolchainLibrariesSolibDir != null
@@ -164,81 +167,88 @@ private NestedSet<String> collectToolchainRuntimeLibrarySearchDirectories(
164167
}
165168

166169
private ImmutableList<String> findPotentialSolibParents() {
167-
// The runtime location of the solib directory relative to the binary depends on four factors:
168-
//
169-
// * whether the binary is contained in the main repository or an external repository;
170-
// * whether the binary is executed directly or from a runfiles tree;
171-
// * whether the binary is staged as a symlink (sandboxed execution; local execution if the
172-
// binary is in the runfiles of another target) or a regular file (remote execution) - the
173-
// dynamic linker follows sandbox and runfiles symlinks into its location under the
174-
// unsandboxed execroot, which thus becomes the effective $ORIGIN;
175-
// * whether --experimental_sibling_repository_layout is enabled or not.
176-
//
177-
// The rpaths emitted into the binary thus have to cover the following cases (assuming that
178-
// the binary target is located in the pkg `pkg` and has name `file`) for the directory used
179-
// as $ORIGIN by the dynamic linker and the directory containing the solib directories:
180-
//
181-
// 1. main, direct, symlink:
182-
// $ORIGIN: $EXECROOT/pkg
183-
// solib root: $EXECROOT
184-
// 2. main, direct, regular file:
185-
// $ORIGIN: $EXECROOT/pkg
186-
// solib root: $EXECROOT/pkg/file.runfiles/main_repo
187-
// 3. main, runfiles, symlink:
188-
// $ORIGIN: $EXECROOT/pkg
189-
// solib root: $EXECROOT
190-
// 4. main, runfiles, regular file:
191-
// $ORIGIN: other_target.runfiles/main_repo/pkg
192-
// solib root: other_target.runfiles/main_repo
193-
// 5a. external, direct, symlink:
194-
// $ORIGIN: $EXECROOT/external/other_repo/pkg
195-
// solib root: $EXECROOT
196-
// 5b. external, direct, symlink, with --experimental_sibling_repository_layout:
197-
// $ORIGIN: $EXECROOT/../other_repo/pkg
198-
// solib root: $EXECROOT/../other_repo
199-
// 6a. external, direct, regular file:
200-
// $ORIGIN: $EXECROOT/external/other_repo/pkg
201-
// solib root: $EXECROOT/external/other_repo/pkg/file.runfiles/main_repo
202-
// 6b. external, direct, regular file, with --experimental_sibling_repository_layout:
203-
// $ORIGIN: $EXECROOT/../other_repo/pkg
204-
// solib root: $EXECROOT/../other_repo/pkg/file.runfiles/other_repo
205-
// 7a. external, runfiles, symlink:
206-
// $ORIGIN: $EXECROOT/external/other_repo/pkg
207-
// solib root: $EXECROOT
208-
// 7b. external, runfiles, symlink, with --experimental_sibling_repository_layout:
209-
// $ORIGIN: $EXECROOT/../other_repo/pkg
210-
// solib root: $EXECROOT/../other_repo
211-
// 8a. external, runfiles, regular file:
212-
// $ORIGIN: other_target.runfiles/some_repo/pkg
213-
// solib root: other_target.runfiles/main_repo
214-
// 8b. external, runfiles, regular file, with --experimental_sibling_repository_layout:
215-
// $ORIGIN: other_target.runfiles/some_repo/pkg
216-
// solib root: other_target.runfiles/some_repo
217-
//
218-
// Cases 1, 3, 4, 5, 7, and 8b are covered by an rpath that walks up the root relative path.
219-
// Cases 2 and 6 covered by walking into file.runfiles/main_repo.
220-
// Case 8a is covered by walking up some_repo/pkg and then into main_repo.
221-
boolean isExternal =
222-
output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX);
223-
boolean usesLegacyRepositoryLayout = output.getRoot().isLegacy();
224170
ImmutableList.Builder<String> solibParents = ImmutableList.builder();
225-
// Handles cases 1, 3, 4, 5, and 7.
226-
solibParents.add("../".repeat(output.getRootRelativePath().segmentCount() - 1));
227-
// Handle cases 2 and 6.
228-
String solibRepositoryName;
229-
if (isExternal && !usesLegacyRepositoryLayout) {
230-
// Case 6b
231-
solibRepositoryName = output.getRunfilesPath().getSegment(1);
232-
} else {
233-
// Cases 2 and 6a
234-
solibRepositoryName = workspaceName;
171+
ImmutableList.Builder<Artifact> outputs = ImmutableList.builder();
172+
outputs.add(output);
173+
if (dynamicLibrarySolibSymlinkOutput != null) {
174+
outputs.add(dynamicLibrarySolibSymlinkOutput);
235175
}
236-
solibParents.add(output.getFilename() + ".runfiles/" + solibRepositoryName + "/");
237-
if (isExternal && usesLegacyRepositoryLayout) {
238-
// Handles case 8a. The runfiles path is of the form ../some_repo/pkg/file and we need to
239-
// walk up some_repo/pkg and then down into main_repo.
240-
solibParents.add(
241-
"../".repeat(output.getRunfilesPath().segmentCount() - 2) + workspaceName + "/");
176+
for (Artifact output : outputs.build()) {
177+
// The runtime location of the solib directory relative to the binary depends on four factors:
178+
//
179+
// * whether the binary is contained in the main repository or an external repository;
180+
// * whether the binary is executed directly or from a runfiles tree;
181+
// * whether the binary is staged as a symlink (sandboxed execution; local execution if the
182+
// binary is in the runfiles of another target) or a regular file (remote execution) - the
183+
// dynamic linker follows sandbox and runfiles symlinks into its location under the
184+
// unsandboxed execroot, which thus becomes the effective $ORIGIN;
185+
// * whether --experimental_sibling_repository_layout is enabled or not.
186+
//
187+
// The rpaths emitted into the binary thus have to cover the following cases (assuming that
188+
// the binary target is located in the pkg `pkg` and has name `file`) for the directory used
189+
// as $ORIGIN by the dynamic linker and the directory containing the solib directories:
190+
//
191+
// 1. main, direct, symlink:
192+
// $ORIGIN: $EXECROOT/pkg
193+
// solib root: $EXECROOT
194+
// 2. main, direct, regular file:
195+
// $ORIGIN: $EXECROOT/pkg
196+
// solib root: $EXECROOT/pkg/file.runfiles/main_repo
197+
// 3. main, runfiles, symlink:
198+
// $ORIGIN: $EXECROOT/pkg
199+
// solib root: $EXECROOT
200+
// 4. main, runfiles, regular file:
201+
// $ORIGIN: other_target.runfiles/main_repo/pkg
202+
// solib root: other_target.runfiles/main_repo
203+
// 5a. external, direct, symlink:
204+
// $ORIGIN: $EXECROOT/external/other_repo/pkg
205+
// solib root: $EXECROOT
206+
// 5b. external, direct, symlink, with --experimental_sibling_repository_layout:
207+
// $ORIGIN: $EXECROOT/../other_repo/pkg
208+
// solib root: $EXECROOT/../other_repo
209+
// 6a. external, direct, regular file:
210+
// $ORIGIN: $EXECROOT/external/other_repo/pkg
211+
// solib root: $EXECROOT/external/other_repo/pkg/file.runfiles/main_repo
212+
// 6b. external, direct, regular file, with --experimental_sibling_repository_layout:
213+
// $ORIGIN: $EXECROOT/../other_repo/pkg
214+
// solib root: $EXECROOT/../other_repo/pkg/file.runfiles/other_repo
215+
// 7a. external, runfiles, symlink:
216+
// $ORIGIN: $EXECROOT/external/other_repo/pkg
217+
// solib root: $EXECROOT
218+
// 7b. external, runfiles, symlink, with --experimental_sibling_repository_layout:
219+
// $ORIGIN: $EXECROOT/../other_repo/pkg
220+
// solib root: $EXECROOT/../other_repo
221+
// 8a. external, runfiles, regular file:
222+
// $ORIGIN: other_target.runfiles/some_repo/pkg
223+
// solib root: other_target.runfiles/main_repo
224+
// 8b. external, runfiles, regular file, with --experimental_sibling_repository_layout:
225+
// $ORIGIN: other_target.runfiles/some_repo/pkg
226+
// solib root: other_target.runfiles/some_repo
227+
//
228+
// Cases 1, 3, 4, 5, 7, and 8b are covered by an rpath that walks up the root relative path.
229+
// Cases 2 and 6 covered by walking into file.runfiles/main_repo.
230+
// Case 8a is covered by walking up some_repo/pkg and then into main_repo.
231+
boolean isExternal =
232+
output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX);
233+
boolean usesLegacyRepositoryLayout = output.getRoot().isLegacy();
234+
// Handles cases 1, 3, 4, 5, and 7.
235+
solibParents.add("../".repeat(output.getRootRelativePath().segmentCount() - 1));
236+
// Handle cases 2 and 6.
237+
String solibRepositoryName;
238+
if (isExternal && !usesLegacyRepositoryLayout) {
239+
// Case 6b
240+
solibRepositoryName = output.getRunfilesPath().getSegment(1);
241+
} else {
242+
// Cases 2 and 6a
243+
solibRepositoryName = workspaceName;
244+
}
245+
solibParents.add(output.getFilename() + ".runfiles/" + solibRepositoryName + "/");
246+
if (isExternal && usesLegacyRepositoryLayout) {
247+
// Handles case 8a. The runfiles path is of the form ../some_repo/pkg/file and we need to
248+
// walk up some_repo/pkg and then down into main_repo.
249+
solibParents.add(
250+
"../".repeat(output.getRunfilesPath().segmentCount() - 2) + workspaceName + "/");
251+
}
242252
}
243253

244254
return solibParents.build();

0 commit comments

Comments
 (0)