From ad67875a5256933fdc29f974f0f752146b3f8050 Mon Sep 17 00:00:00 2001 From: Marcel Hlopko Date: Thu, 24 Oct 2024 13:08:12 +0200 Subject: [PATCH 1/2] Add opt-out from merging cc_lib object files in bindgen This is a ~rollback of https://github.com/bazelbuild/rules_rust/pull/2590, as it turns out the current behavior is incomatible with https://bazel.build/docs/user-manual#dynamic-mode (which is very hard to use in the OSS because of https://github.com/bazelbuild/bazel/issues/4135). In short, the dynamic mode works as follows: * Each cc_library produces object files that are used to produce * a static library (often not materialized but objects get passed as `--start-lib`/`--end-lib` to the linker), * a shared library (only containing the objects, not transitive dependencies), * and an interface library (produced from .so by "stripping function bodies"). * When linking binaries, by default we use the static linking. * When linking tests, by default we use interface libraries for linking, and shared libraries for test execution. The motivation for this is to avoid lengthy linking actions when all that has changed between previous build is the method body (therefore Bazel produces the same interface library, so we don't need to reexecute linking action). We do not support dynamic mode in rules_rust yet. The current bindgen behavior works by taking object from `cc_lib`, and merging them into the `rlib` output. When dynamic_mode is on, we now have a linking action that has the rlib early on the command line, and then interface libraries of dependencies of the `cc_lib`, and then the interface library of the `cc_lib`. For reasons, lld prefers statically defined symbols, it sees that the `rlib` defines the same symbols as `cc_lib` but statically, and errors out with `backward reference detected`, even though `cc_lib` is positioned correctly. The fix for us is to not merge objects. --- bindgen/private/bindgen.bzl | 44 +++++++++++++++++++----------- test/bindgen/bindgen_test.bzl | 50 ++++++++++++++++++++++++++++++++++- 2 files changed, 78 insertions(+), 16 deletions(-) diff --git a/bindgen/private/bindgen.bzl b/bindgen/private/bindgen.bzl index 1b4dc9ba6e..3c67129e92 100644 --- a/bindgen/private/bindgen.bzl +++ b/bindgen/private/bindgen.bzl @@ -78,6 +78,9 @@ def rust_bindgen_library( ): if shared in kwargs: bindgen_kwargs.update({shared: kwargs[shared]}) + if "merge_cc_lib_objects_into_rlib" in kwargs: + bindgen_kwargs.update({"merge_cc_lib_objects_into_rlib": kwargs["merge_cc_lib_objects_into_rlib"]}) + kwargs.pop("merge_cc_lib_objects_into_rlib") rust_bindgen( name = name + "__bindgen", @@ -334,22 +337,28 @@ def _rust_bindgen_impl(ctx): toolchain = None, ) - return [ - _generate_cc_link_build_info(ctx, cc_lib), - # As in https://github.com/bazelbuild/rules_rust/pull/2361, we want - # to link cc_lib to the direct parent (rlib) using `-lstatic=` - # rustc flag. Hence, we do not need to provide the whole CcInfo of - # cc_lib because it will cause the downstream binary to link the cc_lib - # again. The CcInfo here only contains the custom link flags (i.e. - # linkopts attribute) specified by users in cc_lib. - CcInfo( - linking_context = cc_common.create_linking_context( - linker_inputs = depset([cc_common.create_linker_input( - owner = ctx.label, - user_link_flags = _get_user_link_flags(cc_lib), - )]), + if ctx.attr.merge_cc_lib_objects_into_rlib: + providers = [ + _generate_cc_link_build_info(ctx, cc_lib), + # As in https://github.com/bazelbuild/rules_rust/pull/2361, we want + # to link cc_lib to the direct parent (rlib) using `-lstatic=` + # rustc flag. Hence, we do not need to provide the whole CcInfo of + # cc_lib because it will cause the downstream binary to link the cc_lib + # again. The CcInfo here only contains the custom link flags (i.e. + # linkopts attribute) specified by users in cc_lib. + CcInfo( + linking_context = cc_common.create_linking_context( + linker_inputs = depset([cc_common.create_linker_input( + owner = ctx.label, + user_link_flags = _get_user_link_flags(cc_lib), + )]), + ), ), - ), + ] + else: + providers = [cc_lib[CcInfo]] + + return providers + [ OutputGroupInfo( bindgen_bindings = depset([output]), bindgen_c_thunks = depset(([c_output] if wrap_static_fns else [])), @@ -380,6 +389,11 @@ rust_bindgen = rule( doc = "Whether to create a separate .c file for static fns. Requires nightly toolchain, and a header that actually needs this feature (otherwise bindgen won't generate the file and Bazel complains).", default = False, ), + "merge_cc_lib_objects_into_rlib": attr.bool( + doc = ("When True, objects from `cc_lib` will be copied into the `rlib` archive produced by " + + "the rust_library that depends on this `rust_bindgen` rule (using `BuildInfo` provider)"), + default = True, + ), "_cc_toolchain": attr.label( default = Label("@bazel_tools//tools/cpp:current_cc_toolchain"), ), diff --git a/test/bindgen/bindgen_test.bzl b/test/bindgen/bindgen_test.bzl index a5adf5df51..e3877a7bf1 100644 --- a/test/bindgen/bindgen_test.bzl +++ b/test/bindgen/bindgen_test.bzl @@ -2,7 +2,7 @@ load("@rules_cc//cc:defs.bzl", "cc_library") load("@rules_rust//bindgen:defs.bzl", "rust_bindgen_library") -load("@rules_rust//rust:defs.bzl", "rust_binary") +load("@rules_rust//rust:defs.bzl", "rust_binary", "rust_library") load("@rules_testing//lib:analysis_test.bzl", "analysis_test", "test_suite") def _test_cc_linkopt_impl(env, target): @@ -44,10 +44,58 @@ def _test_cc_linkopt(name): impl = _test_cc_linkopt_impl, ) +def _test_cc_lib_object_merging_impl(env, target): + env.expect.that_collection(target.actions).has_size(3) + env.expect.that_action(target.actions[0]).mnemonic().contains("RustBindgen") + env.expect.that_action(target.actions[1]).mnemonic().contains("FileWrite") + env.expect.that_action(target.actions[1]).content().contains("-lstatic=test_cc_lib_object_merging_cc") + env.expect.that_action(target.actions[2]).mnemonic().contains("FileWrite") + env.expect.that_action(target.actions[2]).content().contains("-Lnative=") + +def _test_cc_lib_object_merging_disabled_impl(env, target): + # no FileWrite actions writing arg files registered + env.expect.that_collection(target.actions).has_size(1) + env.expect.that_action(target.actions[0]).mnemonic().contains("RustBindgen") + +def _test_cc_lib_object_merging(name): + cc_library(name = name + "_cc", hdrs = ["simple.h"], srcs = ["simple.cc"]) + + rust_bindgen_library( + name = name + "_rust_bindgen", + cc_lib = name + "_cc", + header = "simple.h", + tags = ["manual"], + ) + + analysis_test( + name = name, + target = name + "_rust_bindgen__bindgen", + impl = _test_cc_lib_object_merging_impl, + ) + +def _test_cc_lib_object_merging_disabled(name): + cc_library(name = name + "_cc", hdrs = ["simple.h"], srcs = ["simple.cc"]) + + rust_bindgen_library( + name = name + "_rust_bindgen", + cc_lib = name + "_cc", + header = "simple.h", + tags = ["manual"], + merge_cc_lib_objects_into_rlib = False, + ) + + analysis_test( + name = name, + target = name + "_rust_bindgen__bindgen", + impl = _test_cc_lib_object_merging_disabled_impl, + ) + def bindgen_test_suite(name): test_suite( name = name, tests = [ _test_cc_linkopt, + _test_cc_lib_object_merging, + _test_cc_lib_object_merging_disabled, ], ) From 53f17bc7fd80245acb8b211a089c0347ebdbc46f Mon Sep 17 00:00:00 2001 From: Marcel Hlopko Date: Thu, 24 Oct 2024 13:46:13 +0200 Subject: [PATCH 2/2] Regenerate documentation --- bindgen/private/bindgen.bzl | 8 ++++---- docs/src/flatten.md | 4 +++- docs/src/rust_bindgen.md | 4 +++- test/bindgen/bindgen_test.bzl | 8 +++++--- 4 files changed, 15 insertions(+), 9 deletions(-) diff --git a/bindgen/private/bindgen.bzl b/bindgen/private/bindgen.bzl index 3c67129e92..8a70be1216 100644 --- a/bindgen/private/bindgen.bzl +++ b/bindgen/private/bindgen.bzl @@ -385,15 +385,15 @@ rust_bindgen = rule( allow_single_file = True, mandatory = True, ), - "wrap_static_fns": attr.bool( - doc = "Whether to create a separate .c file for static fns. Requires nightly toolchain, and a header that actually needs this feature (otherwise bindgen won't generate the file and Bazel complains).", - default = False, - ), "merge_cc_lib_objects_into_rlib": attr.bool( doc = ("When True, objects from `cc_lib` will be copied into the `rlib` archive produced by " + "the rust_library that depends on this `rust_bindgen` rule (using `BuildInfo` provider)"), default = True, ), + "wrap_static_fns": attr.bool( + doc = "Whether to create a separate .c file for static fns. Requires nightly toolchain, and a header that actually needs this feature (otherwise bindgen won't generate the file and Bazel complains).", + default = False, + ), "_cc_toolchain": attr.label( default = Label("@bazel_tools//tools/cpp:current_cc_toolchain"), ), diff --git a/docs/src/flatten.md b/docs/src/flatten.md index c16189963b..bcbb000481 100644 --- a/docs/src/flatten.md +++ b/docs/src/flatten.md @@ -329,7 +329,8 @@ is available under the key `dsym_folder` in `OutputGroupInfo`. ## rust_bindgen 
-rust_bindgen(name, bindgen_flags, cc_lib, clang_flags, header, wrap_static_fns)
+rust_bindgen(name, bindgen_flags, cc_lib, clang_flags, header, merge_cc_lib_objects_into_rlib,
+             wrap_static_fns)
Generates a rust source file from a cc_library and a header. @@ -344,6 +345,7 @@ Generates a rust source file from a cc_library and a header. | cc_lib | The cc_library that contains the `.h` file. This is used to find the transitive includes. | Label | required | | | clang_flags | Flags to pass directly to the clang executable. | List of strings | optional | `[]` | | header | The `.h` file to generate bindings for. | Label | required | | +| merge_cc_lib_objects_into_rlib | When True, objects from `cc_lib` will be copied into the `rlib` archive produced by the rust_library that depends on this `rust_bindgen` rule (using `BuildInfo` provider) | Boolean | optional | `True` | | wrap_static_fns | Whether to create a separate .c file for static fns. Requires nightly toolchain, and a header that actually needs this feature (otherwise bindgen won't generate the file and Bazel complains). | Boolean | optional | `False` | diff --git a/docs/src/rust_bindgen.md b/docs/src/rust_bindgen.md index 7489a7cbd3..2c139530cc 100644 --- a/docs/src/rust_bindgen.md +++ b/docs/src/rust_bindgen.md @@ -53,7 +53,8 @@ toolchains following the instructions for [rust_bindgen_toolchain](#rust_bindgen ## rust_bindgen 
-rust_bindgen(name, bindgen_flags, cc_lib, clang_flags, header, wrap_static_fns)
+rust_bindgen(name, bindgen_flags, cc_lib, clang_flags, header, merge_cc_lib_objects_into_rlib,
+             wrap_static_fns)
Generates a rust source file from a cc_library and a header. @@ -68,6 +69,7 @@ Generates a rust source file from a cc_library and a header. | cc_lib | The cc_library that contains the `.h` file. This is used to find the transitive includes. | Label | required | | | clang_flags | Flags to pass directly to the clang executable. | List of strings | optional | `[]` | | header | The `.h` file to generate bindings for. | Label | required | | +| merge_cc_lib_objects_into_rlib | When True, objects from `cc_lib` will be copied into the `rlib` archive produced by the rust_library that depends on this `rust_bindgen` rule (using `BuildInfo` provider) | Boolean | optional | `True` | | wrap_static_fns | Whether to create a separate .c file for static fns. Requires nightly toolchain, and a header that actually needs this feature (otherwise bindgen won't generate the file and Bazel complains). | Boolean | optional | `False` | diff --git a/test/bindgen/bindgen_test.bzl b/test/bindgen/bindgen_test.bzl index e3877a7bf1..92b010dba4 100644 --- a/test/bindgen/bindgen_test.bzl +++ b/test/bindgen/bindgen_test.bzl @@ -2,7 +2,7 @@ load("@rules_cc//cc:defs.bzl", "cc_library") load("@rules_rust//bindgen:defs.bzl", "rust_bindgen_library") -load("@rules_rust//rust:defs.bzl", "rust_binary", "rust_library") +load("@rules_rust//rust:defs.bzl", "rust_binary") load("@rules_testing//lib:analysis_test.bzl", "analysis_test", "test_suite") def _test_cc_linkopt_impl(env, target): @@ -45,7 +45,7 @@ def _test_cc_linkopt(name): ) def _test_cc_lib_object_merging_impl(env, target): - env.expect.that_collection(target.actions).has_size(3) + env.expect.that_int(len(target.actions)).is_greater_than(2) env.expect.that_action(target.actions[0]).mnemonic().contains("RustBindgen") env.expect.that_action(target.actions[1]).mnemonic().contains("FileWrite") env.expect.that_action(target.actions[1]).content().contains("-lstatic=test_cc_lib_object_merging_cc") @@ -54,7 +54,7 @@ def _test_cc_lib_object_merging_impl(env, target): def _test_cc_lib_object_merging_disabled_impl(env, target): # no FileWrite actions writing arg files registered - env.expect.that_collection(target.actions).has_size(1) + env.expect.that_int(len(target.actions)).is_greater_than(0) env.expect.that_action(target.actions[0]).mnemonic().contains("RustBindgen") def _test_cc_lib_object_merging(name): @@ -65,6 +65,7 @@ def _test_cc_lib_object_merging(name): cc_lib = name + "_cc", header = "simple.h", tags = ["manual"], + edition = "2021", ) analysis_test( @@ -82,6 +83,7 @@ def _test_cc_lib_object_merging_disabled(name): header = "simple.h", tags = ["manual"], merge_cc_lib_objects_into_rlib = False, + edition = "2021", ) analysis_test(