From c603a7710d4a8e88163f81754b547852fe9fd42d Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Sat, 26 Oct 2024 21:36:01 +0100 Subject: [PATCH] Add per-target link flags in the right place (#2963) Previously we were adding these early on in the link args. This was a bug - if some transitive dep foo has a linkopt of `-lbar`, the `-lbar` needs to be later in the link args list than `-lfoo`. Rather than arbitrarily adding them early on and hoping things work, this adds the args for each library just after we add a dependency on that library. --- bindgen/private/bindgen.bzl | 2 +- cargo/private/cargo_build_script.bzl | 2 +- examples/hello_world/src/main.rs | 2 +- rust/private/rustc.bzl | 18 ++-- test/linker_inputs_propagation/BUILD.bazel | 83 +++++++++++-------- .../bar_uses_shared_foo_for_rust.rs | 12 +++ .../main_uses_foo.rs | 5 ++ .../linker_inputs_propagation_test.bzl | 39 +++++++-- 8 files changed, 110 insertions(+), 53 deletions(-) create mode 100644 test/linker_inputs_propagation/bar_uses_shared_foo_for_rust.rs create mode 100644 test/linker_inputs_propagation/main_uses_foo.rs diff --git a/bindgen/private/bindgen.bzl b/bindgen/private/bindgen.bzl index dc4e77ce5..1b4dc9ba6 100644 --- a/bindgen/private/bindgen.bzl +++ b/bindgen/private/bindgen.bzl @@ -303,7 +303,7 @@ def _rust_bindgen_impl(ctx): open_arg = True continue - _, _, linker_env = get_linker_and_args(ctx, ctx.attr, "bin", cc_toolchain, feature_configuration, None) + _, _, linker_env = get_linker_and_args(ctx, "bin", cc_toolchain, feature_configuration, None) env.update(**linker_env) # Set the dynamic linker search path so that clang uses the libstdcxx from the toolchain. diff --git a/cargo/private/cargo_build_script.bzl b/cargo/private/cargo_build_script.bzl index 8908d441b..b8b8859be 100644 --- a/cargo/private/cargo_build_script.bzl +++ b/cargo/private/cargo_build_script.bzl @@ -385,7 +385,7 @@ def _cargo_build_script_impl(ctx): # Pull in env vars which may be required for the cc_toolchain to work (e.g. on OSX, the SDK version). # We hope that the linker env is sufficient for the whole cc_toolchain. cc_toolchain, feature_configuration = find_cc_toolchain(ctx) - linker, link_args, linker_env = get_linker_and_args(ctx, ctx.attr, "bin", cc_toolchain, feature_configuration, None) + linker, link_args, linker_env = get_linker_and_args(ctx, "bin", cc_toolchain, feature_configuration, None) env.update(**linker_env) env["LD"] = linker env["LDFLAGS"] = " ".join(_pwd_flags(link_args)) diff --git a/examples/hello_world/src/main.rs b/examples/hello_world/src/main.rs index 03cf43fcf..6382379b3 100644 --- a/examples/hello_world/src/main.rs +++ b/examples/hello_world/src/main.rs @@ -18,5 +18,5 @@ use hello_lib::greeter; fn main() { let hello = greeter::Greeter::new("Hello"); - hello.greet("world"); + hello.greet("world!"); } diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index 866b9c235..54cecab76 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -398,12 +398,11 @@ def get_cc_user_link_flags(ctx): """ return ctx.fragments.cpp.linkopts -def get_linker_and_args(ctx, attr, crate_type, cc_toolchain, feature_configuration, rpaths, add_flags_for_binary = False): +def get_linker_and_args(ctx, crate_type, cc_toolchain, feature_configuration, rpaths, add_flags_for_binary = False): """Gathers cc_common linker information Args: ctx (ctx): The current target's context object - attr (struct): Attributes to use in gathering linker args crate_type (str): The target crate's type (i.e. "bin", "proc-macro", etc.). cc_toolchain (CcToolchain): cc_toolchain for which we are creating build variables. feature_configuration (FeatureConfiguration): Feature configuration to be queried. @@ -437,13 +436,6 @@ def get_linker_and_args(ctx, attr, crate_type, cc_toolchain, feature_configurati else: fail("Unknown `crate_type`: {}".format(crate_type)) - # Add linkopts from dependencies. This includes linkopts from transitive - # dependencies since they get merged up. - for dep in getattr(attr, "deps", []): - if CcInfo in dep and dep[CcInfo].linking_context: - for linker_input in dep[CcInfo].linking_context.linker_inputs.to_list(): - for flag in linker_input.user_link_flags: - user_link_flags.append(flag) link_variables = cc_common.create_link_variables( feature_configuration = feature_configuration, cc_toolchain = cc_toolchain, @@ -1013,7 +1005,7 @@ def construct_arguments( else: rpaths = depset() - ld, link_args, link_env = get_linker_and_args(ctx, attr, crate_info.type, cc_toolchain, feature_configuration, rpaths, add_flags_for_binary = add_flags_for_binary) + ld, link_args, link_env = get_linker_and_args(ctx, crate_info.type, cc_toolchain, feature_configuration, rpaths, add_flags_for_binary = add_flags_for_binary) env.update(link_env) rustc_flags.add(ld, format = "--codegen=linker=%s") @@ -1957,6 +1949,9 @@ def _portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name, for_windows return [] +def _add_user_link_flags(ret, linker_input): + ret.extend(["--codegen=link-arg={}".format(flag) for flag in linker_input.user_link_flags]) + def _make_link_flags_windows(make_link_flags_args, flavor_msvc): linker_input, use_pic, ambiguous_libs, include_link_flags = make_link_flags_args ret = [] @@ -1975,6 +1970,7 @@ def _make_link_flags_windows(make_link_flags_args, flavor_msvc): ]) elif include_link_flags: ret.extend(_portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name_for_windows, for_windows = True, flavor_msvc = flavor_msvc)) + _add_user_link_flags(ret, linker_input) return ret def _make_link_flags_windows_msvc(make_link_flags_args): @@ -1994,6 +1990,7 @@ def _make_link_flags_darwin(make_link_flags_args): ]) elif include_link_flags: ret.extend(_portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name_default, for_darwin = True)) + _add_user_link_flags(ret, linker_input) return ret def _make_link_flags_default(make_link_flags_args): @@ -2011,6 +2008,7 @@ def _make_link_flags_default(make_link_flags_args): ]) elif include_link_flags: ret.extend(_portable_link_flags(lib, use_pic, ambiguous_libs, get_lib_name_default)) + _add_user_link_flags(ret, linker_input) return ret def _libraries_dirnames(make_link_flags_args): diff --git a/test/linker_inputs_propagation/BUILD.bazel b/test/linker_inputs_propagation/BUILD.bazel index 5ff0945b8..e088e1928 100644 --- a/test/linker_inputs_propagation/BUILD.bazel +++ b/test/linker_inputs_propagation/BUILD.bazel @@ -1,6 +1,7 @@ load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_import", "cc_library", "cc_test") load( "@rules_rust//rust:defs.bzl", + "rust_binary", "rust_library", "rust_shared_library", "rust_static_library", @@ -20,9 +21,15 @@ cc_library( "foo_shared.cc", ], hdrs = ["foo_shared.h"], - target_compatible_with = [ - "@platforms//os:linux", +) + +cc_library( + name = "foo_with_linkopts", + srcs = [ + "foo_shared.cc", ], + hdrs = ["foo_shared.h"], + linkopts = ["-L/doesnotexist"], ) cc_binary( @@ -74,21 +81,20 @@ rust_static_library( name = "staticlib_uses_foo", srcs = ["bar_uses_foo.rs"], edition = "2018", - target_compatible_with = [ - "@platforms//os:linux", - "@platforms//os:windows", - ], deps = [":foo"], ) +rust_library( + name = "rlib_uses_foo_with_redundant_linkopts", + srcs = ["bar_uses_shared_foo_for_rust.rs"], + edition = "2018", + deps = [":foo_with_linkopts"], +) + rust_shared_library( name = "sharedlib_uses_foo", srcs = ["bar_uses_foo.rs"], edition = "2018", - target_compatible_with = [ - "@platforms//os:linux", - "@platforms//os:windows", - ], deps = [":foo"], ) @@ -96,10 +102,6 @@ rust_static_library( name = "staticlib_uses_shared_foo", srcs = ["bar_uses_shared_foo.rs"], edition = "2018", - target_compatible_with = [ - "@platforms//os:linux", - "@platforms//os:windows", - ], deps = [":import_foo_shared"], ) @@ -107,49 +109,62 @@ rust_static_library( name = "sharedlib_uses_shared_foo", srcs = ["bar_uses_shared_foo.rs"], edition = "2018", - target_compatible_with = [ - "@platforms//os:linux", - "@platforms//os:windows", - ], deps = [":import_foo_shared"], ) cc_test( name = "depends_on_foo_via_staticlib", srcs = ["baz.cc"], - target_compatible_with = [ - "@platforms//os:linux", - "@platforms//os:windows", - ], + target_compatible_with = select({ + "@platforms//os:linux": ["@platforms//:incompatible"], + "@platforms//os:macos": ["@platforms//:incompatible"], + "@platforms//os:windows": ["@platforms//:incompatible"], + "//conditions:default": [], + }), deps = [":staticlib_uses_foo"], ) cc_test( name = "depends_on_foo_via_sharedlib", srcs = ["baz.cc"], - target_compatible_with = [ - "@platforms//os:linux", - "@platforms//os:windows", - ], + target_compatible_with = select({ + "@platforms//os:linux": ["@platforms//:incompatible"], + "@platforms//os:macos": ["@platforms//:incompatible"], + "@platforms//os:windows": ["@platforms//:incompatible"], + "//conditions:default": [], + }), deps = [":sharedlib_uses_foo"], ) cc_test( name = "depends_on_shared_foo_via_sharedlib", srcs = ["baz.cc"], - target_compatible_with = [ - "@platforms//os:linux", - "@platforms//os:windows", - ], + target_compatible_with = select({ + "@platforms//os:macos": ["@platforms//:incompatible"], + "@platforms//os:windows": ["@platforms//:incompatible"], + "//conditions:default": [], + }), deps = [":sharedlib_uses_shared_foo"], ) cc_test( name = "depends_on_shared_foo_via_staticlib", srcs = ["baz.cc"], - target_compatible_with = [ - "@platforms//os:linux", - "@platforms//os:windows", - ], + target_compatible_with = select({ + "@platforms//os:macos": ["@platforms//:incompatible"], + "@platforms//os:windows": ["@platforms//:incompatible"], + "//conditions:default": [], + }), deps = [":staticlib_uses_shared_foo"], ) + +rust_binary( + name = "depends_on_foo_with_redundant_linkopts", + srcs = ["main_uses_foo.rs"], + edition = "2021", + target_compatible_with = select({ + "@platforms//os:windows": ["@platforms//:incompatible"], + "//conditions:default": [], + }), + deps = [":rlib_uses_foo_with_redundant_linkopts"], +) diff --git a/test/linker_inputs_propagation/bar_uses_shared_foo_for_rust.rs b/test/linker_inputs_propagation/bar_uses_shared_foo_for_rust.rs new file mode 100644 index 000000000..8ae3fd97d --- /dev/null +++ b/test/linker_inputs_propagation/bar_uses_shared_foo_for_rust.rs @@ -0,0 +1,12 @@ +extern "C" { + pub fn foo() -> i32; +} + +/** Safety doc. + + # Safety + +*/ +pub fn double_foo() -> i32 { + 2 * unsafe { foo() } +} diff --git a/test/linker_inputs_propagation/main_uses_foo.rs b/test/linker_inputs_propagation/main_uses_foo.rs new file mode 100644 index 000000000..f9c49f952 --- /dev/null +++ b/test/linker_inputs_propagation/main_uses_foo.rs @@ -0,0 +1,5 @@ +use rlib_uses_foo_with_redundant_linkopts::double_foo; + +fn main() { + println!("{}", double_foo()); +} diff --git a/test/unit/linker_inputs_propagation/linker_inputs_propagation_test.bzl b/test/unit/linker_inputs_propagation/linker_inputs_propagation_test.bzl index 3341acd48..ba8f4166f 100644 --- a/test/unit/linker_inputs_propagation/linker_inputs_propagation_test.bzl +++ b/test/unit/linker_inputs_propagation/linker_inputs_propagation_test.bzl @@ -1,6 +1,6 @@ """Unittests for propagation of linker inputs through Rust libraries""" -load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts") +load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts", "unittest") def _shared_lib_is_propagated_test_impl(ctx): env = analysistest.begin(ctx) @@ -8,7 +8,7 @@ def _shared_lib_is_propagated_test_impl(ctx): link_action = [action for action in tut.actions if action.mnemonic == "CppLink"][0] lib_name = _get_lib_name(ctx, name = "foo_shared") - asserts.true(env, _contains_input(link_action.inputs, lib_name)) + _assert_contains_input(env, link_action.inputs, lib_name) return analysistest.end(env) @@ -18,17 +18,34 @@ def _static_lib_is_not_propagated_test_impl(ctx): link_action = [action for action in tut.actions if action.mnemonic == "CppLink"][0] lib_name = _get_lib_name(ctx, name = "foo") - asserts.false(env, _contains_input(link_action.inputs, lib_name)) + asserts.false(env, _assert_contains_input(env, link_action.inputs, lib_name)) return analysistest.end(env) -def _contains_input(inputs, name): +def _dependency_linkopts_are_propagated_test_impl(ctx): + env = analysistest.begin(ctx) + tut = analysistest.target_under_test(env) + link_action = [action for action in tut.actions if action.mnemonic == "Rustc"][0] + + # Expect a library's own linkopts to come after the flags we create to link them. + # This is required, because linkopts are ordered and the linker will only apply later ones when resolving symbols required for earlier ones. + # This means that if one of our transitive deps has a linkopt like `-lfoo`, the dep will see the symbols of foo at link time. + _assert_contains_in_order(env, link_action.argv, ["-lstatic=foo_with_linkopts", "-Clink-arg=-lfoo_with_linkopts", "--codegen=link-arg=-L/doesnotexist"]) + return analysistest.end(env) + +def _assert_contains_input(env, inputs, name): for input in inputs.to_list(): # We cannot check for name equality because rlib outputs contain # a hash in their name. if input.basename.startswith(name): - return True - return False + return + unittest.fail(env, "Expected {} to contain a library starting with {}".format(inputs.to_list(), name)) + +def _assert_contains_in_order(env, haystack, needle): + for i in range(len(haystack)): + if haystack[i:i + len(needle)] == needle: + return + unittest.fail(env, "Expected {} to contain {}".format(haystack, needle)) def _get_lib_name(ctx, name): if ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo]): @@ -51,6 +68,10 @@ shared_lib_is_propagated_test = analysistest.make( }, ) +dependency_linkopts_are_propagated_test = analysistest.make( + _dependency_linkopts_are_propagated_test_impl, +) + def _linker_inputs_propagation_test(): static_lib_is_not_propagated_test( name = "depends_on_foo_via_staticlib", @@ -72,6 +93,11 @@ def _linker_inputs_propagation_test(): target_under_test = "//test/linker_inputs_propagation:depends_on_shared_foo_via_sharedlib", ) + dependency_linkopts_are_propagated_test( + name = "dependency_linkopts_are_propagated", + target_under_test = "//test/linker_inputs_propagation:depends_on_foo_with_redundant_linkopts", + ) + def linker_inputs_propagation_test_suite(name): """Entry-point macro called from the BUILD file. @@ -87,5 +113,6 @@ def linker_inputs_propagation_test_suite(name): ":depends_on_shared_foo_via_staticlib", ":depends_on_foo_via_sharedlib", ":depends_on_shared_foo_via_sharedlib", + ":dependency_linkopts_are_propagated", ], )