Treat safe target_feature functions as unsafe by default #134317
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
| @@ -2521,8 +2521,14 @@ impl<'tcx> TyCtxt<'tcx> { | |||
| /// that is, a `fn` type that is equivalent in every way for being | |||
| /// unsafe. | |||
| 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?
|
The job Click to see the possible cause of the failure (guessed by this bot) |
oli-obk
added
the
S-waiting-on-author
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