Stabilize #![feature(target_feature_11)]

[LeSeulArtichaut]

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.

// 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/.

Edge cases

• target-feature 1.1: should closures inherit target-feature annotations? #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.

#[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: Document the target_feature_11 feature reference#1181

---

cc tracking issue #69098
r? @ghost

[LeSeulArtichaut]

@rustbot label T-lang needs-fcp

[joshtriplett]

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Aaron1011
• @cjgillot
• @cramertj
• @davidtwco
• @eddyb
• @estebank
• @joshtriplett
• @lcnr
• @matthewjasper
• @michaelwoerister
• @nagisa
• @nikomatsakis
• @oli-obk
• @petrochenkov
• @pnkfelix
• @scottmcm
• @wesleywiser

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[LeSeulArtichaut]

Oops, I should probably have removed the T-compiler label, sorry for the inconvenience

[oli-obk]

@rfcbot cancel

[rfcbot]

@oli-obk proposal cancelled.

[LeSeulArtichaut]

Tried to write up a stabilization report, feedback would be appreciated.

[lcnr]

how does this interact with type alias impl trait

#![feature(target_feature_11, type_alias_impl_trait)]

type Foo = impl FnOnce() + Default;

#[target_feature(enable = "avx2")]
fn avx2() {}

#[target_feature(enable = "avx2")]
fn qux() {
    let x: Foo = || avx2(); // currently errors cause the closure isn't `Default`.
}

in #77688 it was decided against adding Default for function types and closures but that decision wasn't "we will never add that impl in the future" so I do want something apart from closures aren't Default from preventing the above example to work

[LeSeulArtichaut]

@lcnr I think this is analogous to something like this?

unsafe fn unsf() {}

// This has some safety requirements...
unsafe fn foo() {
    // ...which might be used in the closure here
    let x: Foo = || unsf();
}

I think calling x through Foo::default() would be unsound?

Or something like

fn bar() {
    // check that some invariants are upheld before doing some unsafe
    if !invariant_upheld { return; }

    let x: Foo = || unsafe { unsf(); };
}

Wouldn't it be surprising to be able to call x through Default?

This was raised in #77688 (comment) but I couldn't find a proposed solution/workaround. If there is one, would it be applicable here as well?

[lcnr]

While || unsafe { some_unsafe_fn() } is now a safe function pointer which we could leak this way, we still need unsafe somewhere to skip that boundary.

With target features the skip can be fully implicit (except for a #[target_feature(...)] attribute on the function constraining the opaque type).

Wouldn't it be surprising to be able to call x through Default?

as Foo would need an explicit Default bound, I think no ^^

I realized another issue with implementing Default for closures automatically. In the future, we may uncover that an opaque type implements Default using specialization, changing implicit impls of Default to change currently sound APIs to be unsound, making this a breaking change.

I guess I am fine with stabilizing this then ^^ while it seems like a final nail in the coffin of Default for closures and function definitions, there are already a few other nails.

[est31]

👋 Hello, I'm writing this comment in this stabilization PR to notify you, the authors of this PR, that #100591 has been merged, which implemented a change in how features are stabilized.

Your PR has been filed before the change, so will likely require modifications in order to comply with the new rules. I recommend you to:

1. rebase the PR onto latest master, so that uses of the placeholder are possible.
2. replace the version numbers in the PR with the placeholder CURRENT_RUSTC_VERSION. For language changes, this means the version numbers in accepted.rs (example: 4caedba). For library changes, this means the since fields (example e576a9b).

That's it! The CURRENT_RUSTC_VERSION placeholder will, as part of the release process, be replaced with the version number that the PR merged for. It can be used anywhere in rust-lang/rust, not just accepted.rs and the since fields.

If you have any questions, feel free to drop by the zulip stream, or ping me directly in this PR's thread. Thanks! 👋

[LeSeulArtichaut]

Done! Thank you very much for the heads-up @est31

[estebank]

r=me

[Noratrieb]

The comment was resolved and @estebank approved it, so
@bors r=estebank

[bors]

📌 Commit b379d21 has been approved by estebank

It is now in the queue for this repository.

[workingjubilee]

...Hm. I was not aware this had been FCP'd. We are aware that this makes downgrading feature levels a backwards incompatible change for targets, right?

[bors]

⌛ Testing commit b379d21 with merge b583ede...

[bors]

☀️ Test successful - checks-actions
Approved by: estebank
Pushing b583ede to master...

[est31]

@workingjubilee

this makes downgrading feature levels a backwards incompatible change for targets

You mean when proposals like this get accepted, the "default" set of default-enabled target features shrinks and some code calling #[target_feature] enabled attributes that were before part of the default set starts to fail compiling?

From testing the implementation, that seems to have been avoided by not having a default set of target features. Even if the target feature is enabled by default, code still must use unsafe. I wasn't involved much during implementation of this RFC so I can't comment on how intentional that was, but I vaguely remember a comment that they intentionally made the most restricted version, where there is also no implication between features that came after another, e.g. sse2 -> sse.

Take this code (playground):

#![feature(target_feature_11)]

#[target_feature(enable = "sse")] fn foo() {}

#[target_feature(enable = "sse")] fn bar() { foo(); }

fn main() {
    #[cfg(target_feature = "sse")]
    bar(); //~ERROR call to function with `#[target_feature]` is unsafe and requires unsafe function or block
}

It will print the error as indicated. The test is written in a way that shows that sse is enabled by default (as is required on AMD 64 bit), and if it weren't, then it wouldn't error as the cfg would not expand. But it still errors even though sse is enabled.

Also note that downgrading of target requirements are breaking changes already, for example you could have code like:

extern { fn fancy_fn_that_uses_sse(); }

fn main() {
    #[cfg(target_feature = "sse")]
    unsafe { fancy_fn_that_uses_sse(); }

    #[cfg(not(target_feature = "sse"))]
    compile_error!("sorry but we only have an sse using implementation of this!");
}

It doesn't use target_feature_11 at all, but it would fail to compile upon a removal of sse from the default set of enabled target features. Edit: technically, increasing requirements is also a breaking change, which has been done over the years every once in a while.

In these scenarios I also think that compiler errors are preferable to runtime SIGILLs or worse.

One could think about extending the target_feature feature to also take the default-enabled target features into account, but that should be discussed in other threads, not here.

[rust-timer]

Finished benchmarking commit (b583ede): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.0%	[3.0%, 3.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[3.0%, 3.0%]	1

Cycles

This benchmark run did not return any relevant results for this metric.