Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jan 9, 2024

This commit fixes issue #2406 by using the link-args codegen option of rustc:

-C
link-args=<user_link_flags>

where <user_link_flags> are the flags from the user_link_flags field of LinkerInput.

…` in bindgen.

This commit fixes issue #2406 by using the [`link-args`] codegen option of
`rustc`:

```
-C
link-args=<user_link_flags>
```

where `<user_link_flags>` are the flags from the [`user_link_flags`] field
of [`LinkerInput`].

[`user_link_flags`]: https://bazel.build/rules/lib/builtins/LinkerInput#user_link_flags
[`LinkerInput`]: https://bazel.build/rules/lib/builtins/LinkerInput
[`link-args`]: https://doc.rust-lang.org/rustc/codegen-options/index.html#link-args
@ghost ghost marked this pull request as ready for review January 9, 2024 15:32
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@UebelAndre UebelAndre merged commit 90e4505 into bazelbuild:main Jan 9, 2024
@ghost ghost deleted the fix_2406 branch January 9, 2024 16:29
compile_data.append(lib.pic_static_library)

linker_flags.extend(linker_input.user_link_flags)
if linker_input.user_link_flags:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UebelAndre I think this will only work if we link using rustc. If we run the linker directly (i.e. enabling experimental_use_cc_common_link, it won't work because the linker doesn't understand -C link-args. Only rustc understands -C link-args=<args> and passes <args> to the linker.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang, good catch! Any suggestions on how to deal wit this?

cc @thb-sb

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ho, I haven't though of cc_common.link
So basically, the solution would be to use the former if cc_common.link is used, otherwise the latter.
I'll try something to fix this.
Thank you @daivinhtran for reporting this corner case!

Copy link
Contributor

@tidefield tidefield Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another subtle thing I notice in the example provided in #2406 is that rust_bindgen_library shouldn't trigger any linking (neither calling rustc with -C linker=<> -C link-args or a separate linking action) because the macro (which creates rust_library -> rust_bindgen) only produces an rlib, not a shared library or binary.

Is

# BUILD
load("@rules_rust//bindgen:defs.bzl", "rust_bindgen_library")                    
                                                                                 
cc_library(                                                                      
    name = "foo",                                                                
    srcs = ["foo.c"],                                                            
    hdrs = ["foo.h"],                                                            
    linkopts = ["-framework", "Security"],                                       
)                                                                                
                                                                                 
rust_bindgen_library(                                                            
    name = "foo_bindgen",                                                        
    cc_lib = ":foo",                                                             
    header = "foo.h",                                                            
)

all you need to reproduce the error with the link flags? Or is there another downstream target like rust_binary that requires linking?

Copy link
Contributor

@tidefield tidefield Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the right fix isn't simply

  1. If use cc_common.link, don't use -C link-args=<link flags>
  2. Else, use -C link-args
    at the bindgen rule level.

IMO the right fix is to keep link flags without -C link-args= as before.

At the downstream (shared library or binary) targets which run linking, we then decide whether to add -C link-args or not based on the setting of experimental_use_cc_common_link. We already have this logic implemented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More context: In

if ("link" in emit and crate_info.type not in ["rlib", "lib"]) or rustdoc:
# Rust's built-in linker can handle linking wasm files. We don't want to attempt to use the cc
# linker since it won't understand.
compilation_mode = ctx.var["COMPILATION_MODE"]
if toolchain.target_arch != "wasm32":
if output_dir:
use_pic = _should_use_pic(cc_toolchain, feature_configuration, crate_info.type, compilation_mode)
rpaths = _compute_rpaths(toolchain, output_dir, dep_info, use_pic)
else:
rpaths = depset()
ld, link_args, link_env = get_linker_and_args(ctx, attr, crate_info.type, cc_toolchain, feature_configuration, rpaths, rustdoc)
env.update(link_env)
rustc_flags.add(ld, format = "--codegen=linker=%s")
rustc_flags.add_joined("--codegen", link_args, join_with = " ", format_joined = "link-args=%s")
.

The rules' implementation already ignore linking when building rlib and lib. rust_bindgen_library produces rlib which shouldn't involve any link flags at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed #2413 to triage the issue.

tidefield pushed a commit to tidefield/rules_rust that referenced this pull request Jan 12, 2024
…d `user_link_flags` in bindgen. (bazelbuild#2407)"

This reverts commit 90e4505.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants