Skip to content
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

calling functions with implicit-enabled target features is unsafe #19154

Open
usamoi opened this issue Feb 13, 2025 · 8 comments
Open

calling functions with implicit-enabled target features is unsafe #19154

usamoi opened this issue Feb 13, 2025 · 8 comments
Labels
C-bug Category: bug

Comments

@usamoi
Copy link
Contributor

usamoi commented Feb 13, 2025

rust-analyzer version: 0.4.2302-standalone
rustc version: rustc 1.86.0-nightly (ef148cd7e 2025-02-12)
code snippet to reproduce:

#![feature(target_feature_11)]

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

#[target_feature(enable = "avx2")]
fn g() {
    f();
    // it's safe, because enabling `avx2` enables `avx` implicitly, but ra reports that it's unsafe
    // https://github.com/rust-lang/reference/pull/1720 describe it, and nightly rustc agrees with it
}

This should be caused by

// RFC 2396 <https://rust-lang.github.io/rfcs/2396-target-feature-1.1.html>.
let callee_target_features = TargetFeatures::from_attrs(&db.attrs(func.into()));
if !caller_target_features.enabled.is_superset(&callee_target_features.enabled) {
return Unsafety::Unsafe;
}

It should calculate the set of all explicit-enabled and implicit-enabled target features of caller and calee, and then check if's a subset.

The "target feature to implicit-enabled target features" relationship is recorded in https://github.com/rust-lang/rust/blob/master/compiler/rustc_target/src/target_features.rs. (How to get it in ra?)

@usamoi usamoi added the C-bug Category: bug label Feb 13, 2025
@Veykril
Copy link
Member

Veykril commented Feb 13, 2025

cc @ChayimFriedman2

The "target feature to implicit-enabled target features" relationship is recorded in https://github.com/rust-lang/rust/blob/master/compiler/rustc_target/src/target_features.rs. (How to get it in ra?)

We can't depend on rustc_target unfortunately (due to requiring nightly and also depending on rustc_span, both of which I tried to gate away rust-lang/rust@master...Veykril:rust:veykril/rustc_target-rust-analyzer but gave up at some point)

@ChayimFriedman2
Copy link
Contributor

I could swear I checked that. Weird...

Anyway, I'm not sure it will be a good idea to embed a list of target feature implications into rust-analyzer. I will try again later to make it compiler on stable (what will be the process for that? Create a compiler team MCP?) and if I'll fail, we'll see what to do.

@Veykril
Copy link
Member

Veykril commented Feb 13, 2025

(what will be the process for that? Create a compiler team MCP?)

Probably ask t-compiler / MCP yes.

The main reason I dropped it was that I only wanted to look briefly into it (because of the assembly things) but due to the pervasive use of symbols I kind of gave up as making all of those generic (or outright replace the symbol use with string literals) seemed like too much to propose.

@ChayimFriedman2
Copy link
Contributor

ChayimFriedman2 commented Feb 13, 2025

What I imagine is a sym!(symbol) macro that expands to either sym::symbol or to "symbol".

@Veykril
Copy link
Member

Veykril commented Feb 14, 2025

That might be a way forward. We should open a zulip topic for a vibe check on this ideally though, you wanna do that or should I?

@ChayimFriedman2
Copy link
Contributor

Isn't that the job of the MCP?

@Veykril
Copy link
Member

Veykril commented Feb 15, 2025

Well I guess? If you already have a concrete design in mind an MCP is probably the way to go then

@ChayimFriedman2
Copy link
Contributor

MCP submitted: rust-lang/compiler-team#839.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

3 participants