Skip to content

Commit c7aa392

Browse files
oquenchilcopybara-github
oquenchil
authored and
copybara-github
committed
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 bazelbuild#16214 Closes bazelbuild#16215. PiperOrigin-RevId: 474792702 Change-Id: I2bfa1fd77be83d7bfe54ba591af5cb0ad0e630fc
1 parent d834905 commit c7aa392

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
@@ -2122,7 +2122,7 @@ public void testRpathIsNotAddedWhenThereAreNoSoDeps() throws Exception {
21222122
Artifact mainBin = getBinArtifact("main", main);
21232123
CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin);
21242124
assertThat(Joiner.on(" ").join(action.getLinkCommandLine().arguments()))
2125-
.doesNotContain("-Wl,-rpath");
2125+
.doesNotContain("-Xlinker -rpath");
21262126
}
21272127

21282128
@Test
@@ -2155,12 +2155,21 @@ public void testRpathAndLinkPathsWithoutTransitions() throws Exception {
21552155
Artifact mainBin = getBinArtifact("main", main);
21562156
CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin);
21572157
List<String> linkArgv = action.getLinkCommandLine().arguments();
2158-
assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/");
2158+
assertThat(linkArgv)
2159+
.containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/")
2160+
.inOrder();
2161+
assertThat(linkArgv)
2162+
.containsAtLeast(
2163+
"-Xlinker",
2164+
"-rpath",
2165+
"-Xlinker",
2166+
"$ORIGIN/main.runfiles/" + TestConstants.WORKSPACE_NAME + "/_solib_k8/")
2167+
.inOrder();
21592168
assertThat(linkArgv)
21602169
.contains("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild/bin/_solib_k8");
21612170
assertThat(linkArgv).contains("-lno-transition_Slibdep1");
21622171
assertThat(Joiner.on(" ").join(linkArgv))
2163-
.doesNotContain("-Wl,-rpath,$ORIGIN/../_solib_k8/../../../k8-fastbuild-ST-");
2172+
.doesNotContain("-Xlinker -rpath -Xlinker $ORIGIN/../_solib_k8/../../../k8-fastbuild-ST-");
21642173
assertThat(Joiner.on(" ").join(linkArgv))
21652174
.doesNotContain("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild-ST-");
21662175
assertThat(Joiner.on(" ").join(linkArgv)).doesNotContain("-lST-");
@@ -2215,9 +2224,18 @@ public void testRpathRootIsAddedEvenWithTransitionedDepsOnly() throws Exception
22152224
Artifact mainBin = getBinArtifact("main", main);
22162225
CppLinkAction action = (CppLinkAction) getGeneratingAction(mainBin);
22172226
List<String> linkArgv = action.getLinkCommandLine().arguments();
2218-
assertThat(linkArgv).contains("-Wl,-rpath,$ORIGIN/../_solib_k8/");
2227+
assertThat(linkArgv)
2228+
.containsAtLeast("-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../_solib_k8/")
2229+
.inOrder();
2230+
assertThat(linkArgv)
2231+
.containsAtLeast(
2232+
"-Xlinker",
2233+
"-rpath",
2234+
"-Xlinker",
2235+
"$ORIGIN/main.runfiles/" + TestConstants.WORKSPACE_NAME + "/_solib_k8/")
2236+
.inOrder();
22192237
assertThat(Joiner.on(" ").join(linkArgv))
2220-
.contains("-Wl,-rpath,$ORIGIN/../../../k8-fastbuild-ST-");
2238+
.contains("-Xlinker -rpath -Xlinker $ORIGIN/../../../k8-fastbuild-ST-");
22212239
assertThat(Joiner.on(" ").join(linkArgv))
22222240
.contains("-L" + TestConstants.PRODUCT_NAME + "-out/k8-fastbuild-ST-");
22232241
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
@@ -3037,4 +3037,130 @@ EOF
30373037
[[ $(cat bazel-bin/pkg/b.txt) == non/existent ]] || fail "expected symlink target to be non/existent"
30383038
}
30393039

3040+
function setup_cc_binary_tool_with_dynamic_deps() {
3041+
local repo=$1
3042+
3043+
cat >> WORKSPACE <<'EOF'
3044+
local_repository(
3045+
name = "other_repo",
3046+
path = "other_repo",
3047+
)
3048+
EOF
3049+
3050+
mkdir -p $repo
3051+
touch $repo/WORKSPACE
3052+
3053+
mkdir -p $repo/lib
3054+
# Use a comma in the target name as that is known to be problematic whith -Wl,
3055+
# which is commonly used to pass rpaths to the linker.
3056+
cat > $repo/lib/BUILD <<'EOF'
3057+
cc_binary(
3058+
name = "l,ib",
3059+
srcs = ["lib.cpp"],
3060+
linkshared = True,
3061+
linkstatic = True,
3062+
)
3063+
3064+
cc_import(
3065+
name = "dynamic_l,ib",
3066+
shared_library = ":l,ib",
3067+
hdrs = ["lib.h"],
3068+
visibility = ["//visibility:public"],
3069+
)
3070+
EOF
3071+
cat > $repo/lib/lib.h <<'EOF'
3072+
void print_greeting();
3073+
EOF
3074+
cat > $repo/lib/lib.cpp <<'EOF'
3075+
#include <cstdio>
3076+
void print_greeting() {
3077+
printf("Hello, world!\n");
3078+
}
3079+
EOF
3080+
3081+
mkdir -p $repo/pkg
3082+
cat > $repo/pkg/BUILD <<'EOF'
3083+
cc_binary(
3084+
name = "tool",
3085+
srcs = ["tool.cpp"],
3086+
deps = ["//lib:dynamic_l,ib"],
3087+
)
3088+
3089+
genrule(
3090+
name = "rule",
3091+
outs = ["out"],
3092+
cmd = "$(location :tool) > $@",
3093+
tools = [":tool"],
3094+
)
3095+
EOF
3096+
cat > $repo/pkg/tool.cpp <<'EOF'
3097+
#include "lib/lib.h"
3098+
int main() {
3099+
print_greeting();
3100+
}
3101+
EOF
3102+
}
3103+
3104+
function test_cc_binary_tool_with_dynamic_deps() {
3105+
if [[ "$PLATFORM" == "darwin" ]]; then
3106+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3107+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3108+
# action executors in order to select the appropriate Xcode toolchain.
3109+
return 0
3110+
fi
3111+
3112+
setup_cc_binary_tool_with_dynamic_deps .
3113+
3114+
bazel build \
3115+
--remote_executor=grpc://localhost:${worker_port} \
3116+
//pkg:rule >& $TEST_log || fail "Build should succeed"
3117+
}
3118+
3119+
function test_cc_binary_tool_with_dynamic_deps_sibling_repository_layout() {
3120+
if [[ "$PLATFORM" == "darwin" ]]; then
3121+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3122+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3123+
# action executors in order to select the appropriate Xcode toolchain.
3124+
return 0
3125+
fi
3126+
3127+
setup_cc_binary_tool_with_dynamic_deps .
3128+
3129+
bazel build \
3130+
--experimental_sibling_repository_layout \
3131+
--remote_executor=grpc://localhost:${worker_port} \
3132+
//pkg:rule >& $TEST_log || fail "Build should succeed"
3133+
}
3134+
3135+
function test_external_cc_binary_tool_with_dynamic_deps() {
3136+
if [[ "$PLATFORM" == "darwin" ]]; then
3137+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3138+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3139+
# action executors in order to select the appropriate Xcode toolchain.
3140+
return 0
3141+
fi
3142+
3143+
setup_cc_binary_tool_with_dynamic_deps other_repo
3144+
3145+
bazel build \
3146+
--remote_executor=grpc://localhost:${worker_port} \
3147+
@other_repo//pkg:rule >& $TEST_log || fail "Build should succeed"
3148+
}
3149+
3150+
function test_external_cc_binary_tool_with_dynamic_deps_sibling_repository_layout() {
3151+
if [[ "$PLATFORM" == "darwin" ]]; then
3152+
# TODO(b/37355380): This test is disabled due to RemoteWorker not supporting
3153+
# setting SDKROOT and DEVELOPER_DIR appropriately, as is required of
3154+
# action executors in order to select the appropriate Xcode toolchain.
3155+
return 0
3156+
fi
3157+
3158+
setup_cc_binary_tool_with_dynamic_deps other_repo
3159+
3160+
bazel build \
3161+
--experimental_sibling_repository_layout \
3162+
--remote_executor=grpc://localhost:${worker_port} \
3163+
@other_repo//pkg:rule >& $TEST_log || fail "Build should succeed"
3164+
}
3165+
30403166
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
],

0 commit comments

Comments
 (0)