Improve logging of abort

[Thomasdezeeuw]

While trying to debug #52629, I've hit an abort() condition when an Arc count wrapped around (it was being dropped twice). At least on macOS it causes the following message to be printed when testing:

error: process didn't exit successfully: `/binary/path` (signal: 4, SIGILL: illegal instruction)

To me this doesn't say that the process was aborted, in fact I though this was a problem with the code being generated. Only after running the code with lldb did I find out that this was actually caused by a call to abort.

So my suggestion is to add some minimal logging that the process was aborted, atleast in debug mode. Just a minimal "aborted process" would done. Maybe this needs a RFC, but I though to start small.

P.S: is the printed error message any better on other Oses?

[ehuss]

P.S: is the printed error message any better on other Oses?

Not really. When calling intrinsics::abort(), the rust process itself doesn't print anything, so the message depends on the calling process to print something. Things like bash will just say "Illegal instruction" (since all it sees is SIGILL). I just ran into this while working on a panic handler. Fortunately it does print a message before going down:

rust/src/libstd/panicking.rs

Lines 456 to 460 in 6bb50f5

	if panics > 2 {
	util::dumb_print(format_args!("thread panicked while processing \
	panic. aborting.\n"));
	unsafe { intrinsics::abort() }
	}

but I think "illegal instruction" is confusing. I'm curious why llvm.trap uses the ud2 instruction on x86 instead of calling abort().

[Thomasdezeeuw]

This seems to be resolved. Maybe in 1b1bf24? Thanks @ijackson.

[ijackson]

I don't think any of the things I have done have improved this area, yet, unfortunately. And I think the issue is still present (although I don't have a repro).

In #85377 I am trying to improve the docs on std::process::abort vs core::intrinsics::abort (although I seem to have somehow dropped the ball there; I've put it back on my todo list).

I have another branch where I am trying to make things call std::process::abort (ie abort(3)) more but I had trouble with my new tests failing. A related commit is "panic aborts: Use process::abort() not intrinsics::abort() " ijackson@473313f in that branch which changes panic to use std::process::abort instead.

Fom the messages above, Arc calls abort directly. Indeed I see use core::intrinsics::abort; in library/alloc/src/sync.rs. Fixing that is not so easy because Arc is in alloc, so doesn't even have access to std::process::abort.

So, err, sorry, but I think this problem is still outstanding. Some of these things are on my todo list (having apparently fallen out of my file where I keep these things) but I haven't worked on it recently, and I haven't done a survey of all the abort calls.

[Thomasdezeeuw]

@ijackson I'm not sure it was you personally, but multiple situations have certainly improved:

• Double panics now print an backtrack and an error saying there was a panic while panicking: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=3eb50efa8b31b58970167ed6c50f4c76. If I remember correctly this wasn't the case before.
• Arc deosn't double drop its content anymore even when it is double dropped, nor does it call abort: https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=a6ade9d0eee09e4480203b40d7c59d9c. However that is because it doesn't check the strong count anymore, so it's not quite the same.

The only other abort I can find is in Packet::clone_chan in

rust/library/std/src/sync/mpsc/shared.rs

Line 357 in 117799b

abort();

, which is a bit difficult to hit (cloning the channel isize::MAX times).

You're changes in #85377 do seem like a good addition. But I think this issue is correctly closed, maybe another more specific issue can be opened for discouraging use of intrinsics::abort in favour of rocess::abort.