-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Conversation
If rustc exits with a signal, previously all you saw was: ``` Compiling foo ... error: Could not compile `foo`. To learn more, run the command again with --verbose. ``` Now it shows the complete error by default: ``` Compiling foo ... error: Could not compile `foo`. Caused by: process didn't exit successfully: `rustc ...` (signal: 6, SIGABRT: process abort signal) ```
(rust_highfive has picked a reviewer for you, use r? to override) |
I'm not sure if the test is worth it? I'm uncertain if it's dependable to have consistent behavior on all unix platforms. I could easily remove it. Also, does not affect Windows, which uses weird exit codes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much!
@@ -4373,3 +4373,65 @@ fn target_filters_workspace_not_found() { | |||
.with_stderr("[ERROR] no library targets found in packages: a, b") | |||
.run(); | |||
} | |||
|
|||
#[cfg(unix)] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
if err | ||
.downcast_ref::<ProcessError>() | ||
.as_ref() | ||
.and_then(|perr| perr.exit.and_then(|e| e.code())) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@bors: r+ |
📌 Commit 6bc5e71 has been approved by |
⌛ Testing commit 6bc5e71 with merge 5a890f1c66d11bfbd26087e637a34b56e7a6a1e8... |
💥 Test timed out |
@bors: retry |
Don't treat rustc exit on signal as internal error. If rustc exits with a signal, previously all you saw was: ``` Compiling foo ... error: Could not compile `foo`. To learn more, run the command again with --verbose. ``` Now it shows the complete error by default: ``` Compiling foo ... error: Could not compile `foo`. Caused by: process didn't exit successfully: `rustc ...` (signal: 6, SIGABRT: process abort signal) ```
☀️ Test successful - status-appveyor, status-travis |
Show better error message for Windows abnormal termination. This helps display better error messages when there is an abnormal termination on Windows. If rustc crashes, there was a slight mistake in #6270, where the error code was actually negative, so the message was getting hidden behind the `--verbose` flag. This changes it show that the abnormal error is always shown if rustc crashes in an unusual way. This also changes `cargo test` to display a better message if the test crashes. Previously: ``` running 1 test error: test failed, to rerun pass '--bin crash' ``` New: ``` running 1 test error: test failed, to rerun pass '--bin crash' Caused by: process didn't exit successfully: `D:\Temp\crash\target\debug\deps\crash-b3c2389529da3d3e.exe` (exit code: 0xc0000374, STATUS_HEAP_CORRUPTION) ``` I didn't add any tests because testing on this on Windows seems a little precarious. AFAIK, the exact error message depends on the processor (like x86 vs i686), and Windows version. I was concerned about chasing down every little nuance, though I'm willing to try if it seems important. Fixes rust-lang/rust#65692 (I think).
Show better error message for Windows abnormal termination. This helps display better error messages when there is an abnormal termination on Windows. If rustc crashes, there was a slight mistake in #6270, where the error code was actually negative, so the message was getting hidden behind the `--verbose` flag. This changes it show that the abnormal error is always shown if rustc crashes in an unusual way. This also changes `cargo test` to display a better message if the test crashes. Previously: ``` running 1 test error: test failed, to rerun pass '--bin crash' ``` New: ``` running 1 test error: test failed, to rerun pass '--bin crash' Caused by: process didn't exit successfully: `D:\Temp\crash\target\debug\deps\crash-b3c2389529da3d3e.exe` (exit code: 0xc0000374, STATUS_HEAP_CORRUPTION) ``` I didn't add any tests because testing on this on Windows seems a little precarious. AFAIK, the exact error message depends on the processor (like x86 vs i686), and Windows version. I was concerned about chasing down every little nuance, though I'm willing to try if it seems important. Fixes rust-lang/rust#65692 (I think).
Show better error message for Windows abnormal termination. This helps display better error messages when there is an abnormal termination on Windows. If rustc crashes, there was a slight mistake in #6270, where the error code was actually negative, so the message was getting hidden behind the `--verbose` flag. This changes it show that the abnormal error is always shown if rustc crashes in an unusual way. This also changes `cargo test` to display a better message if the test crashes. Previously: ``` running 1 test error: test failed, to rerun pass '--bin crash' ``` New: ``` running 1 test error: test failed, to rerun pass '--bin crash' Caused by: process didn't exit successfully: `D:\Temp\crash\target\debug\deps\crash-b3c2389529da3d3e.exe` (exit code: 0xc0000374, STATUS_HEAP_CORRUPTION) ``` I didn't add any tests because testing on this on Windows seems a little precarious. AFAIK, the exact error message depends on the processor (like x86 vs i686), and Windows version. I was concerned about chasing down every little nuance, though I'm willing to try if it seems important. Fixes rust-lang/rust#65692 (I think).
If rustc exits with a signal, previously all you saw was:
Now it shows the complete error by default: