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

rustc_driver: get rid of the extra thread #48575

Merged
merged 10 commits into from
Apr 4, 2018
57 changes: 49 additions & 8 deletions src/librustc_driver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#![feature(quote)]
#![feature(rustc_diagnostic_macros)]
#![feature(set_stdio)]
#![feature(rustc_stack_internals)]

extern crate arena;
extern crate getopts;
Expand Down Expand Up @@ -1461,16 +1462,56 @@ pub fn in_rustc_thread<F, R>(f: F) -> Result<R, Box<Any + Send>>
// Temporarily have stack size set to 16MB to deal with nom-using crates failing
const STACK_SIZE: usize = 16 * 1024 * 1024; // 16MB

let mut cfg = thread::Builder::new().name("rustc".to_string());
#[cfg(unix)]
let spawn_thread = unsafe {
// Fetch the current resource limits
let mut rlim = libc::rlimit {
rlim_cur: 0,
rlim_max: 0,
};
if libc::getrlimit(libc::RLIMIT_STACK, &mut rlim) != 0 {
let err = io::Error::last_os_error();
error!("in_rustc_thread: error calling getrlimit: {}", err);
true
} else if rlim.rlim_max < STACK_SIZE as libc::rlim_t {
true
} else {
std::rt::deinit_stack_guard();
rlim.rlim_cur = STACK_SIZE as libc::rlim_t;
if libc::setrlimit(libc::RLIMIT_STACK, &mut rlim) != 0 {
let err = io::Error::last_os_error();
// We have already deinited the stack. Further corruption is
Copy link
Member

Choose a reason for hiding this comment

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

We could fall through and call update_stack_guard again here, right?

// not allowed.
panic!("in_rustc_thread: error calling setrlimit: {}", err);
} else {
std::rt::update_stack_guard();
false
}
}
};

// FIXME: Hacks on hacks. If the env is trying to override the stack size
// then *don't* set it explicitly.
if env::var_os("RUST_MIN_STACK").is_none() {
cfg = cfg.stack_size(STACK_SIZE);
}
// We set the stack size at link time. See src/rustc/rustc.rs.
#[cfg(windows)]
let spawn_thread = false;

#[cfg(not(any(windows,unix)))]
let spawn_thread = true;

let thread = cfg.spawn(f);
thread.unwrap().join()
// 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());

// FIXME: Hacks on hacks. If the env is trying to override the stack size
// then *don't* set it explicitly.
if env::var_os("RUST_MIN_STACK").is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be checked earlier? If this env var is set I think we'll want to force spawning a thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree: that environment variable only affects stack size of non-main thread. If we can extend the main thread stack, that's all what we need.

Copy link
Member

Choose a reason for hiding this comment

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

Without moving this check earlier it's breaking existing behavior. Today the stack size of the rustc thread can be controlled by configuring RUST_MIN_STACK (can be useful for debugging). This commit breaks that ability because setrlimit will probably succeed on Linux.

cfg = cfg.stack_size(STACK_SIZE);
}

let thread = cfg.spawn(f);
thread.unwrap().join()
} else {
Ok(f())
}
}

/// Get a list of extra command-line flags provided by the user, as strings.
Expand Down
15 changes: 15 additions & 0 deletions src/libstd/rt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,18 @@ fn lang_start<T: ::process::Termination + 'static>
{
lang_start_internal(&move || main().report(), argc, argv)
}

/// Function used for reverting changes to the main stack before setrlimit().
/// This is POSIX (non-Linux) specific and unlikely to be directly stabilized.
#[unstable(feature = "rustc_stack_internals", issue = "0")]
pub unsafe fn deinit_stack_guard() {
::sys::thread::guard::deinit();
}

/// Function used for resetting the main stack guard address after setrlimit().
/// This is POSIX specific and unlikely to be directly stabilized.
#[unstable(feature = "rustc_stack_internals", issue = "0")]
pub unsafe fn update_stack_guard() {
let main_guard = ::sys::thread::guard::init();
::sys_common::thread_info::reset_guard(main_guard);
}
20 changes: 19 additions & 1 deletion src/libstd/sys/unix/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ pub mod guard {
#[cfg_attr(test, allow(dead_code))]
pub mod guard {
use libc;
use libc::mmap;
use libc::{mmap, munmap};
use libc::{PROT_NONE, MAP_PRIVATE, MAP_ANON, MAP_FAILED, MAP_FIXED};
use ops::Range;
use sys::os;
Expand Down Expand Up @@ -336,6 +336,24 @@ pub mod guard {
}
}

pub unsafe fn deinit() {
if !cfg!(target_os = "linux") {
if let Some(mut stackaddr) = get_stack_start() {
// Ensure address is aligned. Same as above.
let remainder = (stackaddr as usize) % PAGE_SIZE;
Copy link
Member

Choose a reason for hiding this comment

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

This looks the same as the calculation above, could they be unified perhaps?

if remainder != 0 {
stackaddr = ((stackaddr as usize) + PAGE_SIZE - remainder)
as *mut libc::c_void;
}

// Undo the guard page mapping.
if munmap(stackaddr, PAGE_SIZE) != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like on FreeBSD we map 2 pages? Could the logic above and here be shared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't. It's just a wider guard address range, unrelated to what we memory map.

panic!("unable to deallocate the guard page");
}
}
}
}

#[cfg(any(target_os = "macos",
target_os = "bitrig",
target_os = "openbsd",
Expand Down
4 changes: 4 additions & 0 deletions src/libstd/sys_common/thread_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,7 @@ pub fn set(stack_guard: Option<Guard>, thread: Thread) {
thread,
}));
}

pub fn reset_guard(stack_guard: Option<Guard>) {
THREAD_INFO.with(move |c| c.borrow_mut().as_mut().unwrap().stack_guard = stack_guard);
}
9 changes: 9 additions & 0 deletions src/rustc/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@
// except according to those terms.

#![feature(rustc_private)]
#![feature(link_args)]

// Set the stack size at link time on Windows. See rustc_driver::in_rustc_thread
// for the rationale.
#[cfg_attr(all(windows, target_env = "msvc"), link_args = "/STACK:16777216")]
// We only build for msvc and gnu now, but we use a exhaustive condition here
// so we can expect either the stack size to be set or the build fails.
#[cfg_attr(all(windows, not(target_env = "msvc")), link_args = "-Wl,--stack,16777216")]
extern {}

extern crate rustc_driver;

Expand Down