Treat safe target_feature functions as unsafe by default#134317
Treat safe target_feature functions as unsafe by default#134317oli-obk wants to merge 1 commit intorust-lang:masterfrom
Conversation
rustbot
added
A-rustdoc-json
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval |
| if b_hdr.safety == hir::Safety::Safe | ||
| && !self.tcx.codegen_fn_attrs(def_id).target_features.is_empty() | ||
| if b_hdr.safety.is_safe() | ||
| && matches!(a_sig.safety(), hir::Safety::Unsafe { target_feature: true }) |
There was a problem hiding this comment.
As mentioned in #134090 (comment), we might want to make this actually safe if the function this happens in has the required features.
We probably shouldn't change it now, but we might want to modify the comment/FIXME accordingly
There was a problem hiding this comment.
If the features are also set via target information that can't differ between crates linked together?
There was a problem hiding this comment.
Not sure if doing so is supposed to be ok...
Anyway, I guess the point of the comment is that if you can call a function, then you also ought to be able to take a safe fn pointer to it.
There was a problem hiding this comment.
But safe pointers can be passed around to code where it's not safe to use them.
With the change I made (plus encoding the actually used target features) we can in theory start creating safe target feature pointers by keeping the target feature information on the function pointer type.
There was a problem hiding this comment.
target_feature_11 enables this:
#[target_feature(enable = "avx2")]
fn bar() -> fn() {
|| avx2()
}
which is not particularly different from allowing coercion of avx2 to a fn pointer. If you have concerns about this, the stabilization PR is probably the better place to discuss it (the stabilization report mentions why it's OK to do this)
There was a problem hiding this comment.
Oh right, yea that makes sense. You already had to call an unsafe function to get here, and it's not a footgun, because it's global informatiom, not local
| pub fn safe_to_unsafe_fn_ty(self, sig: PolyFnSig<'tcx>) -> Ty<'tcx> { | ||
| assert_eq!(sig.safety(), hir::Safety::Safe); | ||
| Ty::new_fn_ptr(self, sig.map_bound(|sig| ty::FnSig { safety: hir::Safety::Unsafe, ..sig })) | ||
| matches!(sig.safety(), hir::Safety::Safe | hir::Safety::Unsafe { target_feature: true }); |
There was a problem hiding this comment.
Didn't this drop an assertion?
This comment has been minimized.
This comment has been minimized.
oli-obk
added
the
S-waiting-on-author
oli-obk
force-pushed
the
push-kmzkoqkuosko
branch
from
69bacc5 to
2800108
Compare
|
Closed in favor of #134353 |
…-by-default, r=wesleywiser Treat safe target_feature functions as unsafe by default [less invasive variant] This unblocks * rust-lang#134090 As I stated in rust-lang#134090 (comment) I think the previous impl was too easy to get wrong, as by default it treated safe target feature functions as safe and had to add additional checks for when they weren't. Now the logic is inverted. By default they are unsafe and you have to explicitly handle safe target feature functions. This is the less (imo) invasive variant of rust-lang#134317, as it doesn't require changing the Safety enum, so it only affects FnDefs and nothing else, as it should.
…y-default, r=wesleywiser Treat safe target_feature functions as unsafe by default [less invasive variant] This unblocks * rust-lang#134090 As I stated in rust-lang#134090 (comment) I think the previous impl was too easy to get wrong, as by default it treated safe target feature functions as safe and had to add additional checks for when they weren't. Now the logic is inverted. By default they are unsafe and you have to explicitly handle safe target feature functions. This is the less (imo) invasive variant of rust-lang#134317, as it doesn't require changing the Safety enum, so it only affects FnDefs and nothing else, as it should.
…y-default, r=wesleywiser Treat safe target_feature functions as unsafe by default [less invasive variant] This unblocks * rust-lang#134090 As I stated in rust-lang#134090 (comment) I think the previous impl was too easy to get wrong, as by default it treated safe target feature functions as safe and had to add additional checks for when they weren't. Now the logic is inverted. By default they are unsafe and you have to explicitly handle safe target feature functions. This is the less (imo) invasive variant of rust-lang#134317, as it doesn't require changing the Safety enum, so it only affects FnDefs and nothing else, as it should.
5 participants
This unblocks
As I stated in #134090 (comment) I think the previous impl was too easy to get wrong, as by default it treated safe target feature functions as safe and had to add additional checks for when they weren't. Now the logic is inverted. By default they are unsafe and you have to explicitly handle safe target feature functions.
I will make another version of this PR that doesn't modify the safety enum but just checks the attrs. Should be less invasive