Skip to content

Commit 95ae4db

Browse files
oquenchilcopybara-github
oquenchil
authored and
copybara-github
committed
Stop linking indirect dynamic_deps in binaries
RELNOTES[INC]:cc_binary targets with dynamic_deps attributes no longer link indirect dynamic_deps on Unix. This might be an incompatible change if you are using RUNPATHs (instead of RPATHs) in your cc_shared_libraries. Enable the feature "exclude_bazel_rpaths_in_transitive_libs" or "use_rpath_instead_of_runpath" for those cc_shared_libraries. PiperOrigin-RevId: 542842891 Change-Id: I261e27d96760f0a68a85c7e28961067085f9c400
1 parent 815eccc commit 95ae4db

File tree

9 files changed

+105
-21
lines changed

9 files changed

+105
-21
lines changed

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -831,6 +831,13 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
831831
userLinkFlags.addAll(cppConfiguration.getLtoIndexOptions());
832832
}
833833

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+
}
834841
variables =
835842
LinkBuildVariables.setupVariables(
836843
((RuleContext) actionConstructionContext).getStarlarkThread(),
@@ -858,7 +865,7 @@ public CppLinkAction build() throws InterruptedException, RuleErrorException {
858865
ltoOutputRootPrefix,
859866
defFile != null ? defFile.getExecPathString() : null,
860867
fdoContext,
861-
collectedLibrariesToLink.getRuntimeLibrarySearchDirectories(),
868+
runtimeLibrarySearchDirectories,
862869
collectedLibrariesToLink.getLibrariesToLink(),
863870
collectedLibrariesToLink.getLibrarySearchDirectories(),
864871
/* addIfsoRelatedVariables= */ true);

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,37 @@ 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+
520551
/** Ancestor for all rules that do include scanning. */
521552
public static final class CcIncludeScanningRule implements RuleDefinition {
522553
private final boolean addGrepIncludes;

‎src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,7 @@ def _build_map_direct_dynamic_dep_to_transitive_dynamic_deps(ctx):
347347

348348
return direct_dynamic_dep_to_transitive_dynamic_deps, all_dynamic_dep_linker_inputs
349349

350-
def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context):
350+
def _filter_libraries_that_are_linked_dynamically(ctx, feature_configuration, cc_linking_context):
351351
merged_cc_shared_library_infos = merge_cc_shared_library_infos(ctx)
352352
link_once_static_libs_map = build_link_once_static_libs_map(merged_cc_shared_library_infos)
353353
transitive_exports = build_exports_map_from_only_dynamic_deps(merged_cc_shared_library_infos)
@@ -363,8 +363,6 @@ def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context):
363363
if owner in transitive_exports:
364364
can_be_linked_dynamically[owner] = True
365365

