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

Add std::process::abort #37833

Merged
merged 1 commit into from
Nov 20, 2016
Merged

Add std::process::abort #37833

merged 1 commit into from
Nov 20, 2016

Conversation

sfackler
Copy link
Member

This calls libc abort on Unix and fastfail on Windows, first running
cleanups to do things like flush stdout buffers. This matches with libc
abort's behavior, which flushes open files.

r? @alexcrichton

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 17, 2016
@alexcrichton
Copy link
Member

Looks reasonable to me. I forget, did we decide that we'd only fcp to merge for stabilizations or also new APIs? I think it's the former?

I'd be fine r+'ing if we did indeed conclude there's no need for fcp merge for all new unstable apis.

@alexcrichton
Copy link
Member

Oh right, this'll also need a tracking issue before merging

@sfackler
Copy link
Member Author

Tracking issue added.

@brson
Copy link
Contributor

brson commented Nov 17, 2016

If this is doing the same thing as the std runtime is today then I'm fine landing, if not I'd like to understand the differences.

@sfackler
Copy link
Member Author

Runtime aborts do use abort_internal as well. The only difference here is that we also run cleanups. Not sure if that's something we should do universally, but it definitely seems appropriate for std::process::abort.

@retep998
Copy link
Member

retep998 commented Nov 17, 2016

abort_internal appears to only be called by sys_common::util::abort which in turn is only called by rtabort!. which in turn is only invoked when unix platforms overflow the stack or if libstd fails to initiate a panic.

There's so many places where Rust calls intrinsics::abort on Windows, even though the current Windows impl of abort_internal would be so much better (because it triggers an unstoppable fast fail exception rather than a confusing illegal instruction exception). We really need an RFC to do a comprehensive cleanup of all the ways that Rust can currently abort.

@alexcrichton
Copy link
Member

Looking again at sys_common::cleanup, we may actually want to skip it. It performs tasks like:

  • Synchronization via Once
    • May involve thread::park
    • May involve TLS
    • May involve registering TLS depending on the platform
  • Cleaning up stored global arguments
  • Deregistering stack overflow handlers
  • Calling at_exit_imp callbacks
    • This will drop I/O handles and flush stdout/stderr I believe
    • De-initializes windows networking
    • De-initializes and deallocates windows TLS storage tracking.

I think that all of this is relatively dangerous, and optional, to be calling in a context where abort was invoked. These are all very atexit-like things and in C at least abort doesn't execute atexit callbacks unlike the exit function.

@retep998
Copy link
Member

retep998 commented Nov 17, 2016

This matches with libc abort's behavior, which flushes open files.

On Windows, libc::abort does not actually flush any files. It merely raises the SIGABRT handler if one is registered and then does a __fastfail(FAST_FAIL_FATAL_APP_EXIT);.

This calls libc abort on Unix and fastfail on Windows.
@sfackler
Copy link
Member Author

Updated to no longer run cleanups.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 18, 2016

📌 Commit fc0140d has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 20, 2016

⌛ Testing commit fc0140d with merge 7c535c6...

bors added a commit that referenced this pull request Nov 20, 2016
Add std::process::abort

This calls libc abort on Unix and fastfail on Windows, first running
cleanups to do things like flush stdout buffers. This matches with libc
abort's behavior, which flushes open files.

r? @alexcrichton
@bors bors merged commit fc0140d into rust-lang:master Nov 20, 2016
@sfackler sfackler deleted the process-abort branch November 20, 2016 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants