Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign uprustc: Support various flavors of linkages #12556
Conversation
This comment has been minimized.
This comment has been minimized.
|
cc #11978 (original implementation) |
bharrisau
reviewed
Feb 26, 2014
| type F = extern "C" unsafe fn(*libc::pthread_attr_t) -> libc::size_t; | ||
| extern { | ||
| #[linkage = "extern_weak"] | ||
| static __pthread_get_minstack: *u8; |
This comment has been minimized.
This comment has been minimized.
bharrisau
Feb 26, 2014
Contributor
I get that it is a pointer type/length anyway, but why the *u8 instead of *F?
This comment has been minimized.
This comment has been minimized.
huonw
Feb 26, 2014
Member
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.)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bharrisau
Feb 26, 2014
Contributor
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)
This comment has been minimized.
This comment has been minimized.
alexcrichton
Feb 26, 2014
Author
Member
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?
This comment has been minimized.
This comment has been minimized.
huonw
reviewed
Feb 26, 2014
|
|
||
| extern { | ||
| #[linkage = "extern_weak"] | ||
| static foo: *int; |
This comment has been minimized.
This comment has been minimized.
huonw
Feb 26, 2014
Member
What about static hopefully_this_symbol_doesnt_exist: *int; and checking that it's null? (And that compilation succeeds.)
huonw
reviewed
Feb 26, 2014
| ty::ty_ptr(ref mt) => type_of::type_of(ccx, mt.ty), | ||
| _ => { | ||
| ccx.sess.span_fatal(foreign_item.span, | ||
| "must have type `*T`"); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Is it worth adding a WeakFn trait to std::ptr. Just copy the RawPtr but change the to_option to give an |
This comment has been minimized.
This comment has been minimized.
Due to this being a gated feature, I don't think that's necessary at this time. |
huonw
reviewed
Feb 26, 2014
| fn main() { | ||
| assert!(!foo.is_null()); | ||
| assert_eq!(unsafe { *foo }, 3); | ||
| asset!(something_that_should_never_exist.is_null()); |
This comment has been minimized.
This comment has been minimized.
huonw
Feb 26, 2014
Member
Neat macro, maybe it should take an argument that's the monetary value of the asset too?
(I think you're missing an r.)
huonw
reviewed
Feb 26, 2014
|
|
||
| extern { | ||
| #[linkage = "extern_weak"] | ||
| static foo: *mut int; |
This comment has been minimized.
This comment has been minimized.
huonw
Feb 26, 2014
Member
*mut pointing to a non-mut static? Feels like bad practice, even for a test.
(Also, it's a little unfortunate that this only works on Linux :( )
This comment has been minimized.
This comment has been minimized.
alexcrichton
Feb 26, 2014
Author
Member
Sadly weak linkage doesn't work so great elsewhere. I think it may work on android (linux based), but it definitely doesn't work on windows and OSX needs extra linker flags to get it to work. I can swap the mut pointers around though.
huonw
reviewed
Mar 2, 2014
| } | ||
|
|
||
| let ident = link_name(foreign_item); | ||
| match attr::first_attr_value_str_by_name(foreign_item.attrs, "linkage") { |
This comment has been minimized.
This comment has been minimized.
huonw
Mar 2, 2014
Member
It might be better to have #[linkage(extern_weak)] etc, it feels less freeform that way? (And, anyway, we're going off the #[... = "..."] syntax.)
This comment has been minimized.
This comment has been minimized.
alexcrichton
Mar 2, 2014
Author
Member
I was a little curious about that when writing this, but I'm not entirely sure if it's going away at this point. move #[..]; to #![..] then I think we'll keep name = "value" at the top-level for now.
Semantically, "name equals value" seems like the right thing for this rather than "name with a value sub-attribute". Of course this is fairly easy to change, and I don't mind too much.
This comment has been minimized.
This comment has been minimized.
|
Bors is close to getting bored. Is this waiting on anything? |
This comment has been minimized.
This comment has been minimized.
|
ping r? |
flaper87
reviewed
Mar 9, 2014
| @@ -0,0 +1,14 @@ | |||
| // Copyright 2014 The Rust Project Developers. See the COPYRIGHT | |||
This comment has been minimized.
This comment has been minimized.
flaper87
Mar 9, 2014
Contributor
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 ?
This comment has been minimized.
This comment has been minimized.
alexcrichton
Mar 9, 2014
Author
Member
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.
This comment has been minimized.
This comment has been minimized.
flaper87
Mar 10, 2014
Contributor
+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.
This comment has been minimized.
This comment has been minimized.
alexcrichton
Mar 10, 2014
Author
Member
Ah, I remember now, an error is a span_fatal rather than a span_err, so that's why they're in separate files.
This comment has been minimized.
This comment has been minimized.
flaper87
reviewed
Mar 9, 2014
| // currently always the case. Note that you need to check that the symbol | ||
| // is non-null before calling it! | ||
| #[cfg(target_os = "linux", not(stage0))] | ||
| fn min_stack_size(attr: *libc::pthread_attr_t) -> libc::size_t { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I just have a couple of nitpicky comments. |
bors
added a commit
that referenced
this pull request
Mar 10, 2014
bors
added a commit
that referenced
this pull request
Mar 11, 2014
This comment has been minimized.
This comment has been minimized.
|
r=brson |
This comment has been minimized.
This comment has been minimized.
|
saw approval from brson |
This comment has been minimized.
This comment has been minimized.
|
merging alexcrichton/rust/weak-linkage = 699b33d into auto |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
fast-forwarding master to auto = 9f3ebd8 |
alexcrichton commentedFeb 26, 2014
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
foothen reference_some_internal_symbolinstead of the symbol
_fooitself. This allows us to guarantee that theaddress of
foowill never be null while the value may sometimes be null.An example was implemented in
std::rt::threadto 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!