Skip to content

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
Contributor

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
#[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
Collaborator

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