Skip to content

Commit e3dcfa5

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Roll forward of c7aa392:
Fix dynamic library lookup with remotely executed tools When a cc_binary tool is executed directly (e.g. in a genrule) with remote execution, its dynamic dependencies are only available from the solib directory in the runfiles directory. This commit adds an additional rpath entry for this directory. Since target names may contain commas and the newly added rpaths contain target names, the `-Xlinker` compiler is now used everywhere instead of the `-Wl` flag, which doesn't support commas in linker arguments. Stacked on #16214 Closes #16215. PiperOrigin-RevId: 481621970 Change-Id: I5969a15ef0c828477f893216b6b702a3bad6b6fa
1 parent a624e21 commit e3dcfa5

File tree

11 files changed

+217
-30
lines changed

11 files changed

+217
-30
lines changed

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

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -549,18 +549,25 @@ public static ImmutableList<CToolchain.Feature> getLegacyFeatures(
549549
" flag_group {",
550550
" expand_if_true: 'is_cc_test'",
551551
// TODO(b/27153401): This should probably be @loader_path on osx.
552-
" flag: ",
553-
" '-Wl,-rpath,$EXEC_ORIGIN/%{runtime_library_search_directories}'",
552+
" flag: '-Xlinker'",
553+
" flag: '-rpath'",
554+
" flag: '-Xlinker'",
555+
" flag: '$EXEC_ORIGIN/%{runtime_library_search_directories}'",
554556
" }",
555557
" flag_group {",
556558
" expand_if_false: 'is_cc_test'",
557559
ifLinux(
558560
platform,
559-
" flag: '-Wl,-rpath,$ORIGIN/"
560-
+ "%{runtime_library_search_directories}'"),
561+
" flag: '-Xlinker'",
562+
" flag: '-rpath'",
563+
" flag: '-Xlinker'",
564+
" flag: '$ORIGIN/" + "%{runtime_library_search_directories}'"),
561565
ifMac(
562566
platform,
563-
" flag: '-Wl,-rpath,@loader_path/"
567+
" flag: '-Xlinker'",
568+
" flag: '-rpath'",
569+
" flag: '-Xlinker'",
570+
" flag: '@loader_path/"
564571
+ "%{runtime_library_search_directories}'"),
565572
" }",
566573
" }",
@@ -579,11 +586,16 @@ public static ImmutableList<CToolchain.Feature> getLegacyFeatures(
579586
" flag_group {",
580587
ifLinux(
581588
platform,
582-
" flag: '-Wl,-rpath,$ORIGIN/"
583-
+ "%{runtime_library_search_directories}'"),
589+
" flag: '-Xlinker'",
590+
" flag: '-rpath'",
591+
" flag: '-Xlinker'",
592+
" flag: '$ORIGIN/" + "%{runtime_library_search_directories}'"),
584593
ifMac(
585594
platform,
586-
" flag: '-Wl,-rpath,@loader_path/"
595+
" flag: '-Xlinker'",
596+
" flag: '-rpath'",
597+
" flag: '-Xlinker'",
598+
" flag: '@loader_path/"
587599
+ "%{runtime_library_search_directories}'"),
588600
" }",
589601
" }",

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

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -162,16 +162,25 @@ public LibrariesToLinkCollector(
162162
// solib root: other_target.runfiles/some_repo
163163
//
164164
// Cases 1, 3, 4, 5, 7, and 8b are covered by an rpath that walks up the root relative path.
165+
// Cases 2 and 6 covered by walking into file.runfiles/main_repo.
165166
// Case 8a is covered by walking up some_repo/pkg and then into main_repo.
166-
// Cases 2 and 6 are currently not covered as they would require an rpath containing the
167-
// binary filename, which may contain commas that would clash with the `-Wl` argument used to
168-
// pass the rpath to the linker.
169-
// TODO(#14600): Fix this by using `-Xlinker` instead of `-Wl`.
167+
boolean isExternal =
168+
output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX);
169+
boolean usesLegacyRepositoryLayout = output.getRoot().isLegacy();
170170
ImmutableList.Builder<String> execRoots = ImmutableList.builder();
171171
// Handles cases 1, 3, 4, 5, and 7.
172172
execRoots.add("../".repeat(output.getRootRelativePath().segmentCount() - 1));
173-
if (output.getRunfilesPath().startsWith(LabelConstants.EXTERNAL_RUNFILES_PATH_PREFIX)
174-
&& output.getRoot().isLegacy()) {
173+
// Handle cases 2 and 6.
174+
String solibRepositoryName;
175+
if (isExternal && !usesLegacyRepositoryLayout) {
176+
// Case 6b
177+
solibRepositoryName = output.getRunfilesPath().getSegment(1);
178+
} else {
179+
// Cases 2 and 6a
180+
solibRepositoryName = workspaceName;
181+
}
182+
execRoots.add(output.getFilename() + ".runfiles/" + solibRepositoryName + "/");
183+
if (isExternal && usesLegacyRepositoryLayout) {
175184
// Handles case 8a. The runfiles path is of the form ../some_repo/pkg/file and we need to
176185
// walk up some_repo/pkg and then down into main_repo.
177186
execRoots.add(

‎src/test/java/com/google/devtools/build/lib/packages/util/mock/osx_cc_toolchain_config.bzl

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7922,7 +7922,10 @@ def _impl(ctx):
79227922
flag_groups = [
79237923
flag_group(
79247924
flags = [
7925-
"-Wl,-rpath,$ORIGIN/%{runtime_library_search_directories}",
7925+
"-Xlinker",
7926+
"-rpath",
7927+
"-Xlinker",
7928+
"@loader_path/%{runtime_library_search_directories}",
79267929
],
79277930
iterate_over = "runtime_library_search_directories",
79287931
expand_if_available = "runtime_library_search_directories",

‎src/test/java/com/google/devtools/build/lib/rules/cpp/CcImportBaseConfiguredTargetTest.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -444,7 +444,9 @@ public void testCcImportWithSharedLibraryAddsRpathEntry() throws Exception {
444444
Artifact mainBin = getBinArtifact("bin", main);
445445
CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin);
446446
List<String> linkArgv = action.getLinkCommandLine().arguments();
447-
assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua");
447+
assertThat(linkArgv)
448+
.containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua")
449+
.inOrder();
448450
}
449451

450452
@Test
@@ -525,6 +527,8 @@ public void testCcImportWithSharedLibraryWithTransitionAddsRpathEntry() throws E
525527
Artifact mainBin = getBinArtifact("bin", main);
526528
CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin);
527529
List<String> linkArgv = action.getLinkCommandLine().arguments();
528-
assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua");
530+
assertThat(linkArgv)
531+
.containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/_U_S_Sa_Cfoo___Ua")
532+
.inOrder();
529533
}
530534
}

‎src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2124,7 +2124,7 @@ public void testRpathIsNotAddedWhenThereAreNoSoDeps() throws Exception {
21242124
Artifact mainBin = getBinArtifact("main", main);
21252125
CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin);
21262126
assertThat(Joiner.on(" ").join(action.getLinkCommandLine().arguments()))
2127-
.doesNotContain("-Wl,-rpath");
2127+
.doesNotContain("-Xlinker -rpath");
21282128
}
21292129

21302130
@Test
@@ -2157,12 +2157,21 @@ public void testRpathAndLinkPathsWithoutTransitions() throws Exception {
21572157
Artifact mainBin = getBinArtifact("main", main);
21582158
CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin);
21592159
List<String> linkArgv = action.getLinkCommandLine().arguments();
2160-
assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/");
2160+
assertThat(linkArgv)
2161+
.containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/")
2162+
.inOrder();
2163+
assertThat(linkArgv)
2164+
.containsAtLeast(
2165+
"-Xlinker",
2166+
"-rpath",
2167+
"-Xlinker",
2168+
"$ORIGIN/main.runfiles/" + TestConstants.WORKSPACE_NAME + "/_solib_k8/")
2169+
.inOrder();
21612170
assertThat(linkArgv)
21622171
.contains("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild/bin/_solib_k8");
21632172
assertThat(linkArgv).contains("-lno-transition_Slibdep1");
21642173
assertThat(Joiner.on(" ").join(linkArgv))
2165-
.doesNotContain("-Wl,-rpath,$ORIGIN/../_solib_k8/../../../k8-fastbuild-ST-");
2174+
.doesNotContain("-Xlinker -rpath -Xlinker $ORIGIN/../_solib_k8/../../../k8-fastbuild-ST-");
21662175
assertThat(Joiner.on(" ").join(linkArgv))
21672176
.doesNotContain("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild-ST-");
21682177
assertThat(Joiner.on(" ").join(linkArgv)).doesNotContain("-lST-");
@@ -2217,9 +2226,18 @@ public void testRpathRootIsAddedEvenWithTransitionedDepsOnly() throws Exception
22172226
Artifact mainBin = getBinArtifact("main", main);
22182227
CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin);
22192228
List<String> linkArgv = action.getLinkCommandLine().arguments();
2220-
assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/");
2229+
assertThat(linkArgv)
2230+
.containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/")
2231+
.inOrder();
2232+
assertThat(linkArgv)
2233+
.containsAtLeast(
2234+
"-Xlinker",
2235+
"-rpath",
2236+
"-Xlinker",
2237+
"$ORIGIN/main.runfiles/" + TestConstants.WORKSPACE_NAME + "/_solib_k8/")
2238+
.inOrder();
22212239
assertThat(Joiner.on(" ").join(linkArgv))
2222-
.contains("-Wl,-rpath,$ORIGIN/../../../k8-fastbuild-ST-");
2240+
.contains("-Xlinker -rpath -Xlinker $ORIGIN/../../../k8-fastbuild-ST-");
22232241
assertThat(Joiner.on(" ").join(linkArgv))
22242242
.contains("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild-ST-");
22252243
assertThat(Joiner.on(" ").join(linkArgv)).containsMatch("-lST-[0-9a-f]+_transition_Slibdep2");

‎src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,9 @@ public void testLibOptsAndLibSrcsAreInCorrectOrder() throws Exception {
260260
".* -L[^ ]*some-dir(?= ).* -L[^ ]*some-other-dir(?= ).* "
261261
+ "-lbar -l:qux.so(?= ).* -ldl -lutil .*");
262262
assertThat(Joiner.on(" ").join(arguments))
263-
.matches(".* -Wl,-rpath[^ ]*some-dir(?= ).* -Wl,-rpath[^ ]*some-other-dir .*");
263+
.matches(
264+
".* -Xlinker -rpath -Xlinker [^ ]*some-dir(?= ).* -Xlinker -rpath -Xlinker [^"
265+
+ " ]*some-other-dir .*");
264266
}
265267

266268
@Test

‎src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -521,7 +521,8 @@ protected static void addAppleBinaryStarlarkRule(Scratch scratch) throws Excepti
521521
" if binary_type == 'dylib':",
522522
" linkopts.append('-dynamiclib')",
523523
" elif binary_type == 'loadable_bundle':",
524-
" linkopts.extend(['-bundle', '-Wl,-rpath,@loader_path/Frameworks'])",
524+
" linkopts.extend(['-bundle', '-Xlinker', '-rpath', '-Xlinker',"
525+
+ " '@loader_path/Frameworks'])",
525526
" if ctx.attr.bundle_loader:",
526527
" bundle_loader = ctx.attr.bundle_loader",
527528
" bundle_loader_file = bundle_loader[apple_common.AppleExecutableBinary].binary",
@@ -782,7 +783,7 @@ protected void checkBundleLoaderIsCorrectlyPassedToTheLinker(RuleType ruleType)
782783
assertThat(Joiner.on(" ").join(linkAction.getArguments()))
783784
.contains("-bundle_loader " + getBinArtifact("bin_lipobin", binTarget).getExecPath());
784785
assertThat(Joiner.on(" ").join(linkAction.getArguments()))
785-
.contains("-Wl,-rpath,@loader_path/Frameworks");
786+
.contains("-Xlinker -rpath -Xlinker @loader_path/Frameworks");
786787
}
787788

788789
protected Action lipoLibAction(String libLabel) throws Exception {

‎src/test/shell/bazel/remote/remote_execution_test.sh

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3062,4 +3062,130 @@ function test_unresolved_symlink_spawn_absolute() {
30623062
do_test_unresolved_symlink spawn /non/existent
30633063
}
30643064

3065+
function setup_cc_binary_tool_with_dynamic_deps() {
3066+
local repo=$1
3067+
3068+
cat >> WORKSPACE <<'EOF'
3069+
local_repository(
3070+
name = "other_repo",
3071+
path = "other_repo",
3072+
)
3073+
EOF
3074+
3075+
mkdir -p $repo
3076+
touch $repo/WORKSPACE
3077+
3078+
mkdir -p $repo/lib
3079+
# Use a comma in the target name as that is known to be problematic whith -Wl,
3080+
# which is commonly used to pass rpaths to the linker.
3081+
cat > $repo/lib/BUILD <<'EOF'
3082+
cc_binary(
3083+
name = "l,ib",
3084+
srcs = ["lib.cpp"],
3085+
linkshared = True,
3086+
linkstatic = True,
3087+
)
3088+
3089+
cc_import(
3090+
name = "dynamic_l,ib",
3091+
shared_library = ":l,ib",
3092+
hdrs = ["lib.h"],
3093+
visibility = ["//visibility:public"],
3094+
)
3095+
EOF
3096+
cat > $repo/lib/lib.h <<'EOF'
3097+
void print_greeting();
3098+
EOF
3099+
cat > $repo/lib/lib.cpp <<'EOF'
3100+
#include <cstdio>
3101+
void print_greeting() {
3102+
printf("Hello, world!\n");
3103+
}
3104+
EOF
3105+
3106+
mkdir -p $repo/pkg
3107+
cat > $repo/pkg/BUILD <<'EOF'
3108+
cc_binary(
3109+
name = "tool",
3110+
srcs = ["tool.cpp"],
3111+
deps = ["//lib:dynamic_l,ib"],
3112+
)
3113+
3114+
genrule(
3115+
name = "rule",
3116+
outs = ["out"],
3117+
cmd = "$(location :tool) > $@",
3118+
tools = [":tool"],
3119+
)
3120+
EOF
3121+
cat > $repo/pkg/tool.cpp <<'EOF'
3122+
#include "lib/lib.h"
3123+
int main() {
3124+
print_greeting();
3125+
}
3126+
EOF
3127+
}
3128+
3129+
function test_cc_binary_tool_with_dynamic_deps() {
3130+
if [[ "$PLATFORM" == "darwin" ]]; then
3131+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3132+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3133+
# action executors in order to select the appropriate Xcode toolchain.
3134+
return 0
3135+
fi
3136+
3137+
setup_cc_binary_tool_with_dynamic_deps .
3138+
3139+
bazel build \
3140+
--remote_executor=grpc://localhost:${worker_port} \
3141+
//pkg:rule >& $TEST_log || fail "Build should succeed"
3142+
}
3143+
3144+
function test_cc_binary_tool_with_dynamic_deps_sibling_repository_layout() {
3145+
if [[ "$PLATFORM" == "darwin" ]]; then
3146+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3147+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3148+
# action executors in order to select the appropriate Xcode toolchain.
3149+
return 0
3150+
fi
3151+
3152+
setup_cc_binary_tool_with_dynamic_deps .
3153+
3154+
bazel build \
3155+
--experimental_sibling_repository_layout \
3156+
--remote_executor=grpc://localhost:${worker_port} \
3157+
//pkg:rule >& $TEST_log || fail "Build should succeed"
3158+
}
3159+
3160+
function test_external_cc_binary_tool_with_dynamic_deps() {
3161+
if [[ "$PLATFORM" == "darwin" ]]; then
3162+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3163+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3164+
# action executors in order to select the appropriate Xcode toolchain.
3165+
return 0
3166+
fi
3167+
3168+
setup_cc_binary_tool_with_dynamic_deps other_repo
3169+
3170+
bazel build \
3171+
--remote_executor=grpc://localhost:${worker_port} \
3172+
@other_repo//pkg:rule >& $TEST_log || fail "Build should succeed"
3173+
}
3174+
3175+
function test_external_cc_binary_tool_with_dynamic_deps_sibling_repository_layout() {
3176+
if [[ "$PLATFORM" == "darwin" ]]; then
3177+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3178+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3179+
# action executors in order to select the appropriate Xcode toolchain.
3180+
return 0
3181+
fi
3182+
3183+
setup_cc_binary_tool_with_dynamic_deps other_repo
3184+
3185+
bazel build \
3186+
--experimental_sibling_repository_layout \
3187+
--remote_executor=grpc://localhost:${worker_port} \
3188+
@other_repo//pkg:rule >& $TEST_log || fail "Build should succeed"
3189+
}
3190+
30653191
run_suite "Remote execution and remote cache tests"

‎tools/cpp/osx_cc_wrapper.sh.tpl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ function parse_option() {
4242
LIBS="${BASH_REMATCH[1]} $LIBS"
4343
elif [[ "$opt" =~ ^-L(.*)$ ]]; then
4444
LIB_DIRS="${BASH_REMATCH[1]} $LIB_DIRS"
45-
elif [[ "$opt" =~ ^-Wl,-rpath,\@loader_path/(.*)$ ]]; then
45+
elif [[ "$opt" =~ ^\@loader_path/(.*)$ ]]; then
4646
RPATHS="${BASH_REMATCH[1]} ${RPATHS}"
4747
elif [[ "$opt" = "-o" ]]; then
4848
# output is coming

‎tools/cpp/unix_cc_toolchain_config.bzl

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -486,13 +486,19 @@ def _impl(ctx):
486486
flag_groups = [
487487
flag_group(
488488
flags = [
489-
"-Wl,-rpath,$EXEC_ORIGIN/%{runtime_library_search_directories}",
489+
"-Xlinker",
490+
"-rpath",
491+
"-Xlinker",
492+
"$EXEC_ORIGIN/%{runtime_library_search_directories}",
490493
],
491494
expand_if_true = "is_cc_test",
492495
),
493496
flag_group(
494497
flags = [
495-
"-Wl,-rpath,$ORIGIN/%{runtime_library_search_directories}",
498+
"-Xlinker",
499+
"-rpath",
500+
"-Xlinker",
501+
"$ORIGIN/%{runtime_library_search_directories}",
496502
],
497503
expand_if_false = "is_cc_test",
498504
),
@@ -513,7 +519,10 @@ def _impl(ctx):
513519
flag_groups = [
514520
flag_group(
515521
flags = [
516-
"-Wl,-rpath,$ORIGIN/%{runtime_library_search_directories}",
522+
"-Xlinker",
523+
"-rpath",
524+
"-Xlinker",
525+
"$ORIGIN/%{runtime_library_search_directories}",
517526
],
518527
),
519528
],

‎tools/osx/crosstool/cc_toolchain_config.bzl

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -777,7 +777,10 @@ def _impl(ctx):
777777
flag_groups = [
778778
flag_group(
779779
flags = [
780-
"-Wl,-rpath,@loader_path/%{runtime_library_search_directories}",
780+
"-Xlinker",
781+
"-rpath",
782+
"-Xlinker",
783+
"@loader_path/%{runtime_library_search_directories}",
781784
],
782785
iterate_over = "runtime_library_search_directories",
783786
expand_if_available = "runtime_library_search_directories",

0 commit comments

Comments
 (0)