rustc_driver: get rid of the extra thread by ishitatsuyuki · Pull Request #48575 · rust-lang/rust

ishitatsuyuki · 2018-02-27T07:55:22Z

Do not rollup

We can alter the stack size afterwards on Unix.

Having a separate thread causes poor debugging experience when interrupting with signals. I have to get the backtrace of the all thread, as the main thread is waiting to join doing nothing else. This patch allows me to just run bt to get the desired backtrace.

rust-highfive · 2018-02-27T07:55:27Z

r? @petrochenkov

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

ishitatsuyuki · 2018-02-27T07:56:16Z

r? @alexcrichton

alexcrichton

Thanks! I'm a little worried about the portability here but I think we can manage it where a slice of cases can be optimized to not use a separate thread

alexcrichton · 2018-02-27T09:24:02Z

Should this be rlimit_stack?
Are you also sure this works on all Unix platforms?

Oops, typo. rlimit is a POSIX standard, so it should.

alexcrichton · 2018-02-27T09:25:00Z

If the rlim_max is too small, shouldnt a thread be spawned?

alexcrichton · 2018-02-27T09:25:33Z

If this env var is set shouldn't a thread be spawned?

alexcrichton · 2018-02-27T09:26:54Z

Could a Windows solution be explored as well? It looks like the stack size can be configured in the executable headers as there is an msvc linker flag for this.

Sounds good. Do you know a way to pass custom linker flags? It doesn't seem to be supported in build scripts.

kennytm · 2018-02-27T09:28:14Z

This needs #[cfg(unix)].
Or combine the two functions in the form:
pubfnin_rustc_thread(...){constSTACK_SIZE:usize = 16*1048576;#[cfg(not(unix))]{// use threads}#[cfg(unix)]{// use rlimits}}

kennytm · 2018-02-27T09:28:35Z

I'm not sure if this FIXME applies to the new code.

ishitatsuyuki · 2018-02-27T10:32:21Z

r? @alexcrichton

I think I've addressed all the review comments except support for Windows. As I said inline, I couldn't figure out how I can pass the /STACK link flag.

I guess it's OK because it compiles, but can you also check if I'm doing inline attributes correctly?

alexcrichton

Could a comment be added to the code as well to indicate why we're sometimes spawning a thread and sometimes not?

For Windows it looks like we'll want to pass -Wl,--stack,N on MinGW and /STACK:N on MSVC. Perhaps an unstable crate attribute could be added to src/rustc/rustc.rs?

alexcrichton · 2018-02-27T12:04:37Z

If setrlimit fails perhaps an info! log statement could be done but we fall back to spawning a thread? I don't personally know the situations where setrlimit can fail but it seems like one of those APIs that fails in obscure circumstances.

According to manual there's no way the current implementation can fail, and thus if we detected an error it's either an OS bug or logic bug of this rlimit setting code.

Unfortunately though in practice "POSIX says X" or "the manual says this can't fail" rarely holds up. Functions fail for mysterious reasons that aren't always strictly documented. I think it's best to try to head off any regressions and just fall back to spawning a thread here.

alexcrichton · 2018-02-27T12:05:22Z

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

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.

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.

retep998 · 2018-03-06T04:54:03Z

The /STACK option sets the size of the stack in bytes.
16384 bytes seems rather small.

ishitatsuyuki · 2018-03-06T07:45:42Z

r? @alexcrichton

I think I have addressed the concerns, and I have also added linker flags for Windows (didn't test on Windows). Also I no longer attempt to setrlimit as it doesn't refresh stack guard configuration.

Additionally, I modified the CI script to the CI runs benefit from this change.

alexcrichton · 2018-03-06T15:29:32Z

Oh hm isn't this missing the point of the original PR now though? Wasn't the intention to avoid the main thread for debugging and such by default?

ishitatsuyuki · 2018-03-06T15:33:21Z

Unfortunately setting rlimit after initialization isn't a safe stuff, and instead we now provide a way to get rid of it if the rlimit is configured by the user.

Maybe this can be documented somewhere else, but well, we can now achieve the ideal debugging experience by putting one line of shell configuration.

alexcrichton · 2018-03-06T15:36:24Z

Ah sorry I'm not too familiar with setrlimit, but what makes it unsafe to extend the main stack?

ishitatsuyuki · 2018-03-07T08:29:56Z

Extending the main stack breaks our stack guard page detection. Basically, when you extend the main stack after initialization, the page address stored in the SEGV handler becomes obsolete and we can no longer handle stack overflow as panic.

(What we do with stack guard changed a few times, and although currently it isn't UB it's very likely to cause unwanted behavior.)

alexcrichton · 2018-03-07T15:34:10Z

Ah ok thanks for the clarification. That strategy though sounded like a pretty good one to me, so maybe an unstable API could be added to libstd for "reset stack guard detection of this thread"?

ishitatsuyuki · 2018-03-28T10:26:49Z

Partially addressed the Windows issues. Though, the default stack size is 1MB on Windows so it's more likely to affect users with clippy.

It's possible to get the stack size, but it is rather complicated:
https://github.com/mozilla/gecko-dev/blob/9bfc0167075b756824a1f6654a5a5cc03bba4f10/js/xpconnect/src/XPCJSContext.cpp

alexcrichton · 2018-04-02T19:26:43Z

Looks like CI may be failing?

ishitatsuyuki · 2018-04-03T05:21:52Z

@alexcrichton Fixed.

alexcrichton · 2018-04-03T19:01:13Z

@bors: r+

bors · 2018-04-03T19:01:14Z

📌 Commit 7db854b has been approved by alexcrichton

bors · 2018-04-04T06:19:47Z

⌛ Testing commit 7db854b with merge 5758c2d...

rustc_driver: get rid of the extra thread **Do not rollup** We can alter the stack size afterwards on Unix. Having a separate thread causes poor debugging experience when interrupting with signals. I have to get the backtrace of the all thread, as the main thread is waiting to join doing nothing else. This patch allows me to just run `bt` to get the desired backtrace.

bors · 2018-04-04T08:53:54Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 5758c2d to master...

eddyb · 2018-04-12T00:24:05Z

I wish this was discussed by the compiler team... there are several issues with the approach and I do not believe the motivation is sufficient (debuggers are usually necessary only for LLVM aborts).

It's unsound to not spawn a dedicated thread for the compiler because we can't correctly control thread-local resources - ideally they would be allocated in the parent thread, enforcing the fact that other thread-local data cannot possibly use-after-free the resources in their own destructors.

cc @rust-lang/compiler

@ishitatsuyuki

rustc_driver: Catch ICEs on the main thread too #48575 introduced an optimization to run rustc directly on the main thread when possible. However, the threaded code detects panics when they `join()` to report as an ICE. When running directly, we need to use `panic::catch_unwind` to get the same effect. cc @ishitatsuyuki r? @alexcrichton

nagisa · 2018-07-25T15:23:46Z

There’s one more potential reason to revert this: #52577.

pnkfelix · 2018-08-02T12:48:32Z

@ishitatsuyuki would putting this mode under a -Z flag suffice for your purposes?

ishitatsuyuki · 2018-08-02T13:39:54Z

Yes, as it's not really critical.

@ishitatsuyuki

Always run rustc in a thread cc @ishitatsuyuki @eddyb r? @pnkfelix [Previously](#48575) we moved to only producing threads when absolutely necessary. Even before we opted to only create threads in some cases, which [is unsound](#48575 (comment)) due to the way we use thread local storage.

ishitatsuyuki requested a review from alexcrichton February 27, 2018 07:55

rust-highfive assigned petrochenkov Feb 27, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2018

rust-highfive assigned alexcrichton and unassigned petrochenkov Feb 27, 2018

alexcrichton reviewed Feb 27, 2018
View reviewed changes

kennytm reviewed Feb 27, 2018
View reviewed changes

ishitatsuyuki force-pushed the unix-no-thread branch 2 times, most recently from 95d750d to b11df43Compare February 27, 2018 10:30

alexcrichton reviewed Feb 27, 2018
View reviewed changes

ishitatsuyuki added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2018

ishitatsuyuki force-pushed the unix-no-thread branch 2 times, most recently from c012840 to 472fc3fCompare March 4, 2018 01:30

ishitatsuyuki changed the title ~~rustc_driver: get rid of extra thread on Unix~~rustc_driver: get rid of the extra threadMar 6, 2018

retep998 reviewed Mar 6, 2018
View reviewed changes

ishitatsuyuki force-pushed the unix-no-thread branch from 438ed2c to 06329dcCompare March 6, 2018 07:38

ishitatsuyuki added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 6, 2018

ishitatsuyuki added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2018

Fix imports
7db854b

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 Apr 3, 2018

bors merged commit 7db854b into rust-lang:masterApr 4, 2018

Mark-Simulacrum mentioned this pull request Apr 9, 2018
Move deny(warnings) into rustbuild #49715
Merged

cuviper mentioned this pull request Apr 10, 2018
rustc_driver: Catch ICEs on the main thread too #49826
Merged

nikomatsakis added the I-nominated label Apr 12, 2018

Ionic mentioned this pull request Jul 12, 2018
Upgrading to Rust 1.27.0 fails on Mac OS 10.10 with Homebrew #51838
Closed

ishitatsuyuki deleted the unix-no-thread branch July 15, 2018 13:26

nagisa mentioned this pull request Jul 25, 2018
'solaris' runtime stack guard conflicts with OS stack-clash mitigation #52577
Closed

pnkfelix mentioned this pull request Oct 4, 2018
Compiler Team Weekly Agenda #54818
Closed

oli-obk mentioned this pull request Dec 14, 2018
Always run rustc in a thread #56813
Merged

ishitatsuyuki mentioned this pull request Oct 17, 2020
SIGSEGV from rustc while building crate legion#77869
Closed

Uh oh!

Uh oh!

Conversation

ishitatsuyuki commented Feb 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Feb 27, 2018

Uh oh!

ishitatsuyuki commented Feb 27, 2018

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ishitatsuyuki commented Feb 27, 2018

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ishitatsuyuki commented Mar 6, 2018

Uh oh!

alexcrichton commented Mar 6, 2018

Uh oh!

ishitatsuyuki commented Mar 6, 2018

Uh oh!

alexcrichton commented Mar 6, 2018

Uh oh!

ishitatsuyuki commented Mar 7, 2018

Uh oh!

alexcrichton commented Mar 7, 2018

Uh oh!

ishitatsuyuki commented Mar 28, 2018

Uh oh!

alexcrichton commented Apr 2, 2018

Uh oh!

ishitatsuyuki commented Apr 3, 2018

Uh oh!

alexcrichton commented Apr 3, 2018

Uh oh!

bors commented Apr 3, 2018

Uh oh!

bors commented Apr 4, 2018

Uh oh!

bors commented Apr 4, 2018

Uh oh!

eddyb commented Apr 12, 2018

Uh oh!

nagisa commented Jul 25, 2018

Uh oh!

pnkfelix commented Aug 2, 2018

Uh oh!

ishitatsuyuki commented Feb 27, 2018•
edited
Loading