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

Document differences between std::process::abort and std::intrinsics::abort #40230

Closed
SimonSapin opened this issue Mar 3, 2017 · 19 comments · Fixed by #85377
Closed

Document differences between std::process::abort and std::intrinsics::abort #40230

SimonSapin opened this issue Mar 3, 2017 · 19 comments · Fixed by #85377
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

#37833 mentions clean ups done by the former that, as far as I know, the latter doesn’t do.

@alexcrichton
Copy link
Member

Neither of these perform any cleanups (it's exit that cleans up, not abort). The process version is the platform-specific approach wheras the intrinsics version is the processor-specific approach, typically an illegal instruction.

@SimonSapin
Copy link
Contributor Author

Perhaps clean-up is not the right word, I was referring to this PR message:

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.

The std::process::abort docs already say that Rust destructors are not run.

@alexcrichton
Copy link
Member

I believe that's another "forgot to update the PR description with precisely what was merged problem". We don't run cleanups on abort.

@SimonSapin
Copy link
Contributor Author

Alright, so this is another detail that would be good to clarify in docs.

@Mark-Simulacrum
Copy link
Member

It's not quite clear to me what we should clarify in docs here. The only thing that I can potentially see is a note on std::intrinsics::abort that it will not run cleanups -- but I'm not sure we should really document intrinsics.

@Mark-Simulacrum Mark-Simulacrum added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label May 26, 2017
@SimonSapin
Copy link
Contributor Author

I don’t know what the differences are or what "cleanups" includes exactly. servo/servo#16899 is a case where process::abort and intrinsics::abort are significantly different.

Calling process::abort from the crash handler causes the crash handler to be invoked again recursively.

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 27, 2017
@steveklabnik steveklabnik added the P-medium Medium priority label Aug 30, 2017
@steveklabnik steveklabnik added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label May 29, 2018
@nagisa
Copy link
Member

nagisa commented Nov 11, 2018

The intrinsics::abort says that its stabilised version is process::abort which is obviouosly not true, because process::abort does not call intrinsics::abort. For those who need a lightweight trap process::abort is not suitable.

@DevQps
Copy link
Contributor

DevQps commented Apr 2, 2019

Neither of these perform any cleanups (it's exit that cleans up, not abort). The process version is the platform-specific approach wheras the intrinsics version is the processor-specific approach, typically an illegal instruction.

@alexcrichton So if I understand correctly what needs to be done is documenting these two points right?:

  • Document that std::intrinsics::abort uses a processor-specific approach.
  • Document that std::process::abort uses a platform-specific approach.

Or do you believe that this distinction should not be documented in the docs? Otherwise we should probably close this issue.

@alexcrichton
Copy link
Member

I didn't open this issue, so I'll leave that to @SimonSapin, although this is a very old issue so it's likely out of cache for everyone involved.

@SimonSapin
Copy link
Contributor Author

As far as I can tell, on Linux x64 intrinsics::abort() cause a CPU exception which makes the kernel terminate the process, and process::abort() calls libc::abort which is documented to trigger the SIGABRT signal which (after possibly running a signal handler) also eventually terminates the process.

I don’t know how to document this in a OS- and architecture-independent way, or more importantly why should one be picked over the other.

@DevQps
Copy link
Contributor

DevQps commented Apr 9, 2019

@SimonSapin I just read through the docs of process::abort again and it seems that they already make some statements about cleanup:

From std::process::abort

The function will never return and will immediately terminate the current process in a platform-specific "abnormal" manner.

Note that because this function never returns, and that it terminates the process, no destructors on the current stack or any other thread's stack will be run.

I guess it might better to no document the platform specifics and rather leave it as it is? That way the process::abort function could potentially be implemented differently in the future without people relying on the specific behavior of a certain platform.

If you agree we can close this issue!

@DevQps
Copy link
Contributor

DevQps commented May 4, 2019

@SimonSapin Could you maybe still respond to my previous comment? Then we can maybe close this issue.

@comex
Copy link
Contributor

comex commented Jul 23, 2019

Is there currently no stabilized way to access intrinsics::abort?

@DevQps
Copy link
Contributor

DevQps commented Aug 8, 2019

@SimonSapin Could you maybe still respond to my previous comment? Thanks in advance!

@comex There is an issue to stabilize instrinsics::abort here: #2512 but the opinions about that different. You can however use std::process::abort. Hope that helps! And sorry for the slow response!

@SimonSapin
Copy link
Contributor Author

@DevQps I’m not sure what kind of response you expect :/ In my view we are in either one of two situations:

  • The two functions are fully interchangeable, or one of them is strictly "better" and could be used instead of the other. In this case we would not have a good reason to keep two functions, and could remove one of them.

  • Or, there is some kind trade-off: one function is preferable in some situations and the other in others. In this case I would like to understand how a user should make that choice. If someone can come up with a criteria or an explanation, we could copy that in a doc-comment. And we should stabilize a wrapper for the intrinsic.

@DevQps
Copy link
Contributor

DevQps commented Aug 22, 2019

I do not know too much about intrinsincs and abort signals sadly. @alexcrichton Could you maybe answer the questions of Simon?

@alexcrichton
Copy link
Member

These functions are not interchangeable (as pointed out by @nagisa above). Stable wrappers for intrinsics::abort isn't easy because it's platform-specific and apparently not all platforms have an abort instruction. This was stabilized as core::arch::wasm32::unreachable on wasm and if necessary we'd stabilize core::arch::x86::ud2 on x86/x86_64/etc.

@jonas-schievink jonas-schievink added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 6, 2020
@ojeda
Copy link
Contributor

ojeda commented Mar 14, 2021

From #81895 (comment):

Related: I'd argue core::intrinsics::abort() should also be renamed core::intrinsics::trap(), which reflects better what it does, matches the usual lingo and makes it less prone to be confused with the usual std::process::abort(), etc. For similar reasons I sent #81530 in an attempt to avoid potential confusion with an abort vs. unreachable instance in wasm.

@ijackson
Copy link
Contributor

ijackson commented May 17, 2021

People interested in this thread should probably indeed look at my MR #85377 which I think deals with many of the things raised here. I have linked this issue so it will be closed by my MR.

So please do review my docs proposals.

m-ou-se pushed a commit to ijackson/rust that referenced this issue Jul 5, 2021
There is discussion of this in rust-lang#40230 which requests clarification.

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
@bors bors closed this as completed in add24d2 Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants