aborts: Clarify documentation and comments #85377

ijackson · 2021-05-16T17:25:16Z

In the docs for intrinsics::abort():

Strengthen the recommendation by to use process::abort instead.
Document the fact that it sometimes (ab)uses an LLVM debug trap and what the likely consequences are.
State that the precise behaviour is unstable.

In the docs for process::abort():

Promise that we have the same behaviour as C abort().
Document the likely consequences, including, specifically, the consequences on Unix.

In the internal comment for unix::abort_internal:

Refer to the public docs for the public API functions.
Correct and expand the description of libc::abort. Specifically:
Do not claim that abort() unregisters signal handlers. It doesn't; it honours the SIGABRT handler.
Discuss, extensively, the issue with abort() flushing stdio buffers.
Describe the glibc behaviour in some detail.

Co-authored-by: Mark Wooding [email protected]
Signed-off-by: Ian Jackson [email protected]

Fixes #40230

rust-highfive · 2021-05-16T17:25:19Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

ijackson · 2021-05-16T17:26:31Z

I have a WIP branch to make panic aborts all be libc::abort but it's not ready yet.

Urgau · 2021-05-16T17:35:09Z

I have a WIP branch to make panic aborts all be libc::abort but it's not ready yet.

How will you handle system that don't have libc or not even an OS ?

library/std/src/sys/unix/mod.rs

library/core/src/intrinsics.rs

ijackson · 2021-05-16T19:58:51Z

How will you handle system that don't have libc or not even an OS ?

This is in std, and (IIRC, without reminding myself by looking at the code again) the answer is by calling abort_internal. But let us discuss that in the MR for that other branch when I have it prepared. Right now it doesn't actually work completely right :-).

ojeda · 2021-05-17T17:04:17Z

library/std/src/sys/unix/mod.rs

+// See the public documentation for `intrinsics::abort()` and `process::abort()`
+// for further discussion.
+//
+// There is confusion about whether libc::abort() flushes stdio streams.


Perhaps mention it is implementation-defined as a first point (C18 7.22.4.1p1).

ojeda · 2021-05-17T17:06:45Z

library/std/src/sys/unix/mod.rs

+// for further discussion.
+//
+// There is confusion about whether libc::abort() flushes stdio streams.
+// libc::abort() is required by ISO C 99 (7.14.1.1p5) to be async-signal-safe,


It would be better to point to C18 (it is still 7.14.1.1p5).

library/std/src/sys/unix/mod.rs

SimonSapin · 2021-05-19T07:34:37Z

What motivated opening #40230 is that Servo has a "crash handler" registered for SEGV, ILL, IOT, and BUS signals that prints the stack/backtrace of the current thread then aborts the process. These backtraces have proven many times essential in debugging crashes that happen during automated CI.

When aborting with std::process::abort we encountered a bug where the signal handler would be called in a loop. This is consistent with this PR’s comment that libc::abort may trigger one of the registered signals. The best (least worse) work-around we found at the time was to use std::intrinsics::abort instead, despite the recommendation to prefer std::process::abort: servo/servo#16899. Now I realize that a better fix might have been to have the handler unregister itself the first time it runs?

ijackson · 2021-05-19T12:11:20Z

What motivated opening #40230 is that Servo has a "crash handler" registered for SEGV, ILL, IOT, and BUS signals that prints the stack/backtrace of the current thread then aborts the process. These backtraces have proven many times essential in debugging crashes that happen during automated CI.

Yes, this is not an uncommon technique. I'm not sure, though, why ABRT wasn't in your list there.

When aborting with std::process::abort we encountered a bug where the signal handler would be called in a loop. This is consistent with this PR’s comment that libc::abort may trigger one of the registered signals. The best (least worse) work-around we found at the time was to use std::intrinsics::abort instead, despite the recommendation to prefer std::process::abort: servo/servo#16899. Now I realize that a better fix might have been to have the handler unregister itself the first time it runs?

Yes, the usual approach to this problem is indeed to make sure the signal handler does not run recursively. You can easily have the kernel reset the signal handler for you: pass the SA_RESETHAND flag in sa_flags when calling sigaction.

Do you think my docs here would have been sufficient to help your former selves? :-)

SimonSapin · 2021-05-19T12:17:51Z

I'm not sure, though, why ABRT wasn't in your list there.

I’m not sure either. Presumably printing the stack is less useful in cases of deliberate abort than for crashes caused by Undefined Behavior?

I just realized that it’s the intrinsics::abort docs that mention SIGILL and SIGSEGV, but the process::abort docs only mention SIGART, which is not consistent with how we observed the looped/recursive handler at the time :/

m-ou-se · 2021-06-05T14:30:25Z

library/core/src/intrinsics.rs

+    /// The current implementation of `intrinsics::abort` (ab)uses a debug trap
+    /// on some popular platforms.
+    /// On Unix, the
+    /// process will probably die of a signal like `SIGABRT`, `SIGILL`, `SIGTRAP`, `SIGSEGV` or
+    /// `SIGBUS`.  The precise behaviour is not guaranteed and not stable.


Two notes:

I don't think there's much use in calling this 'abuse'.

In the rest of std we use the word 'terminate' instead of 'die' for processes.

How about something like this?

Suggested change

/// The current implementation of `intrinsics::abort` (ab)uses a debug trap

/// on some popular platforms.

/// On Unix, the

/// process will probably die of a signal like `SIGABRT`, `SIGILL`, `SIGTRAP`, `SIGSEGV` or

/// `SIGBUS`. The precise behaviour is not guaranteed and not stable.

/// The current implementation of `intrinsics::abort` results in an invalid

/// instruction on most platforms. On Unix, this will probably cause the

/// process to be terminated with a signal like `SIGABRT`, `SIGILL`,

/// `SIGTRAP`, `SIGSEGV` or `SIGBUS`. The precise behaviour is not

/// guaranteed and not stable.

Thaks for the attention. I have more-or-less adopted your suggestions. I preferred a more active voice.

ijackson · 2021-06-07T15:21:24Z

I have (belatedly) enabled the pre-push hook that stops me pushing those empty WIP commits. Apologies for the noise.

library/core/src/intrinsics.rs

bors · 2021-07-02T22:53:04Z

☔ The latest upstream changes (presumably #86817) made this pull request unmergeable. Please resolve the merge conflicts.

In the docs for intrinsics::abort(): * Strengthen the recommendation by to use process::abort instead. * Document the fact that it (ab)uses an LLVM debug trap and what the likely consequences are. * State that the precise behaviour is unstable. In the docs for process::abort(): * Promise that we have the same behaviour as C `abort()`. * Document the likely consequences, including, specifically, the consequences on Unix. In the internal comment for unix::abort_internal: * Refer to the public docs for the public API functions. * Correct and expand the description of libc::abort. Specifically: * Do not claim that abort() unregisters signal handlers. It doesn't; it honours the SIGABRT handler. * Discuss, extensively, the issue with abort() flushing stdio buffers. * Describe the glibc behaviour in some detail. Co-authored-by: Mark Wooding <[email protected]> Signed-off-by: Ian Jackson <[email protected]>

As per discussion here rust-lang#85377 (review) Signed-off-by: Ian Jackson <[email protected]>

There is discussion of this in rust-lang#40230 which requests clarification. Signed-off-by: Ian Jackson <[email protected]>

@m-ou-se

And withdraw the allegation of "abuse". Adapted from a suggestion by @m-ou-se. Co-authored-by: Mara Bos <[email protected]> Signed-off-by: Ian Jackson <[email protected]>

@m-ou-se

Adapted from a suggestion by @m-ou-se. Co-authored-by: Mara Bos <[email protected]> Signed-off-by: Ian Jackson <[email protected]>

Co-authored-by: Yuki Okushi <[email protected]>

m-ou-se · 2021-07-05T10:49:21Z

Applied a suggestion and rebased for a merge conflict.

@bors r+ rollup

bors · 2021-07-05T10:49:23Z

📌 Commit 08d912f has been approved by m-ou-se

ijackson · 2021-07-05T10:56:47Z

Mara Bos writes ("Re: [rust-lang/rust] aborts: Clarify documentation and comments (#85377)"):

Applied a suggestion and rebased for a merge conflict.

Oh, thank you!

…

-- Ian Jackson ***@***.***> These opinions are my own. Pronouns: they/he. If I emailed you from @fyvzl.net or @evade.org.uk, that is a private address which bypasses my fierce spamfilter.

Rollup of 7 pull requests Successful merges: - rust-lang#83581 (Add std::os::unix::fs::DirEntryExt2::file_name_ref(&self) -> &OsStr) - rust-lang#85377 (aborts: Clarify documentation and comments) - rust-lang#86685 (double-check mutability inside Allocation) - rust-lang#86794 (Stabilize `Seek::rewind()`) - rust-lang#86852 (Remove some doc aliases) - rust-lang#86878 (:arrow_up: rust-analyzer) - rust-lang#86886 (Remove `impl Clean for {Ident, Symbol}`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned m-ou-se May 16, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2021

ijackson mentioned this pull request May 16, 2021

Introduce sys_common::rt::rtprintpanic! to replace sys_common::util functionality #84697

Merged

the8472 reviewed May 16, 2021

View reviewed changes

library/std/src/sys/unix/mod.rs Show resolved Hide resolved

ghost reviewed May 16, 2021

View reviewed changes

library/core/src/intrinsics.rs Outdated Show resolved Hide resolved

ijackson mentioned this pull request May 17, 2021

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

Closed

ojeda reviewed May 17, 2021

View reviewed changes

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2021

m-ou-se reviewed Jun 5, 2021

View reviewed changes

ijackson force-pushed the abort-docs branch from 668154a to 3174fd6 Compare June 7, 2021 11:40

ijackson mentioned this pull request Jun 25, 2021

Improve logging of abort #52633

Closed

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021

JohnTitor reviewed Jun 28, 2021

View reviewed changes

library/core/src/intrinsics.rs Outdated Show resolved Hide resolved

ijackson and others added 5 commits July 5, 2021 12:43

abort docs: Do not claim that intrinsics::abort is always a debug trap

de19e4d

As per discussion here rust-lang#85377 (review) Signed-off-by: Ian Jackson <[email protected]>

abort docs: Document buffer non-flushing

4e7c348

There is discussion of this in rust-lang#40230 which requests clarification. Signed-off-by: Ian Jackson <[email protected]>

Talk about invalid instructions rather than debug traps

44852e0

And withdraw the allegation of "abuse". Adapted from a suggestion by @m-ou-se. Co-authored-by: Mara Bos <[email protected]> Signed-off-by: Ian Jackson <[email protected]>

Talk about "terminate" rather than "die"

19c347e

Adapted from a suggestion by @m-ou-se. Co-authored-by: Mara Bos <[email protected]> Signed-off-by: Ian Jackson <[email protected]>

m-ou-se and others added 2 commits July 5, 2021 12:43

Use american spelling for behaviour

f73a555

Co-authored-by: Yuki Okushi <[email protected]>

s/die/terminate/ in abort documentation.

08d912f

m-ou-se force-pushed the abort-docs branch from 67050e1 to 08d912f Compare July 5, 2021 10:48

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2021

m-ou-se mentioned this pull request Jul 5, 2021

Fix incorrect comment on abort_internal #86760

Closed

JohnTitor mentioned this pull request Jul 5, 2021

Rollup of 7 pull requests #86891

Merged

bors merged commit add24d2 into rust-lang:master Jul 5, 2021

rustbot added this to the 1.55.0 milestone Jul 5, 2021

jyn514 mentioned this pull request Apr 26, 2023

Why does panicking in a Drop impl cause SIGILL? #81895

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aborts: Clarify documentation and comments #85377

aborts: Clarify documentation and comments #85377

ijackson commented May 16, 2021 •

edited by JohnTitor

Loading

rust-highfive commented May 16, 2021

ijackson commented May 16, 2021

Urgau commented May 16, 2021

ijackson commented May 16, 2021

ojeda May 17, 2021

ojeda May 17, 2021

SimonSapin commented May 19, 2021

ijackson commented May 19, 2021

SimonSapin commented May 19, 2021

m-ou-se Jun 5, 2021

ijackson Jun 7, 2021

ijackson commented Jun 7, 2021

bors commented Jul 2, 2021

m-ou-se commented Jul 5, 2021

bors commented Jul 5, 2021

ijackson commented Jul 5, 2021 via email

aborts: Clarify documentation and comments #85377

aborts: Clarify documentation and comments #85377

Conversation

ijackson commented May 16, 2021 • edited by JohnTitor Loading

rust-highfive commented May 16, 2021

ijackson commented May 16, 2021

Urgau commented May 16, 2021

ijackson commented May 16, 2021

ojeda May 17, 2021

Choose a reason for hiding this comment

ojeda May 17, 2021

Choose a reason for hiding this comment

SimonSapin commented May 19, 2021

ijackson commented May 19, 2021

SimonSapin commented May 19, 2021

m-ou-se Jun 5, 2021

Choose a reason for hiding this comment

ijackson Jun 7, 2021

Choose a reason for hiding this comment

ijackson commented Jun 7, 2021

bors commented Jul 2, 2021

m-ou-se commented Jul 5, 2021

bors commented Jul 5, 2021

ijackson commented Jul 5, 2021 via email

ijackson commented May 16, 2021 •

edited by JohnTitor

Loading