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

Target-feature documented as unsafe #64145

Open
wants to merge 1 commit into
base: master
from

Conversation

@togiberlin
Copy link

commented Sep 4, 2019

@nikomatsakis asked me to help out on the docs on this issue: #63597

The following docs have been modified

  • rustc -C help text for target-feature
  • RustC book:

Preview of src/doc/rustc/src/targets/index.md

Screenshot 2019-09-17 at 12 22 45

Preview of src/doc/rustc/src/targets/known-issues.md

Screenshot 2019-09-17 at 12 22 25

Fixes #63597

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

Hey @nikic or @rkruppe -- any comments on the text to be added to the rustc-guide? It looks like it was largely cribbed from @rkruppe's comment here, which seems like a good source.

Thanks @togiberlin for pulling this together!

@rkruppe
Copy link
Member

left a comment

Thanks for taking a stab at this! I have some suggestions and one correction.

src/doc/rustc/src/targets/index.md Outdated Show resolved Hide resolved
src/doc/rustc/src/targets/known-issues.md Outdated Show resolved Hide resolved
src/doc/rustc/src/targets/known-issues.md Outdated Show resolved Hide resolved

@togiberlin togiberlin force-pushed the togiberlin:feature/target-features-doc branch 2 times, most recently from aa92e72 to 54aadaf Sep 8, 2019

@togiberlin

This comment has been minimized.

Copy link
Author

commented Sep 8, 2019

Hello @rkruppe ,
thx for your detailed feedback :)
I rewrote some parts of my text.

Could you please check it again?
Thanks!

src/doc/rustc/src/codegen-options/index.md Outdated Show resolved Hide resolved
src/doc/rustc/src/command-line-arguments.md Outdated Show resolved Hide resolved
# Known Issues
This section informs you about known "gotchas". Keep in mind, that this section is (and always will be) incomplete. For suggestions and amendments, feel free to [contribute](https://doc.rust-lang.org/rustc/contributing.html) to this guide.

## Target Features

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 13, 2019

Contributor

Why isn't this in the target_feature section? (i.e. why do we need a new section for this?).

This comment has been minimized.

Copy link
@togiberlin

togiberlin Sep 17, 2019

Author

As far as I understood, there are waay more issues, which are not documented yet.
Niko asked me to create a dedicated page, where all issues are listed.
The benefit is, that this vital info is not scattered/fragmented across many pages.

I am always open to suggestions :)

src/librustc/session/config.rs Outdated Show resolved Hide resolved

By default, compiling your code with the `-C target-feature` flag will not recompile the entire standard library and/or imported crates with matching target features. Therefore, target features are generally considered as unsafe. Using `#[target_feature]` on individual functions makes the function unsafe.

Examples:

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 13, 2019

Contributor

I'm not sure how useful this would be. I don't think it is practical for us to document all features on all architectures that could cause problems when combined in different ways, and would very much prefer a solution to the problem that does not rely on documentation.

Right now Rust does not know much about how target-features interact with call ABI. It probably should.

This comment has been minimized.

Copy link
@rkruppe

rkruppe Sep 13, 2019

Member

You're invited to wish for a better solution but we're not even generating shims for the more specific sub-problem of vector values and we've been suffering from that problem for the several years. Documenting that it is de facto unsafe now and underlining that with some scary examples is a good step forward. I agree that an exhaustive list is not practical but that's not necessary.

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 17, 2019

Contributor

@eddyb a quick workaround for this form of UB would be to pass f32 and f64 by memory in the Rust ABI, just like we do for Vector types.

This would be a temporary solution, until we can either diagnose these issues or generate appropriate shims. It might be horrible for performance of numeric code, I don't know.

@rkruppe

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Sorry for taking a while to get back to this, @togiberlin! It seems to be all correct now and largely in good shape. There's some minor editorial suggestions I could make but it seems like @gnzlbg is already on that.

@gnzlbg
gnzlbg approved these changes Sep 17, 2019
Copy link
Contributor

left a comment

I'm not sure it is a good idea to split all the knowledge required to use target-feature correctly across multiple documents, but I suppose we can copy paste what gets documented here into a central document and try to keep those in sync.

@togiberlin togiberlin force-pushed the togiberlin:feature/target-features-doc branch from 936a038 to 536e650 Sep 17, 2019

@togiberlin togiberlin force-pushed the togiberlin:feature/target-features-doc branch from 536e650 to a0c56cb Sep 17, 2019

@togiberlin

This comment has been minimized.

Copy link
Author

commented Sep 17, 2019

@gnzlbg @rkruppe Thank you very much for your helpful input! :)
Do you think this PR is ready?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.