Skip to content
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

RFC: Allow symbol re-export in cdylib crate from linked staticlib #3556

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

aidanhs
Copy link
Member

@aidanhs aidanhs commented Jan 6, 2024

cc #2771, #3435, rust-lang/rust#110624

Not sure if compiler team or lang team owns this.

Rendered

@aidanhs aidanhs changed the title Allow symbol re-export in cdylib crate from linked staticlib RFC: Allow symbol re-export in cdylib crate from linked staticlib Jan 6, 2024
@Noratrieb Noratrieb added T-lang Relevant to the language team, which will review and decide on the RFC. T-compiler Relevant to the compiler team, which will review and decide on the RFC. labels Jan 7, 2024
text/3556-re-export-symbols.md Outdated Show resolved Hide resolved
text/3556-re-export-symbols.md Outdated Show resolved Hide resolved
@vi
Copy link

vi commented Jan 11, 2024

Does this RFC propose to use pub fn foo to re-export the foo symbol even if it is actually not a function?

@aidanhs
Copy link
Member Author

aidanhs commented Jan 25, 2024

I've updated the RFC based on the comments above, including:

  • clarifying that the RFC proposes to re-export both fn and static items if they have no_mangle applied on them in an extern block (in practice, on Linux if you re-export using the wrong type of item it'll work anyway...but such behavior is unspecified)
  • adding some detail about when the warning was added in Emit specific warning to clarify that #[no_mangle] should not be applied on foreign statics or functions rust#86376. cc @asquared31415, I want to make sure I've fairly represented the reasoning you had at the time and that you're convinced about the removal of the warning
  • elaborating that there are multiple ways to explicitly specify a library is to be linked as static, both via the rustc command line as well as inline as an annotation on the extern block

The linked repository at https://github.com/aidanhs/rust-re-export-lib is now a much more extensive demonstration of the assorted combinations of pub, no_mangle and fn vs static. Take a look at the ext_rfc3556 extern block in https://github.com/aidanhs/rust-re-export-lib/blob/master/src/lib.rs, which describes what should happen in each case - on Linux, all the cases produce the correct binary output, but the warnings proposed by this RFC are missing.

Allows 'correct' re-export of symbols that need C++ types
@aidanhs
Copy link
Member Author

aidanhs commented Feb 7, 2024

@rustbot label +I-lang-nominated

Nominating this for the Lang Team to request that warning: `[no_mangle]` has no effect on a foreign function not be moved to a hard error before this RFC is reviewed.

(I'm not in a hurry for a review or discussion of the RFC itself - the feature works fine for me right now, I just don't want to regress!)

@rustbot

This comment was marked as resolved.

@compiler-errors compiler-errors added the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Feb 7, 2024
@asquared31415
Copy link

I want to make sure I've fairly represented the reasoning you had at the time and that you're convinced about the removal of the warning

I am fairly certain my reasoning at the time was along the lines of "no_mangle does too many things, and none of them are documented. It doesn't make sense to import and export the same symbol, because whatever you were exporting to could just import it from wherever you got the symbol." I think that this comment represents the best of my understanding at the time. I think there was too much focus on the fact that foreign items are not mangled (because they have to import by name) and a misunderstanding about exactly how no_mangle works, I think that everyone was operating under the assumptions stated in this comment by @Mark-Simulacrum:

Hm,I believe that no_mangle does not reexport extern static/fns (which are in some sense imports, as you say), and so no_mangle has no effect whatsoever on them, right?

It turns out that this isn't entirely true, specifically because something might import from a static lib and export as a dynamic lib, and the attribute does indeed make the symbol global. I think that it is reasonable to allow this use case and remove the warning stating that it is useless.

I still maintain the position of "no_mangle does too many things, its name doesn't describe half of its function, and docs don't exist". I would love to see work on that, but that is independent of this RFC.

warning: `#[no_mangle]` with `pub` is only valid for re-export when the library is explicitly a `staticlib`
```

If the item is marked as `#[no_mangle]` without being publicly visible in the crate (e.g. the `pub` annotation is missing or the `extern` block is in a private module), the `#[no_mangle]` annotation will have no effect. A non-fatal warning will be emitted:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I find this counter-intuitive, pub is for Rust visibility, it doesn't care about symbol visibility, and a free, non-pub #[no_mangle] fn foo(); works without a warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

a free, non-pub #[no_mangle] fn foo(); works without a warning

First, can you elaborate on what you mean by 'works' and in what context? My sample repo (https://github.com/aidanhs/rust-re-export-lib) indicates that a no_mangle fn in an extern block without the pub modifier (foo_rfc3556_priv_with_no_mangle) does not get exported as a dynamic symbol.

Separately - I don't object to no_mangle implying the symbol gets exported if there's alignment on that being a desirable route. I just have a vague memory of people not being overly happy with that.

Copy link
Contributor

@madsmtm madsmtm Mar 12, 2024

Choose a reason for hiding this comment

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

a free, non-pub #[no_mangle] fn foo(); works without a warning

First, can you elaborate on what you mean by 'works' and in what context?

By "free" I meant that #[no_mangle] on a function outside of an extern block will export the symbol whether it's used or not, like in your foo_priv_no_mangle_rust_fn.

So I'd expect a #[no_mangle] attribute inside an extern block to behave the same.

3. add a warning for re-exporting a symbol when the symbol is not globally visible
4. add a warning for re-exporting a symbol when the library is not explicitly marked as a staticlib

On point 3 - the goal is to catch cases where a) the user has specified non-staticlib link kind or b) the user has not specified any link kind. The semantics of case (a) are not defined in this RFC (see [future-possibilities](#future-possibilities)), and catching (b) is to avoid the user accidentally falling into case (a) where the linker selects a dylib automatically.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll note that linking can be done in many other ways, the #[link] attribute doesn't have to be on the same extern block. So it may not always be possible / feasible to check which library the symbol comes from, and whether that library is static!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I see this as a best-effort warning - if the extern block has no link attribute at all then there's not much we can do as that is a valid setup. But if you do have a link attribute on an extern block and that library is not explicitly static then possibly there's something wrong (there could also not be, e.g. if the function is actually from another lib rather than the one annotated on the extern block, but that seems unusual?)

Comment on lines +118 to +121
Possible other annotations are:

- `#[re_export]`
- re-use the [proposed `#[export]` annotation](https://github.com/rust-lang/rfcs/pull/3435)
Copy link
Contributor

@madsmtm madsmtm Feb 20, 2024

Choose a reason for hiding this comment

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

The #[no_mangle] explicitly documents that it does two things; disable name mangling and publicly exports the symbol, similar to the #[used] attribute.

Symbol name mangling is already disabled within an extern block, so why not use #[used] for this instead? I don't think it's as clear as #[re_export], so that one is still my favourite, but I think it has slightly closer semantics than #[no_mangle].

Copy link
Member

Choose a reason for hiding this comment

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

#[used] doesn't cause a symbol to be exported. It merely prevents it from being dropped entirely by the linker when there are no uses.


Scenarios out of scope of this RFC include:

1. re-export from a cdylib linked into a final cdylib
Copy link
Contributor

Choose a reason for hiding this comment

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

On Mach-O, I'm unsure about exporting individual symbols (maybe -exported_symbols_list would work?), but I do know that you can re-export an entire library using the -reexport_library linker flag.

warning: `#[no_mangle]` will not re-export private items from `extern` block
```

# Reference-level explanation
Copy link
Contributor

Choose a reason for hiding this comment

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

In light of #3325, have you considered whether the attribute should be unsafe or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

The motivating issue there suggests that allowing rust crates to export symbols at all is unsafe, so (assuming that RFC gets merged) whatever attribute this RFC ends up using would presumably also be unsafe.

If that RFC gets merged first then I'd update this one as needed, or if this goes first then I assume there will be a pass over attributes to see if they should be unsafe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants