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

Don't treat rustc exit on signal as internal error. #6270

Merged
merged 3 commits into from
Nov 7, 2018

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 6, 2018

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)

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
Copy link

r? @alexcrichton

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

@ehuss
Copy link
Contributor Author

ehuss commented Nov 6, 2018

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.

Copy link
Member

@alexcrichton alexcrichton left a 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)]
Copy link
Member

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

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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()))
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Nov 6, 2018

📌 Commit 6bc5e71 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Nov 6, 2018

⌛ Testing commit 6bc5e71 with merge 5a890f1c66d11bfbd26087e637a34b56e7a6a1e8...

@bors
Copy link
Collaborator

bors commented Nov 7, 2018

💥 Test timed out

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Collaborator

bors commented Nov 7, 2018

⌛ Testing commit 6bc5e71 with merge a83cd61...

bors added a commit that referenced this pull request Nov 7, 2018
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)
```
@bors
Copy link
Collaborator

bors commented Nov 7, 2018

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

@bors bors merged commit 6bc5e71 into rust-lang:master Nov 7, 2018
bors added a commit that referenced this pull request Oct 23, 2019
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).
bors added a commit that referenced this pull request Oct 24, 2019
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).
bors added a commit that referenced this pull request Oct 24, 2019
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).
@ehuss ehuss added this to the 1.32.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants