-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Tracking issue for target_feature 1.1 RFC #69098
Comments
I've been working on this, trying to see if I would be able to implement this with my very little knowledge of the compiler. I think I can propose a PR soon. @rustbot claim |
In #69274 (comment), @petrochenkov pointed out a complication not made clear in the RFC (nor realized by anyone during the original discussion, AFAICT): safe trait functions are explicitly excluded because we can't check all call sites, but function items implement the |
It would probably be better to add the That would make the unsafety checker the only place (besides AST lowering) where they would be treated specially. Like, "yes, the function is unsafe, but we know that it's safe to call in this specific context, so the unsafe block can be omitted". The special coercion checks would no longer be necessary in that case. |
…r=hanna-kruppe Implement RFC 2396: `#[target_feature]` 1.1 Tracking issue: rust-lang#69098 r? @nikomatsakis cc @gnzlbg @joshtriplett
…hanna-kruppe Implement RFC 2396: `#[target_feature]` 1.1 Tracking issue: rust-lang#69098 r? @nikomatsakis cc @gnzlbg @joshtriplett
Now that #69274 landed, I think we can check |
@rustbot release-assignment |
Filed #72012 for the unsoundness discussed above (I think this is how T-lang tracking issues are supposed to be used now). |
Opened #73631 to discuss the expected behavior of closures with target-feature. |
It looks like there haven't been any updates on this in a while--is there anything I can do to bring this closer to stabilization? |
@calebzulawski I know it's almost a year later but I think this is probably ready for a stabilization report? (cc @rust-lang/lang) |
Tried to help by opening rust-lang/reference#1181, feedback from people who actually know what they are doing would be greatly appreciated. While writing the PR, I noticed that #72012 is a breaking change for wasm targets, where fn call_it(f: impl FnOnce()) {}
#[target_feature(enable = "simd128")]
fn foo_simd128() {}
fn main() {
foo_simd128(); // OK
call_it(foo_simd128); // error: the trait `FnOnce<()>` is not implemented
} |
…re-11, r=estebank Stabilize `#![feature(target_feature_11)]` ## Stabilization report ### Summary Allows for safe functions to be marked with `#[target_feature]` attributes. Functions marked with `#[target_feature]` are generally considered as unsafe functions: they are unsafe to call, cannot be assigned to safe function pointers, and don't implement the `Fn*` traits. However, calling them from other `#[target_feature]` functions with a superset of features is safe. ```rust // Demonstration function #[target_feature(enable = "avx2")] fn avx2() {} fn foo() { // Calling `avx2` here is unsafe, as we must ensure // that AVX is available first. unsafe { avx2(); } } #[target_feature(enable = "avx2")] fn bar() { // Calling `avx2` here is safe. avx2(); } ``` ### Test cases Tests for this feature can be found in [`src/test/ui/rfcs/rfc-2396-target_feature-11/`](https://github.com/rust-lang/rust/tree/b67ba9ba208ac918228a18321fc3a11a99b1c62b/src/test/ui/rfcs/rfc-2396-target_feature-11/). ### Edge cases - rust-lang#73631 Closures defined inside functions marked with `#[target_feature]` inherit the target features of their parent function. They can still be assigned to safe function pointers and implement the appropriate `Fn*` traits. ```rust #[target_feature(enable = "avx2")] fn qux() { let my_closure = || avx2(); // this call to `avx2` is safe let f: fn() = my_closure; } ``` This means that in order to call a function with `#[target_feature]`, you must show that the target-feature is available while the function executes *and* for as long as whatever may escape from that function lives. ### Documentation - Reference: rust-lang/reference#1181 --- cc tracking issue rust-lang#69098 r? `@ghost`
The two remaining bugs listed here now have mitigations:
There is one bug that carries the label, #58279, but I think that one can be closed. So... seems like there's no longer a blocker here? |
Hm, actually I did discover some very surprising interaction with closures, caused by #108655 / #111836. Consider this: #![allow(improper_ctypes_definitions)]
#![feature(target_feature_11)]
use std::mem::transmute;
#[cfg(target_arch = "x86")]
use std::arch::x86::*;
#[cfg(target_arch = "x86_64")]
use std::arch::x86_64::*;
#[inline(never)]
#[target_feature(enable = "avx")]
unsafe extern "C" fn with_tf_c(_dummy: f32, x: __m256) {
let val = unsafe { transmute::<_, [u32; 8]>(x) };
dbg!(val);
}
#[target_feature(enable = "avx")]
unsafe fn with_tf() -> impl FnOnce(f32, __m256) {
#[inline(always)]
|dummy, x| with_tf_c(dummy, x)
}
fn main() {
// This makes all the following target feature stuff sound.
assert!(is_x86_feature_detected!("avx"));
unsafe {
with_tf()(0.0, transmute([1; 8]));
}
} Turns out that the The lint about vector ABI issues does trigger in the above example (I should add that as a testcase). I currently can't think of any other way this could break... but it does seem quite surprising. Should we at least warn against |
Looking at the logic here, just stabilizing target_feature_11 will suddenly change the behavior of all closures defined in target_feature functions: they will now start inheriting feature gates, except if they are marked inline(always). Is that truly what we want? It seems like a potentially surprising flag day... It should be sound (apart from possible ABI issues) because for the closure to be constructed, the function containing it must have been called, which means the target feature must be available, and target features can never become un-available. Still, it should be called out loudly in the stabilization report so t-lang is aware of this when we do the FCP. EDIT: t-lang discussed this in #73631, but it doesn't seem to discuss that we are changing the behavior of existing code the moment we are stabilizing this feature. It's probably fine, but should at least be mentioned. |
A little context/summary here: the target feature attribute is only being dropped in codegen, at the language level it is present and it should still be accounted for when doing safety checks etc. The assumption made is that if you want |
A possible alternative is to drop |
The end result of all that is confusing though - e.g. if inside the closure you call a tf 1.1 fn that needs a particular feature, that will not work (I assume). If you call an extern C fn that takes a vector, it will trigger the ABI mismatch lint. Hence the suggestion to inform the programmer that their inline(always) has some quite surprising consequences.
|
It does work, because the target feature attribute is still present everywhere except codegen/LLVM: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=987b48c66d3ef5d7a0dcdf8f716d976c You're right that the ABI mismatch lint was never accounted for in this solution, though. I think a lint is reasonable, hopefully across an edition change we can simply prevent the closure from being called in one of those edge cases and reenable the codegen attribute. |
Also, the target feature inheritance itself is done in an odd way: just enabling the
This example doesn't show anything since the closure is not
I don't think we should do such checks on how the closure is called, that's too complicated and confusing. We should instead ensure our backend doesn't have critical soundness bugs like #116573 that we have to work around on the language level... forbidding |
Not having the target feature in the closure can also be pretty bad for performance, can't it? Presumably people enabled the target feature for a reason. I think we should have a lint that suggests people drop the |
Prior to I support adding the lint, as long as you are still permitted to use |
That seems odd to me; we don't allow this for regular functions either. Do you have a concrete usecase where this is relevant? I don't think we should just go based on "vibes" here. We'd have a future-compat lint for a while, and if nobody shows up with a usecase, IMO we should make it a hard error. |
Oh, I'm fine with that plan as well, it's the most consistent. #108655 suggested to me that there were users who wanted it, I don't personally have an example. However it does seem there's a PR coming to allow |
However it does seem there's a PR coming to allow #[inline(always)] on regular target feature functions by checking that there isn't a target feature mismatch at the call site, I'm not sure how successful that will be.
I heard there are such plans but IMO they should be blocked on fixing LLVM. Since I posted the link to the soundness issue in that Zulip thread, there has not been further discussion there AFAIK. But anyway that is a separate discussion.
|
I've written some code which I think would trigger the future-compat lint you're proposing (obviously I can't try it out yet). I think it's reasonable code:
There are ways I could rework my code to side-step the lint, but all I can think of make the code more ugly and brittle. And I don't really see a reason why I should have to do this. As I understand it, there's no problem with my code either under the old (closure doesn't inherit target_feature) or proposed new (does inherit) semantics. I appreciate that passing such a closure outside of the function it's defined in causes problems when combined with target_feature inheritance and LLVM's current behavior. But I don't think a future-compat warning and eventually hard error on perfectly good code is a nice way to handle that. |
@hanna-kruppe thanks! Yes that would trigger the lint.
Yes, but that's just incompatible with per-function target features, until LLVM fixes that bug. So the question is, what's more important -- having the target features in the closure, or having Would be interesting whether inheriting target features and regular
To be clear, the new semantics is that your closure still does not inherit target features, because it is |
In code like this, I mostly treat closures and non- Of course there are scenarios where I don't want or need everything to be inlined, e.g. because it's a big chunk of code that gets called from several different places. But in those cases I'm not slapping |
I mean we could also say that we'll "just" document that target_features is inherited into closures except for |
Note that the current behaviour is somewhat more surprising than that: it is inherited in inline(always) closures for purposes of safety checking, but not for codegen... |
What difference does not inheriting make, anyway?
|
If we do inherit target features into |
I hadn't seen that issue before and got confused trying to match what you're saying to the original reproducer in that issue. Are you saying
|
Yes that is exactly what I am saying. Specifically the example I posted above would be exactly that gadget, and I am fairly sure it would miscompile. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This looks like it would trigger it. While not directly related to this topic, I see you are passing the closure via |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
It's a work-around for an LLVM bug. In an ideal world we wouldn't have to worry about this.
Strictly speaking this doesn't make sense either. LLVM "just" has to ensure that the operations from The status quo is that:
It's not pretty, but between that long-standing LLVM bug and the general LLVM limitation around inlining and target features, and given our backwards compatibility constraints, it is unclear how to do better. Compared to stable, with TF 1.1 nothing changes for the behavior of |
This is a tracking issue for the RFC "target_feature 1.1" (rust-lang/rfcs#2396).
Issues: F-target_feature_11target feature 1.1 RFC
People
Last updated in Mar 2023:
gnzlbg@workingjubilee (person who can help answer tricky questions that arise during implementation)Step
#[target_feature]
1.1 #69274target_feature_11
feature reference#1181#![feature(target_feature_11)]
#108654#[target_feature]
is allowed onmain
#108645#[target_feature]
is allowed on default implementations #108646target_feature_11
rejects code that was previously accepted #108655extern "C"
ABI of SIMD vector types depends on target features (tracking issue forabi_unsupported_vector_types
future-incompatibility lint) #116558neon
aarch64 target feature is unsound: it changes the float ABI #131058Unresolved questions
target_feature_11 allows bypassing safety checks through Fn* traits #72012 -- soundness hole where safe target-feature functions implement theFn
traits.target-feature 1.1: should closures inherit target-feature annotations? #73631 -- behavior of closures and target-featuresThe text was updated successfully, but these errors were encountered: