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

Don't treat rustc exit on signal as internal error. #6270

Merged
merged 3 commits into from
Nov 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@ use std::io::{self, Write};
use std::path::{self, Path, PathBuf};
use std::sync::Arc;

use failure::Error;
use same_file::is_same_file;
use serde_json;

use core::manifest::TargetSourcePath;
use core::profiles::{Lto, Profile};
use core::{PackageId, Target};
use util::errors::{CargoResult, CargoResultExt, Internal};
use util::errors::{CargoResult, CargoResultExt, Internal, ProcessError};
use util::paths;
use util::{self, machine_message, Freshness, ProcessBuilder, process};
use util::{internal, join_paths, profile};
Expand Down Expand Up @@ -275,6 +276,19 @@ fn rustc<'a, 'cfg>(
}
}

fn internal_if_simple_exit_code(err: Error) -> Error {
// If a signal on unix (code == None) or an abnormal termination
// on Windows (codes like 0xC0000409), don't hide the error details.
match err
.downcast_ref::<ProcessError>()
.as_ref()
.and_then(|perr| perr.exit.and_then(|e| e.code()))
Copy link
Member

Choose a reason for hiding this comment

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

I think rustc only has one erroneous exit code itself, so could we perhaps whitelist just that one exit code to handle cases like Windows to ensure that Windows also prints segfaults by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think it's safe. It looks like rustdoc uses the same code, I'm just thinking of wrappers using different codes. But maybe that's a good thing?

Copy link
Member

Choose a reason for hiding this comment

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

An alternative is to filter out any exit code less than 128. On Unix I think that's the maximal error code and any portable program won't be using upper codes on Windows. (and on Windows all the abnormal exit codes are far above 128).

Do you think we should perhaps implement that sort of filtering to be a bit more conservative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, also means I don't need to fix the 1.28 failure.

{
Some(n) if n < 128 => Internal::new(err).into(),
_ => err,
}
}

state.running(&rustc);
if json_messages {
exec.exec_json(
Expand All @@ -284,13 +298,14 @@ fn rustc<'a, 'cfg>(
mode,
&mut assert_is_empty,
&mut |line| json_stderr(line, &package_id, &target),
).map_err(Internal::new)
)
.map_err(internal_if_simple_exit_code)
.chain_err(|| format!("Could not compile `{}`.", name))?;
} else if build_plan {
state.build_plan(buildkey, rustc.clone(), outputs.clone());
} else {
exec.exec_and_capture_output(rustc, &package_id, &target, mode, state)
.map_err(Internal::new)
.map_err(internal_if_simple_exit_code)
.chain_err(|| format!("Could not compile `{}`.", name))?;
}

Expand Down
62 changes: 62 additions & 0 deletions tests/testsuite/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4373,3 +4373,65 @@ fn target_filters_workspace_not_found() {
.with_stderr("[ERROR] no library targets found in packages: a, b")
.run();
}

#[cfg(unix)]
Copy link
Member

Choose a reason for hiding this comment

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

Despite this being unix only I personally like having tests like this, so I'd definitely keep it

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this cause an abnormal exit on all systems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I just didn't want to deal with figuring out the text of the error message on every platform. Windows is something like "3221226505", but is it like that on all versions of windows? (probably?) What about non-tier-1 platforms? I'm already concerned this test might be flaky on weird platforms.

Copy link
Member

Choose a reason for hiding this comment

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

For Windows error codes are actually best rendered as hex -- 0xC0000409 instead of 3221226505 -- and I've meant to do that in libstd for some time now...

#[test]
fn signal_display() {
// Cause the compiler to crash with a signal.
let foo = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"
[dependencies]
pm = { path = "pm" }
"#,
)
.file(
"src/lib.rs",
r#"
#[macro_use]
extern crate pm;

#[derive(Foo)]
pub struct S;
"#,
)
.file(
"pm/Cargo.toml",
r#"
[package]
name = "pm"
version = "0.1.0"
[lib]
proc-macro = true
"#,
)
.file(
"pm/src/lib.rs",
r#"
extern crate proc_macro;
use proc_macro::TokenStream;

#[proc_macro_derive(Foo)]
pub fn derive(_input: TokenStream) -> TokenStream {
std::process::abort()
}
"#,
)
.build();

foo.cargo("build")
.with_stderr("\
[COMPILING] pm [..]
[COMPILING] foo [..]
[ERROR] Could not compile `foo`.

Caused by:
process didn't exit successfully: `rustc [..]` (signal: 6, SIGABRT: process abort signal)
")
.with_status(101)
.run();
}