-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
rustc: Support various flavors of linkages #12556
Conversation
cc #11978 (original implementation) |
type F = extern "C" unsafe fn(*libc::pthread_attr_t) -> libc::size_t; | ||
extern { | ||
#[linkage = "extern_weak"] | ||
static __pthread_get_minstack: *u8; |
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.
I get that it is a pointer type/length anyway, but why the *u8 instead of *F?
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.
fn()
is a function pointer already, so *F
would be a pointer to a function pointer. (And this is part of the reason that this change was necessary: function pointers are a slightly peculiar mix of pointer and value.)
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.
@huonw's answer is spot on.
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.
So the FFI tutorial should recommend that *u8 is used in this case? (I was thinking about writing up some docs for this when it lands and just want to know what the recommended practice is)
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.
This is a gated feature because it is not supported on all platforms and I'm not sure that we'll want to stick with this interface forever. I wouldn't be opposed to docs, of course, though.
Any pointer would suffice here, it's essentially a "pointer to instructions" which varies per platform. I suppose a more proper type would be something like *()
so you can't read it?
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.
*()
feels more special than rand(*u8, *i8, *u16, *i16, etc). ;)
Is it worth adding a WeakFn trait to std::ptr. Just copy the RawPtr but change the to_option to give an |
Due to this being a gated feature, I don't think that's necessary at this time. |
fn main() { | ||
assert!(!foo.is_null()); | ||
assert_eq!(unsafe { *foo }, 3); | ||
asset!(something_that_should_never_exist.is_null()); |
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.
Neat macro, maybe it should take an argument that's the monetary value of the asset too?
(I think you're missing an r
.)
Bors is close to getting bored. Is this waiting on anything? |
ping r? |
@@ -0,0 +1,14 @@ | |||
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT |
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.
I'd prefer to have linkage[123].rs
squashed in a single compile-fail test. Also, it would be nice to have a small description of what the test is doing.
Could the test file be renamed to linkage-weak
?
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.
linkage1
must be standalone because it's testing the feature gate, and the other two could be combined, but there'll always be at least 2.
These are testing the linkage
attribute, not just weak linkage, so linkage-weak
is a bit deceptive.
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.
+1 for not renaming, it'd be nice to have a comment and perhaps merge the other 2, though. I'd expect other linkage tests to be added there.
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.
Ah, I remember now, an error is a span_fatal
rather than a span_err
, so that's why they're in separate files.
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.
oh ok, then nevermind, 😄 (mind adding a comment?)
I just have a couple of nitpicky comments. |
It is often convenient to have forms of weak linkage or other various types of linkage. Sadly, just using these flavors of linkage are not compatible with Rust's typesystem and how it considers some pointers to be non-null. As a compromise, this commit adds support for weak linkage to external symbols, but it requires that this is only placed on extern statics of type `*T`. Codegen-wise, we get translations like: // rust code extern { #[linkage = "extern_weak"] static foo: *i32; } // generated IR @foo = extern_weak global i32 @_some_internal_symbol = internal global *i32 @foo All references to the rust value of `foo` then reference `_some_internal_symbol` instead of the symbol `_foo` itself. This allows us to guarantee that the address of `foo` will never be null while the value may sometimes be null. An example was implemented in `std::rt::thread` to determine if `__pthread_get_minstack()` is available at runtime, and a test is checked in to use it for a static value as well. Function pointers a little odd because you still need to transmute the pointer value to a function pointer, but it's thankfully better than not having this capability at all.
It is often convenient to have forms of weak linkage or other various types of linkage. Sadly, just using these flavors of linkage are not compatible with Rust's typesystem and how it considers some pointers to be non-null. As a compromise, this commit adds support for weak linkage to external symbols, but it requires that this is only placed on extern statics of type `*T`. Codegen-wise, we get translations like: ```rust // rust code extern { #[linkage = "extern_weak"] static foo: *i32; } // generated IR @foo = extern_weak global i32 @_some_internal_symbol = internal global *i32 @foo ``` All references to the rust value of `foo` then reference `_some_internal_symbol` instead of the symbol `_foo` itself. This allows us to guarantee that the address of `foo` will never be null while the value may sometimes be null. An example was implemented in `std::rt::thread` to determine if `__pthread_get_minstack()` is available at runtime, and a test is checked in to use it for a static value as well. Function pointers a little odd because you still need to transmute the pointer value to a function pointer, but it's thankfully better than not having this capability at all. Thanks to @bnoordhuis for the original patch, most of this work is still his!
…t, r=Veykril fix: Support generics in extract_function assist This change attempts to resolve issue rust-lang#7636: Extract into Function does not create a generic function with constraints when extracting generic code. In `FunctionBody::analyze_container`, we now traverse the `ancestors` in search of `AnyHasGenericParams`, and attach any `GenericParamList`s and `WhereClause`s we find to the `ContainerInfo`. Later, in `format_function`, we collect all the `GenericParam`s and `WherePred`s from the container, and filter them to keep only types matching `TypeParam`s used within the newly extracted function body or param list. We can then include the new `GenericParamList` and `WhereClause` in the new function definition. This change only impacts `TypeParam`s. `LifetimeParam`s and `ConstParam`s are out of scope for this change. I've never contributed to this project before, but I did try to follow the style guide. I believe that this change represents an improvement over the status quo, but I think it's also fair to argue that it doesn't fully "fix" the linked issue. I'm totally open to merging this as is, or going further to try to make a more complete solution. Also: if there are other unit or integration tests I should add, please let me know where to look!
It is often convenient to have forms of weak linkage or other various types of
linkage. Sadly, just using these flavors of linkage are not compatible with
Rust's typesystem and how it considers some pointers to be non-null.
As a compromise, this commit adds support for weak linkage to external symbols,
but it requires that this is only placed on extern statics of type
*T
.Codegen-wise, we get translations like:
All references to the rust value of
foo
then reference_some_internal_symbol
instead of the symbol
_foo
itself. This allows us to guarantee that theaddress of
foo
will never be null while the value may sometimes be null.An example was implemented in
std::rt::thread
to determine if__pthread_get_minstack()
is available at runtime, and a test is checked in touse it for a static value as well. Function pointers a little odd because you
still need to transmute the pointer value to a function pointer, but it's
thankfully better than not having this capability at all.
Thanks to @bnoordhuis for the original patch, most of this work is still his!