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

rustdoc: set panic output before starting compiler thread pool #52181

Merged
merged 8 commits into from
Jul 24, 2018
22 changes: 17 additions & 5 deletions src/librustc_driver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1497,10 +1497,12 @@ fn parse_crate_attrs<'a>(sess: &'a Session, input: &Input) -> PResult<'a, Vec<as
}
}

/// Runs `f` in a suitable thread for running `rustc`; returns a
/// `Result` with either the return value of `f` or -- if a panic
/// occurs -- the panic value.
pub fn in_rustc_thread<F, R>(f: F) -> Result<R, Box<dyn Any + Send>>
/// Runs `f` in a suitable thread for running `rustc`; returns a `Result` with either the return
/// value of `f` or -- if a panic occurs -- the panic value.
///
/// This version applies the given name to the thread. This is used by rustdoc to ensure consistent
/// doctest output across platforms and executions.
pub fn in_named_rustc_thread<F, R>(name: String, f: F) -> Result<R, Box<dyn Any + Send>>
where F: FnOnce() -> R + Send + 'static,
R: Send + 'static,
{
Expand Down Expand Up @@ -1564,7 +1566,7 @@ pub fn in_rustc_thread<F, R>(f: F) -> Result<R, Box<dyn Any + Send>>

// The or condition is added from backward compatibility.
if spawn_thread || env::var_os("RUST_MIN_STACK").is_some() {
let mut cfg = thread::Builder::new().name("rustc".to_string());
let mut cfg = thread::Builder::new().name(name);

// FIXME: Hacks on hacks. If the env is trying to override the stack size
// then *don't* set it explicitly.
Expand All @@ -1580,6 +1582,16 @@ pub fn in_rustc_thread<F, R>(f: F) -> Result<R, Box<dyn Any + Send>>
}
}

/// Runs `f` in a suitable thread for running `rustc`; returns a
/// `Result` with either the return value of `f` or -- if a panic
/// occurs -- the panic value.
pub fn in_rustc_thread<F, R>(f: F) -> Result<R, Box<dyn Any + Send>>
where F: FnOnce() -> R + Send + 'static,
R: Send + 'static,
{
in_named_rustc_thread("rustc".to_string(), f)
}

/// Get a list of extra command-line flags provided by the user, as strings.
///
/// This function is used during ICEs to show more information useful for
Expand Down
92 changes: 47 additions & 45 deletions src/librustdoc/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,40 +232,42 @@ fn run_test(test: &str, cratename: &str, filename: &FileName, line: usize,
..config::basic_options().clone()
};

let (libdir, outdir) = driver::spawn_thread_pool(sessopts, |sessopts| {
// Shuffle around a few input and output handles here. We're going to pass
// an explicit handle into rustc to collect output messages, but we also
// want to catch the error message that rustc prints when it fails.
//
// We take our thread-local stderr (likely set by the test runner) and replace
// it with a sink that is also passed to rustc itself. When this function
// returns the output of the sink is copied onto the output of our own thread.
//
// The basic idea is to not use a default Handler for rustc, and then also
// not print things by default to the actual stderr.
struct Sink(Arc<Mutex<Vec<u8>>>);
impl Write for Sink {
fn write(&mut self, data: &[u8]) -> io::Result<usize> {
Write::write(&mut *self.0.lock().unwrap(), data)
}
fn flush(&mut self) -> io::Result<()> { Ok(()) }
// Shuffle around a few input and output handles here. We're going to pass
// an explicit handle into rustc to collect output messages, but we also
// want to catch the error message that rustc prints when it fails.
//
// We take our thread-local stderr (likely set by the test runner) and replace
// it with a sink that is also passed to rustc itself. When this function
// returns the output of the sink is copied onto the output of our own thread.
//
// The basic idea is to not use a default Handler for rustc, and then also
// not print things by default to the actual stderr.
struct Sink(Arc<Mutex<Vec<u8>>>);
impl Write for Sink {
fn write(&mut self, data: &[u8]) -> io::Result<usize> {
Write::write(&mut *self.0.lock().unwrap(), data)
}
struct Bomb(Arc<Mutex<Vec<u8>>>, Box<Write+Send>);
impl Drop for Bomb {
fn drop(&mut self) {
let _ = self.1.write_all(&self.0.lock().unwrap());
}
fn flush(&mut self) -> io::Result<()> { Ok(()) }
}
struct Bomb(Arc<Mutex<Vec<u8>>>, Box<Write+Send>);
impl Drop for Bomb {
fn drop(&mut self) {
let _ = self.1.write_all(&self.0.lock().unwrap());
}
let data = Arc::new(Mutex::new(Vec::new()));
}
let data = Arc::new(Mutex::new(Vec::new()));

let old = io::set_panic(Some(box Sink(data.clone())));
Copy link
Contributor

Choose a reason for hiding this comment

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

While this new arrangement delays the printing of the collected output to a later, more appropriate stage, doesn't this still leave the panic handler eating more data, if there is any? Should the panic handler also be restored? (Or maybe it's otherwise guaranteed to exit before that can happen? Sorry, not familiar with the code here.)

Copy link
Member Author

Choose a reason for hiding this comment

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

My assumption is that because this is what it was doing beforehand, it's not much harm to leave it. Before I had this version, I tried calling set_panic to switch the sink back in this Drop call. It didn't change anything about the output, so I set it back. Each test will set the panic writer to a fresh buffer, so there's no chance of test outputs accumulating.

However, if any panic occurs outside of this function, the output will be swallowed. This may be what we want, but it's something to consider.

let _bomb = Bomb(data.clone(), old.unwrap_or(box io::stdout()));

let (libdir, outdir, compile_result) = driver::spawn_thread_pool(sessopts, |sessopts| {
let codemap = Lrc::new(CodeMap::new_doctest(
sessopts.file_path_mapping(), filename.clone(), line as isize - line_offset as isize
));
let emitter = errors::emitter::EmitterWriter::new(box Sink(data.clone()),
Some(codemap.clone()),
false,
false);
let old = io::set_panic(Some(box Sink(data.clone())));
let _bomb = Bomb(data.clone(), old.unwrap_or(box io::stdout()));

// Compile the code
let diagnostic_handler = errors::Handler::with_emitter(true, false, box emitter);
Expand Down Expand Up @@ -312,28 +314,28 @@ fn run_test(test: &str, cratename: &str, filename: &FileName, line: usize,
Err(_) | Ok(Err(CompileIncomplete::Errored(_))) => Err(())
};

match (compile_result, compile_fail) {
(Ok(()), true) => {
panic!("test compiled while it wasn't supposed to")
}
(Ok(()), false) => {}
(Err(()), true) => {
if error_codes.len() > 0 {
let out = String::from_utf8(data.lock().unwrap().to_vec()).unwrap();
error_codes.retain(|err| !out.contains(err));
}
}
(Err(()), false) => {
panic!("couldn't compile the test")
(libdir, outdir, compile_result)
});

match (compile_result, compile_fail) {
(Ok(()), true) => {
panic!("test compiled while it wasn't supposed to")
}
(Ok(()), false) => {}
(Err(()), true) => {
if error_codes.len() > 0 {
let out = String::from_utf8(data.lock().unwrap().to_vec()).unwrap();
error_codes.retain(|err| !out.contains(err));
}
}

if error_codes.len() > 0 {
panic!("Some expected error codes were not found: {:?}", error_codes);
(Err(()), false) => {
panic!("couldn't compile the test")
}
}

(libdir, outdir)
});
if error_codes.len() > 0 {
panic!("Some expected error codes were not found: {:?}", error_codes);
}

if no_run { return }

Expand Down Expand Up @@ -548,7 +550,7 @@ impl Collector {
debug!("Creating test {}: {}", name, test);
self.tests.push(testing::TestDescAndFn {
desc: testing::TestDesc {
name: testing::DynTestName(name),
name: testing::DynTestName(name.clone()),
ignore: should_ignore,
// compiler failures are test failures
should_panic: testing::ShouldPanic::No,
Expand All @@ -558,7 +560,7 @@ impl Collector {
let panic = io::set_panic(None);
let print = io::set_print(None);
match {
rustc_driver::in_rustc_thread(move || with_globals(move || {
rustc_driver::in_named_rustc_thread(name, move || with_globals(move || {
io::set_panic(panic);
io::set_print(print);
hygiene::set_default_edition(edition);
Expand Down
29 changes: 29 additions & 0 deletions src/test/rustdoc-ui/failed-doctest-output.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright 2013-2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Issue #51162: A failed doctest was not printing its stdout/stderr
// FIXME: if/when the output of the test harness can be tested on its own, this test should be
// adapted to use that, and that normalize line can go away

// compile-flags:--test
// normalize-stdout-test: "src/test/rustdoc-ui" -> "$$DIR"
// failure-status: 101

// doctest fails at runtime
/// ```
/// panic!("oh no");
/// ```
pub struct SomeStruct;

// doctest fails at compile time
/// ```
/// no
/// ```
pub struct OtherStruct;
32 changes: 32 additions & 0 deletions src/test/rustdoc-ui/failed-doctest-output.stdout
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@

running 2 tests
test $DIR/failed-doctest-output.rs - OtherStruct (line 26) ... FAILED
test $DIR/failed-doctest-output.rs - SomeStruct (line 20) ... FAILED

failures:

---- $DIR/failed-doctest-output.rs - OtherStruct (line 26) stdout ----
error[E0425]: cannot find value `no` in this scope
--> $DIR/failed-doctest-output.rs:27:1
|
3 | no
| ^^ not found in this scope

thread '$DIR/failed-doctest-output.rs - OtherStruct (line 26)' panicked at 'couldn't compile the test', librustdoc/test.rs:332:13
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- $DIR/failed-doctest-output.rs - SomeStruct (line 20) stdout ----
thread '$DIR/failed-doctest-output.rs - SomeStruct (line 20)' panicked at 'test executable failed:

thread 'main' panicked at 'oh no', $DIR/failed-doctest-output.rs:3:1
note: Run with `RUST_BACKTRACE=1` for a backtrace.

', librustdoc/test.rs:367:17


failures:
$DIR/failed-doctest-output.rs - OtherStruct (line 26)
$DIR/failed-doctest-output.rs - SomeStruct (line 20)

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out

12 changes: 7 additions & 5 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,18 +392,20 @@ impl TestProps {

if let Some(code) = config.parse_failure_status(ln) {
self.failure_status = code;
} else {
self.failure_status = match config.mode {
Mode::RunFail => 101,
_ => 1,
};
}

if !self.run_rustfix {
self.run_rustfix = config.parse_run_rustfix(ln);
}
});

if self.failure_status == -1 {
self.failure_status = match config.mode {
Mode::RunFail => 101,
_ => 1,
};
}

for key in &["RUST_TEST_NOCAPTURE", "RUST_TEST_THREADS"] {
if let Ok(val) = env::var(key) {
if self.exec_env.iter().find(|&&(ref x, _)| x == key).is_none() {
Expand Down