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

Implemented a linkage attribute for functions and foreign statics. #9966

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Oct 19, 2013

Closes #7196 and obsoletes #9945 (I've reused the test from the latter, with the added linkage(external) attribute).
The subset of allowed linkages is left to be decided by the reviewers.

// except according to those terms.

#[linkage(external)]
#[inline(always)]
Copy link
Member

Choose a reason for hiding this comment

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

Why inline(always)? To check that linkage(external) is forcing it to stay around? (Worth a comment, probably.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes the test behaves the same (when toggling linkage(external)), independently of --opt-level.

@huonw
Copy link
Member

huonw commented Oct 19, 2013

What happens for (e.g.) #[linkage(dll_export)] on Linux?

@eddyb
Copy link
Member Author

eddyb commented Oct 19, 2013

That's odd... LLVM doesn't error when using dll_export on Linux. However, I expect that to be a compatibility feature, since the test works with dll_export as it does with external.

@alexcrichton
Copy link
Member

I'm a little uneasy about this change. Could you elaborate a little more on what the main purpose for these is going to be?

I'm generally of the opinion that we should be able to infer as much of this as possible. We already infer whether to make a function externally visible (or at least we should be correctly doing so). You can very easily shoot yourself in the foot by tinkering with these linkage attributes too much, which rust strives to make it very hard to do so.

This seems to be like more of a bug in other portions of the language than to attempt to allow this on a per-item basis. Our management of linkage of symbols is already hairy enough as-is, and throwing this in the mix could very easily produce unexpected results. I personally don't think that we should have the weak_linkage or link_name attributes, I think that we're starting to allow too much power for such small use cases.

I still believe that all use cases should be encodable in some form of rust, but not necessarily through the use of attributes. Things like:

  • If you want an internal linkage function, you shouldn't make it accessible from the outside world
  • If you want an external linkage function, you should make it accessible from the outside world (pub from the root)
  • If you declare extern "C", we probably shouldn't flag that as internal (seeing how you explicitly wrote the extern on it)
  • It's not clear to me what dll import/export do for windows, so I would need to understand more about them before saying how they should be encoded in rust

Again though, I could just be unaware of perfectly legitimate use cases which definitely need fine-grained control over linkage. My perference would be to have a different way to encode this type of use other than being able to specify the linkage per-item, though.

@thestinger
Copy link
Contributor

@alexcrichton: A private extern "C" function should definitely be internal, it's the way to declare a function with a C ABI. You don't want internal functions you pass as function pointers to C to be external.

@alexcrichton
Copy link
Member

I was looking over the list of linkages that llvm provides, and this is what I ended up noticing:

  • private - this was pretty much the same as internal, I don't personally see why rust source would want this and not internal
  • linker_private - same as above
  • internal - this is quite useful, and I believe that the visibility rust has maps well to this, so I'm not sure why you'd want to explicitly flag this on a function
  • available_externally - this is currently used for inlining globals, but it's not clear to me why you would want to do this manually with a function
  • linkonce - This would be very useful for when we instantiate generic functions, but I don't see why you'd want to place this manually on a function
  • weak - It's not clear that this is necessary, if you want this in rust it seems like you may want extern_weak, but there may be a use case here that I'm not seeing.
  • common - It's not clear to me that we'd want this
  • appending - only works if we use llvm arrays, which I don't believe we are currently doing
  • extern_weak - This is useful sometimes. I'm not sure it totally makes sense to put on a function though, it may only be applicable to statics.
  • *_odr - like generic instantiation, this seems like the compiler should be emitting this instead of users explicitly on fuctions
  • external - very useful, but this is encoded with visibility
  • dllexport and dllimport - I'm still not quite sure what these do, so I wouldn't be able to comment much on them.

From this list, it really seems like the useful ones which you'd want to apply are weak and dllimport/export (although I don't know what they do, so perhaps they are encodable in other rust-isms). Overally, it's not clear to me that we're gaining much by allowing such fine-grained control of linkage attributes. The existing cases where a different linkage is desired seem like a bug that should be fixed or it's wontfix behavior.

That being said, I could certainly be unaware of other use cases!

@thestinger
Copy link
Contributor

@alexcrichton: linkonce isn't useful without translation units that are built together either with static linking or via LTO. It could discard copies of the symbol from within the same crate, but we never have those.

@thestinger
Copy link
Contributor

The changes implemented by #9945 have landed, so I think the use case this was meant for is now well covered. I think the Windows issue is likely separate, I don't really understand why external wouldn't be the same thing.

We can revisit this in the future if a use case comes up.

@thestinger thestinger closed this Nov 3, 2013
@eddyb eddyb deleted the attr-linkage branch February 6, 2016 11:45
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 17, 2022
…=xFrednet

Fix manual_let_else produces a wrong suggestion with or-patterns

Fix rust-lang#9938
changelog: Sugg: [`manual_let_else`]: Suggestions for or-patterns now include required brackets.
[rust-lang#9966](rust-lang/rust-clippy#9966)
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.

Add support for DllExport on Windows
4 participants