Misc tweaks #50276
Misc tweaks #50276
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
src/Cargo.toml
Outdated
| @@ -44,9 +44,9 @@ members = [ | |||
| # MSVC when running the compile-fail test suite when a should-fail test panics. | |||
| # But hey if this is removed and it gets past the bots, sounds good to me. | |||
| [profile.release] | |||
| opt-level = 2 | |||
| opt-level = 3 | |||
There was a problem hiding this comment.
This is likely still unsafe. See #48204.
There was a problem hiding this comment.
LLVM was updated to 6.0 since then, so that bug may be fixed. I know that at least one codegen bug disappeared with that update.
There was a problem hiding this comment.
#48204 was tested on 2018 Feb 25th where LLVM was already upgraded to 6.0 (2018 Feb 10th).
There was a problem hiding this comment.
LLVM 6.0 was released Mar 8, 2018.
There was a problem hiding this comment.
Although #47828 isn't the most up-to-date LLVM 6 branch and #48782 corresponds to the actual release version, there's nothing guaranteeing -O3 would become safe.
Please split this change into a separate PR anyway. Given the bad track record, I wouldn't want to lump this together with other things here (especially you're including so many Windows changes).
There was a problem hiding this comment.
I agree with @kennytm that this I think is best left to a separate PR so we can track it independently in terms of perf, regressions, etc.
There was a problem hiding this comment.
I made a separate PR for this.
src/bootstrap/bin/rustc.rs
Outdated
| // Print backtrace in case of ICE | ||
| if env::var("RUST_BACKTRACE").is_err() { | ||
| cmd.env("RUST_BACKTRACE", "1"); | ||
| } |
There was a problem hiding this comment.
You can turn it off with RUST_BACKTRACE=0, if you're hitting ICEs and don't want to look at the backtrace for some odd reason. #37477 should no longer apply since rustc doesn't use panics during normal operation.
There was a problem hiding this comment.
This wrapper is not used for running tests, right? Then it should be fine.
There was a problem hiding this comment.
It is not used for running tests. I don't think it's used for compiling tests either.
There was a problem hiding this comment.
Hm we've had bad enough luck with this in the past that I'd personally prefer to leave this off by default and it can always be opted into by setting it locally, right?
There was a problem hiding this comment.
I can't think of any scenario where you wouldn't want a backtrace here. I want backtraces on CI so it should be enabled there. Opting into it locally is annoying, and it's easy to forget to turn it off when you want to run tests. What kind of bad luck are you referring to?
There was a problem hiding this comment.
I personally do not want backtraces in the sense that any ICE I run into is probably one that I can't fix and it otherwise would print hundreds of lines of backtrace information.
Additionally the "bad luck" is horrible performance with backtraces and otherwise backtraces worsening problems when there are bugs in the backtrace implementation.
There was a problem hiding this comment.
I could probably make this a config.toml option.
There was a problem hiding this comment.
Yeah having a config option seems reasonable
|
I do get 2 odd panics here when building locally (even with opt-level=2). Panic for `compile-fail\issue-36638.rs`:Panic for `compile-fail\borrowck\two-phase-reservation-sharing-interference.rs`: |
src/bootstrap/bin/rustc.rs
Outdated
| // Enable the Windows Error Reporting dialog which bootstrap disabled, | ||
| // so we can JIT debug rustc | ||
| let mode = SetErrorMode(0); | ||
| SetErrorMode(mode & !SEM_NOGPFAULTERRORBOX); |
There was a problem hiding this comment.
This seems like it's not necessarily the most appropriate place for this, should the block in src/bootstrap/job.rs instead be located in compiletest?
There was a problem hiding this comment.
That may be better. This code could be moved to src/bootstrap/job.rs and then compiletest and maybe rustdoc would disable Windows Error Reporting again when running tests.
src/librustc/Cargo.toml
Outdated
| @@ -27,6 +27,7 @@ syntax = { path = "../libsyntax" } | |||
| syntax_pos = { path = "../libsyntax_pos" } | |||
| backtrace = "0.3.3" | |||
| byteorder = { version = "1.1", features = ["i128"]} | |||
| getopts = "0.2" | |||
There was a problem hiding this comment.
How come this is necessary? I believe this means that we're building and shipping two copies of getopts, right? Was the one copy previously causing problems?
There was a problem hiding this comment.
It is more accurate and probably I good idea with #49119, since otherwise librustc will depend on the bootstrap compiler's getopts, which makes updating this dependency tricky.
There was a problem hiding this comment.
We should also make sure compiler crates can depend on crates.io crates which in turn depend on getopts. @nikomatsakis ran into problems adding such a dependency.
There was a problem hiding this comment.
Isn't this what rustc_cratesio_shim fixes?
There was a problem hiding this comment.
In general no compiler crate should depend on getopts because nothing should be doing argument parsing other than rustc itself. @eddyb that's more for the dylib situation rather than depending on things at all, this is a little different because getopts is currently inherited through libtest without an explicit dependency
There was a problem hiding this comment.
I think this can be fixed by having rustc.exe remove --extern=getopts arguments so crates.io crates are also forced to use the getopts in the sysroot.
src/librustc/util/common.rs
Outdated
| unsafe { | ||
| if env::var("RUSTC_BOOTSTRAP").is_ok() { | ||
| extern "system" { | ||
| fn DebugBreak(); |
There was a problem hiding this comment.
What happens here if a debugger isn't attached or if a debugger isn't listening? Does this cause the process to otherwise abnormally terminate in a different way than it would have earlier?
Also doesn't this panic handler run for all sorts of compiler errors as they're communicated through panics?
There was a problem hiding this comment.
What happens here if a debugger isn't attached or if a debugger isn't listening?
You'll get an unhandled exception, which if Windows Error Reporting is enabled gives you the option to step into a debugger.
Does this cause the process to otherwise abnormally terminate in a different way than it would have earlier?
Yes it does. It has similar behavior to -C panic=abort, which we want the compiler to eventually switch to. It will no longer print the ICE message. People building rustc should be able to identify an ICE though.
Also doesn't this panic handler run for all sorts of compiler errors as they're communicated through panics?
Since #47634, compiler errors no longer use panics, they just unwind instead.
There was a problem hiding this comment.
Ok, could this just be updated to be more reliably in detecting when it's under rustbuild?
There was a problem hiding this comment.
What would you consider reliable? I would like this behavior by default btw, at least for rustc.exe.
There was a problem hiding this comment.
I think just a new env var set by rustbuild should work, and I don't think we should enable these by default. I'd hazard a guess that the vast majority of ICEs are hit by non-compiler-developers, in which case opening up a debugger won't really help anything.
There was a problem hiding this comment.
This doesn't open a debugger. It just shows Windows normal crash dialog, which crashing application should open. This is what we do with -C panic=abort.
There was a problem hiding this comment.
I think that's what we also don't want, right? Ideally, again in the non-compiler-dev case, I'd just want the application to exit with an approrpiate message in the terminal to diagnose later.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I've removed the |
| @@ -1578,8 +1610,7 @@ impl<'test> TestCx<'test> { | |||
| let newpath = env::join_paths(&path).unwrap(); | |||
| command.env(dylib_env_var(), newpath); | |||
|
|
|||
| let mut child = command | |||
| .spawn() | |||
| let mut child = disable_error_reporting(|| command.spawn()) | |||
There was a problem hiding this comment.
Doesn't this mean that we're only executing one test at a time since this is globally serialized?
There was a problem hiding this comment.
No. spawn doesn't wait for the completion of the process.
|
@bors: r+ |
|
📌 Commit 7e304aa has been approved by |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit 7e304aa4003f25a74a8244d9c960e80094b82140 with merge c4494cc7bc95a8eb6d4830ae5e0d085ca73fa9fb... |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
| } | ||
|
|
||
| lazy_static! { | ||
| static ref LOCK: Mutex<()> = { |
There was a problem hiding this comment.
You forgot to use std::sync::Mutex;.
error[E0433]: failed to resolve. Use of undeclared type or module `Mutex`
--> tools\compiletest\src\runtest.rs:50:13
|
50 | Mutex::new(())
| ^^^^^ Use of undeclared type or module `Mutex`
error[E0412]: cannot find type `Mutex` in this scope
--> tools\compiletest\src\runtest.rs:49:26
|
49 | static ref LOCK: Mutex<()> = {
| ^^^^^ not found in this scope
help: possible candidate is found in another module, you can import it into scope
|
11 | use std::sync::Mutex;
|
error[E0599]: no method named `lock` found for type `runtest::disable_error_reporting::LOCK` in the current scope
--> tools\compiletest\src\runtest.rs:54:22
|
48 | / lazy_static! {
49 | | static ref LOCK: Mutex<()> = {
50 | | Mutex::new(())
51 | | };
52 | | }
| |_____- method `lock` not found for this
53 | // Error mode is a global variable, so lock it so only one thread will change it
54 | let _lock = LOCK.lock().unwrap();
| ^^^^
|
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: aborting due to 3 previous errors
kennytm
added
S-waiting-on-author
|
@bors r=alexcrichton |
|
📌 Commit e24cbe2 has been approved by |
bors
added
S-waiting-on-bors
Misc tweaks This: - ~~Add explicit dependencies on `getops`~~ - Fixes the libtest-json test when `RUST_BACKTRACE=1` is set - ~~Sets `opt-level` to `3`~~ - Removes the use of `staged_api` from `rustc_plugin` - ~~Enables the Windows Error Reporting dialog when running rustc during bootstrapping~~ - Disables Windows Error Reporting dialog when running compiletest tests - Enables backtraces when running rustc during bootstrapping - ~~Removes the `librustc` dependency on `libtest`~~ - Triggers JIT debugging on Windows if rustc panics during bootstrapping r? @alexcrichton
|
☀️ Test successful - status-appveyor, status-travis |
…richton AppVeyor: Read back trace from crash dump on failure. This is an attempt to debug spurious access violations on Windows (rust-lang#33434, rust-lang#50604). Thanks to rust-lang#50276, there should be minidumps to read when rustc segfault. (The implementation is based on <https://github.com/springmeyer/travis-coredump/blob/master/test.bat>.)
…richton AppVeyor: Read back trace from crash dump on failure. This is an attempt to debug spurious access violations on Windows (rust-lang#33434, rust-lang#50604). Thanks to rust-lang#50276, there should be minidumps to read when rustc segfault. (The implementation is based on <https://github.com/springmeyer/travis-coredump/blob/master/test.bat>.)
…richton AppVeyor: Read back trace from crash dump on failure. This is an attempt to debug spurious access violations on Windows (rust-lang#33434, rust-lang#50604). Thanks to rust-lang#50276, there should be minidumps to read when rustc segfault. (The implementation is based on <https://github.com/springmeyer/travis-coredump/blob/master/test.bat>.)
This:
Add explicit dependencies ongetopsRUST_BACKTRACE=1is setSetsopt-levelto3staged_apifromrustc_pluginEnables the Windows Error Reporting dialog when running rustc during bootstrappingRemoves thelibrustcdependency onlibtestr? @alexcrichton