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

rustbuild: fix bad usage of UNIX exec() in rustc wrapper #74803

Merged
merged 2 commits into from
Jul 27, 2020

Conversation

infinity0
Copy link
Contributor

exec never returns, it replaces the current process. so anything after it is unreachable. that's not how exec_cmd() is used in the surrounding code

We use --on-fail env on Debian. env always returns exit code 0. This means that the rustc bootstrap wrapper always returns exit code 0 even when it fails. However, the crossbeam-utils build process (due to autocfg) relies on rustc returning error exit codes when detecting CPU features, and ends up writing cargo:rustc-cfg=has_atomic_u128 even when it's not detected, because the rustc wrapper is always giving exit code 0.

(This separately is causing our builds to try to compile rustc 40+ times, due to #74801.)

exec never returns, it replaces the current process. so anything after it is
unreachable. that's not how exec_cmd() is used in the surrounding code
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2020
Err(cmd.exec())
}

#[cfg(not(unix))]
fn exec_cmd(cmd: &mut Command) -> io::Result<i32> {
cmd.status().map(|status| status.code().unwrap())
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably not call this exec_cmd anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed in b99668b

@infinity0
Copy link
Contributor Author

@nagisa Bug has existed since your b3b2f1b (Dec 2016) - the very API of returning from a function that calls exec() is buggy, however this commit would never exhibit it due to it only calling the real rustc & passing on the exit code. The bug was later properly exposed in 0e45a5e and we haven't noticed it until now.

@nagisa
Copy link
Member

nagisa commented Jul 27, 2020

Wait, --on-fail still works at all? I thought it was entirely broken… (though perhaps only affecting --on-fail bash usage?)

I think the motivation from b3b2f1b still stands for the proper rustc invocations, though, and only --on-fail is really broken. If that’s true, I’d adjust the --on-fail path only.

@infinity0
Copy link
Contributor Author

@nagisa I don't think the original motivation applies any more, and it's not used for that anyway - we intercept the status code and if there was a failure print the command line invocation. This would be impossible with exec.

@nagisa
Copy link
Member

nagisa commented Jul 27, 2020

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 27, 2020

📌 Commit b99668b has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 27, 2020
…arth

Rollup of 4 pull requests

Successful merges:

 - rust-lang#73858 (Make more primitive integer methods const)
 - rust-lang#74487 (Forbid generic parameters in anon consts inside of type defaults)
 - rust-lang#74803 (rustbuild: fix bad usage of UNIX exec() in rustc wrapper)
 - rust-lang#74822 (More ensure stack to avoid segfault with increased `recursion_limit`)

Failed merges:

r? @ghost
@nikomatsakis
Copy link
Contributor

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned nikomatsakis Jul 27, 2020
@bors bors merged commit c9cdc87 into rust-lang:master Jul 27, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants