-
Notifications
You must be signed in to change notification settings - Fork 471
Linker script support for rust_shared_library
and cc_common.link
#2090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -954,6 +954,12 @@ rust_shared_library = rule( | |||
implementation = _rust_shared_library_impl, | |||
attrs = dict( | |||
_common_attrs.items() + _experimental_use_cc_common_link_attrs.items() + { | |||
"linker_script": attr.label( | |||
doc = dedent("""\ | |||
Link script to forward into linker via rustc options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you indent this and the next line so that they both start at +4 spaces relative to where the doc =
on the previous line starts at
if toolchain.target_os == "windows": | ||
fail("Linking with cc_common.link for windows is currently not supported") | ||
else: | ||
user_link_flags = ["-Wl,--version-script={}".format(linker_script.path)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these user_link_flags necessary? Looking a bit into --version-script, https://sourceware.org/binutils/docs/ld/VERSION.html, it seems it's for a special case of linker scripts. From reading up there, naively I'd expect that the linker will automatically pick up linker scripts by just directly listing the linker script as an input to the linker command line invocation, so I'd expect naively that just passing it to the additional_inputs will have the same effect?
Otherwise if that doesn't work for some reason, could we see if passing this via -T
works: https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_chapter/ld_3.html#:~:text=You%20may%20supply%20a%20command,use%20the%20%60%2DT'%20option.
We currently support linker scripts for
rust_binary
but not forrust_shared_library
. This PR adds thelinker_script
attribute torust_shared_library
as well and tests for both.Additionally, this PR plumbs support for linker scripts when linking via
cc_common.link
, and adds tests for it. I made it an explicit failure to use linker scripts on Windows withcc_common.link
as I'm not very fluent in how Windows works (I think I should have added a/DEF: script_path
to the command line and things would have just worked, but we don't even testcc_common.link
on Windows today, so I'd rather skip supporting it until we have somebody asking for it)