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

Deprecate vfork #1574

Merged
merged 1 commit into from Nov 18, 2019

Conversation

@Amanieu
Copy link
Contributor

Amanieu commented Oct 29, 2019

The compiler may generate incorrect code for vfork and setjmp because they are missing the #[returns_twice] attribute which is currently unstable (tracking issue). Since vfork is impossible to use safely, I propose deprecating it until #[returns_twice] is stable.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Oct 29, 2019

r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Oct 29, 2019

@Amanieu thank you for filling this PR.

Right now, because vfork is lacking the returns_twice attribute, the compiler might mis-compile user code in some cases. Do we have any reported mis-compilations (or MWEs) due to this that illustrate the issue?

Claiming that this feature is impossible to use correctly won't do much for users that are already using vfork, because if it works for them, then in their eyes, it can be used correctly, and also, there is no really an alternative available, so they will just turn the warning off.

So what do we want to achieve ?

I think the best we can do right now is document any known mis-compilations by filling an issue, and then use the deprecation warning to inform users that such known mis-compilations exist, and that there is a nightly feature that they can use to avoid them, but that they are kind of on their own for now.

Just telling them "don't use this" without telling them "what to use instead" produces a very frustrating user experience. If this is really always impossible to use correctly, we should at least point them to an issue that explains that in detail.

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Oct 29, 2019

I haven't managed to create a miscompiling example, but essentially, if the compiler decides to perform tail call optimization in the vfork child, this will corrupt the entire stack frame for the parent as well.

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Oct 29, 2019

Alternatively, the compiler could decide to share a single stack slot for 2 variables, one in the parent and one in the child since it "knows" that only one of the two branches is taken, not both.

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Oct 30, 2019

I think maybe @comex had an actual mis compilation example for a lack of returns_twice attribute but I can find it. There is an issue open in the rfc repo for longjmp, maybe we could link there ? I'd prefer to avoid using the term "impossible", but I think the deprecation warning can be a bit longer and strongly suggest not to use this API and being clear that it is UB and that any new toolchain upgrade could break existing "working" code in subtle ways silently so using it is very dangerous.

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Nov 7, 2019

I've managed to create a miscompilation example: https://play.rust-lang.org/?version=nightly&mode=release&edition=2018&gist=e192ab81e8d408fa9984f766e2356697

As you can see, variables modified in the vfork child cause unrelated variables in the parent to be modified.

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Nov 7, 2019

However if you add #[ffi_returns_twice] then the issue no longer appears: https://play.rust-lang.org/?version=nightly&mode=release&edition=2018&gist=0de019e56b69aa048f3e0f5319cc187e

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Nov 12, 2019

@gnzlbg ping?

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Nov 13, 2019

This looks good to me @Amanieu. CI is red due to unrelated issues, but I'll merge it ASAP.

In the mean time, could you create an issue in this repo explaining the problem with those links, and add a link to it to the deprecation message ?

@Amanieu Amanieu force-pushed the Amanieu:deprecate-vfork branch from 47283af to a29bc51 Nov 17, 2019
@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Nov 17, 2019

Added a reference to #1596 in the deprecation message.

@Amanieu Amanieu force-pushed the Amanieu:deprecate-vfork branch from a29bc51 to 1f7352c Nov 17, 2019
@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Nov 17, 2019

Thanks, as soon as we get up bors working again for the repo I'll start merging PRs.

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Nov 18, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 18, 2019

📌 Commit 1f7352c has been approved by gnzlbg

bors added a commit that referenced this pull request Nov 18, 2019
Deprecate vfork

The compiler may generate incorrect code for `vfork` and `setjmp` because they are missing the `#[returns_twice]` attribute which is currently unstable ([tracking issue](rust-lang/rust#58314)). Since `vfork` is impossible to use safely, I propose deprecating it until `#[returns_twice]` is stable.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 18, 2019

⌛️ Testing commit 1f7352c with merge a13ad69...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 18, 2019

☀️ Test successful - checks-cirrus-freebsd-10, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, status-azure
Approved by: gnzlbg
Pushing a13ad69 to master...

@bors bors merged commit 1f7352c into rust-lang:master Nov 18, 2019
5 of 6 checks passed
5 of 6 checks passed
rust-lang.libc #20191117.2 failed
Details
homu Test successful
Details
nightly x86_64-unknown-freebsd-10 Task Summary
Details
nightly x86_64-unknown-freebsd-12 Task Summary
Details
rust-lang.libc (1) #20191117.2 succeeded
Details
stable x86_64-unknown-freebsd-11 Task Summary
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.