366-
transitive_dynamic_dep_labels = {}
367-
368366
# Entries in unused_dynamic_linker_inputs will be marked None if they are
369367
# used
370368
(
@@ -412,20 +410,27 @@ def _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context):
412410
# Unlike Unix on Windows every dynamic dependency must be linked to the
413411
# main binary, even indirect ones that are dependencies of direct
414412
# dynamic dependencies of this binary. So even though linking indirect
415-
# dynamic dependencies is not needed for Unix, we link them here too
413+
# dynamic dependencies is not needed for Unix, we link them here for tests too
416414
# because we cannot know whether the shared libraries were linked with
417415
# RUNPATH or RPATH. If they were linked with the former, then the loader
418416
# won't search in the runfiles directory of this binary for the library, it
419417
# will only search in the RUNPATH set at the time of linking the shared
420418
# library and we cannot possibly know at that point the runfiles directory
421419
# of all of its dependents.
422-
#
423-
# TODO(bazel-team):Add a feature so that indirect dynamic dependencies are
424-
# not linked. This should be opt-in for those people that are creating a
425-
# production binary and have manually set the RPATH/RUNPATHS correctly.
420+
link_indirect_deps = (
421+
ctx.attr._is_test or
422+
cc_common.is_enabled(feature_configuration = feature_configuration, feature_name = "targets_windows") or
423+
cc_common.is_enabled(
424+
feature_configuration = feature_configuration,
425+
feature_name = "link_indirect_dynamic_deps_in_binary",
426+
)
427+
)
428+
direct_dynamic_dep_labels = {dep[CcSharedLibraryInfo].linker_input.owner: True for dep in ctx.attr.dynamic_deps}
426429
topologically_sorted_labels_set = {label: True for label in topologically_sorted_labels}
427430
for dynamic_linker_input_owner, unused_linker_input in unused_dynamic_linker_inputs.items():
428-
if unused_linker_input:
431+
should_link_input = (unused_linker_input and
432+
(link_indirect_deps or dynamic_linker_input_owner in direct_dynamic_dep_labels))
433+
if should_link_input:
429434
_add_linker_input_to_dict(
430435
dynamic_linker_input_owner,
431436
unused_dynamic_linker_inputs[dynamic_linker_input_owner],
@@ -523,7 +528,7 @@ def _create_transitive_linking_actions(
523528
cc_linking_context = cc_info.linking_context
524529

525530
if len(ctx.attr.dynamic_deps) > 0:
526-
cc_linking_context = _filter_libraries_that_are_linked_dynamically(ctx, cc_linking_context)
531+
cc_linking_context = _filter_libraries_that_are_linked_dynamically(ctx, feature_configuration, cc_linking_context)
527532
link_deps_statically = True
528533
if linking_mode == linker_mode.LINKING_DYNAMIC:
529534
link_deps_statically = False
@@ -659,6 +664,7 @@ def cc_binary_impl(ctx, additional_linkopts):
659664
if ctx.attr._is_test and cpp_config.incompatible_enable_cc_test_feature:
660665
features.append("is_cc_test")
661666
disabled_features.append("legacy_is_cc_test")
667+
662668
feature_configuration = cc_common.configure_features(
663669
ctx = ctx,
664670
cc_toolchain = cc_toolchain,

‎src/main/starlark/builtins_bzl/common/cc/cc_shared_library.bzl‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -417,8 +417,10 @@ def _filter_inputs(
417417
linker_inputs_seen = {}
418418
linker_inputs_count = 0
419419
label_to_linker_inputs = {}
420+
experimental_remove_before_7_0_linker_inputs = []
420421

421422
def _add_linker_input_to_dict(owner, linker_input):
423+
experimental_remove_before_7_0_linker_inputs.append(linker_input)
422424
label_to_linker_inputs.setdefault(owner, []).append(linker_input)
423425

424426
# We use this dictionary to give an error if a target containing only
@@ -500,14 +502,17 @@ def _filter_inputs(
500502
message += dynamic_only_root + "\n"
501503
fail(message)
502504

503-
sorted_linker_inputs = _sort_linker_inputs(
504-
topologically_sorted_labels,
505-
label_to_linker_inputs,
506-
linker_inputs_count,
507-
)
505+
if ctx.attr.experimental_disable_topo_sort_do_not_use_remove_before_7_0:
506+
linker_inputs = experimental_remove_before_7_0_linker_inputs
507+
else:
508+
linker_inputs = _sort_linker_inputs(
509+
topologically_sorted_labels,
510+
label_to_linker_inputs,
511+
linker_inputs_count,
512+
)
508513

509514
_throw_linked_but_not_exported_errors(linked_statically_but_not_exported)
510-
return (exports, sorted_linker_inputs, curr_link_once_static_libs_set.keys(), precompiled_only_dynamic_libraries)
515+
return (exports, linker_inputs, curr_link_once_static_libs_set.keys(), precompiled_only_dynamic_libraries)
511516

512517
def _throw_linked_but_not_exported_errors(error_libs_dict):
513518
if not error_libs_dict:

‎src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ licenses(["notice"])
2525

2626
package(
2727
default_visibility = ["//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:__subpackages__"],
28+
features = ["exclude_bazel_rpaths_in_transitive_libs"],
2829
)
2930

3031
py_test(
@@ -45,6 +46,7 @@ cc_binary(
4546
name = "binary",
4647
srcs = ["main.cc"],
4748
dynamic_deps = ["foo_so"],
49+
features = ["use_rpath_instead_of_runpath"],
4850
deps = [
4951
":foo",
5052
],
@@ -131,7 +133,7 @@ cc_shared_library(
131133
"bar_so",
132134
"//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:diff_pkg_so"
133135
],
134-
features = ["windows_export_all_symbols"],
136+
features = ["windows_export_all_symbols", "use_rpath_instead_of_runpath"],
135137
exports_filter = [
136138
":indirect_dep2",
137139
],

‎src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/cc_shared_library_integration_test.sh‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ function test_shared_library_symbols() {
5252

5353
function test_shared_library_user_link_flags() {
5454
foo_so=$(find . -name libfoo_so.so)
55-
objdump -x $foo_so | grep $RPATH | grep "kittens" > /dev/null \
55+
objdump -x $foo_so | grep RPATH | grep "kittens" > /dev/null \
5656
|| (echo "Expected to have RUNPATH contain 'kittens' (set by user_link_flags)" \
5757
&& exit 1)
5858
}
@@ -73,7 +73,16 @@ function test_cc_test() {
7373
do_test_binary $cc_test
7474
}
7575

76+
function test_number_of_linked_libs() {
77+
binary=$(find . -name binary)
78+
expected_num_libs="6"
79+
num_libs=$(readelf -d $binary | grep NEEDED | wc -l)
80+
echo "$num_libs" | (grep -q "$expected_num_libs" \
81+
|| (echo "Expected no more than "$expected_num_libs" linked libraries but was $num_libs" && exit 1))
82+
}
83+
7684
test_shared_library_user_link_flags
7785
test_shared_library_symbols
7886
test_binary
7987
test_cc_test
88+
test_number_of_linked_libs

‎src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/testenv.sh‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,3 @@
1414
# limitations under the License.
1515

1616
LDD_BINARY="ldd"
17-
RPATH="RUNPATH"

‎src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3/BUILD.builtin_test‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ cc_library(
1616

1717
cc_shared_library(
1818
name = "diff_pkg_so",
19-
features = ["windows_export_all_symbols"],
19+
features = ["windows_export_all_symbols", "exclude_bazel_rpaths_in_transitive_libs"],
2020
deps = [
2121
":diff_pkg",
2222
],

‎tools/cpp/unix_cc_toolchain_config.bzl‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -609,6 +609,10 @@ def _impl(ctx):
609609
],
610610
)
611611

612+
link_indirect_dynamic_deps_in_binary_feature = feature(
613+
name = "link_indirect_dynamic_deps_in_binary",
614+
)
615+
612616
fission_support_feature = feature(
613617
name = "fission_support",
614618
flag_sets = [
@@ -1359,6 +1363,23 @@ def _impl(ctx):
13591363

13601364
# TODO(#8303): Mac crosstool should also declare every feature.
13611365
if is_linux:
1366+
use_rpath_instead_of_runpath_feature = feature(
1367+
name = "use_rpath_instead_of_runpath",
1368+
flag_sets = [
1369+
flag_set(
1370+
actions = all_link_actions + lto_index_actions,
1371+
flag_groups = [
1372+
flag_group(
1373+
flags = ["-Wl,--disable-new-dtags"],
1374+
),
1375+
],
1376+
),
1377+
],
1378+
)
1379+
exclude_bazel_rpaths_in_transitive_libs_feature = feature(
1380+
name = "exclude_bazel_rpaths_in_transitive_libs",
1381+
)
1382+
13621383
# Linux artifact name patterns are the default.
13631384
artifact_name_patterns = []
13641385
features = [
@@ -1383,6 +1404,9 @@ def _impl(ctx):
13831404
linkstamps_feature,
13841405
output_execpath_flags_feature,
13851406
runtime_library_search_directories_feature,
1407+
exclude_bazel_rpaths_in_transitive_libs_feature,
1408+
use_rpath_instead_of_runpath_feature,
1409+
link_indirect_dynamic_deps_in_binary_feature,
13861410
library_search_directories_feature,
13871411
libtool_feature,
13881412
archiver_flags_feature,
@@ -1445,6 +1469,7 @@ def _impl(ctx):
14451469
default_link_flags_feature,
14461470
user_link_flags_feature,
14471471
default_link_libs_feature,
1472+
link_indirect_dynamic_deps_in_binary_feature,
14481473
fdo_optimize_feature,
14491474
dbg_feature,
14501475
opt_feature,

0 commit comments

Comments
 (0)