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

document that target features are unsafe #63597

Closed
nikomatsakis opened this issue Aug 15, 2019 · 5 comments · Fixed by #64145
Closed

document that target features are unsafe #63597

nikomatsakis opened this issue Aug 15, 2019 · 5 comments · Fixed by #64145
Assignees
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Aug 15, 2019

When discussing #63466 in the compiler team triage meeting, it became clear that target-features ought to be considered generally unsafe. We should document this fact in the -Chelp and in the rustc book. Ideally, we'd also start listing out known gotchas.

For example, in #63466 @nikic noted that:

#63466 (comment)

and @rkruppe had a much more detailed comment.

This issue has been assigned to @togiberlin via this comment.

@nikomatsakis nikomatsakis added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Aug 15, 2019
@nikomatsakis nikomatsakis added P-medium Medium priority and removed I-nominated labels Aug 29, 2019
@nikomatsakis
Copy link
Contributor Author

Compiler triage meeting:

Marking as P-medium

@togiberlin
Copy link
Contributor

@rustbot claim

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 13, 2019

The reference section about Behavior considered undefined mentions this:

Executing code compiled with platform features that the current platform does not support (see target_feature).

The link to the target_feature section of the reference mentions this as well:

It is undefined behavior to call a function that is compiled with a feature that is not supported on the current platform the code is running on.

So maybe the -C help could point people to the reference?

@hanna-kruppe
Copy link
Contributor

That is one way in which target_feature is unsafe, but it's not the one which was the subject of #63466 so just pointing at the reference as it is is insufficient. Plus, it's also more about -C target-feature than #[target_feature] (where the compiler will hit you in the face with unsafe). So the rustc book is a better place for it than the reference IMO.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 14, 2019

I think that for users to better properly understand the UB involved, it might be more educational to say that -C target-feature=+X stamps #[target_feature(enable = "X")] on all functions, but with the subtle difference that it doesn't make them unsafe fn, so no unsafe-check happens when this triggers UB.

The reference does not document all current perils of using #[target_feature(enable = "X")]. In particular, it does not document that these can change the function call ABI, and that rustc neither checks that nor handles it correctly - we just currently assume people don't do that.

Independently of whether we document that in the rustc book as well or not, that bit should also probably be documented in the reference, because it is easy to trigger this. On stable Rust, I think one can indeed only trigger this via -C target-feature and the reference should say so, but on nightly one can definitely trigger this with bare #[target_feature(enable = "...")] within a single Rust crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants