book: use abort() over loop {} for panic #38138

hanna-kruppe · 2016-12-03T01:15:29Z

Due to #28728 loop {} is very risky and can lead to fun debugging experiences such as #38136. Besides, aborting is probably better behavior than an infinite loop.

r? @steveklabnik

steveklabnik · 2016-12-04T03:57:34Z

I would like to hear what @rust-lang/libs thinks about this, and possibly other teams. loop {} is usually the idiom here.

oli-obk · 2016-12-04T09:21:53Z

How is loop{} risky? The mentioned loop optimizations only trigger for loops thst have a break/return in them anywhere. Your given example sounds like it is an issue with inline(never) not actually preventing inlining (since moving it to a different crate works, I assume until you use lto)

hanna-kruppe · 2016-12-04T10:58:16Z

loop {} is usually the idiom here.

Maybe for producing a ! value in general, but in these cases we specifically have a panic, and it makes much more sense to me to emulate the panic=abort behavior than to get stuck on panics.

The mentioned loop optimizations only trigger for loops thst have a break/return in them anywhere.

I don't know where you get that from. All the examples in that thread involve a loop {} or while true {}, as that is the kind of loop that LLVM optimizes out. I'm sure you could also trigger it on loops like loop if false { break; } or similar ones, as long as they're provably infinite and side-effect-free. It's not a case of LLVM getting confused by redundant control flow instructions or something, it's a case of the C++ standard making infinite loops with no side effects UB and LLVM running with that.

Your given example sounds like it is an issue with inline(never) not actually preventing inlining (since moving it to a different crate works, I assume until you use lto)

I have multiple problem with this perspective:

Regardless of how you want to slice it, the code was broken with loop {} and wasn't broken without it.
Putting blame on inline(never) or LTO only begins to make sense if we accept that loop {} is broken and search for workarounds. I'd rather just avoid the need for mitigation to begin with.
inline(never) isn't really at fault here, it only means that the code shall not be duplicated into callers, it doesn't mean that inter-procedural optimizations and analyses can't look at the function contents.

oli-obk · 2016-12-04T12:33:45Z

In all my experiments, while true {} was broken and loop {} was not. That's also how I interpret the llvm issue. This must be a new issue I don't know about.

Quoting the bug report:

To be completely clear, my understanding of our current policy is this: As a gentleman's agreement, LLVM will not "break" the kinds of obvious infinite loops written by programmers, in C, for the purposes of testing, causing threads to not terminate, etc. We might, however, assume termination otherwise. We don't , however, have a formal policy one way or the other.

And the relevant bug for pure infinite loops https://llvm.org/bugs/show_bug.cgi?id=965 is still open, thus I assume it should not be ub

You are right of course, inline is not the culprit, but it's still a bug that moving the function into another compilation unit changes behaviour.

hanna-kruppe · 2016-12-04T12:46:00Z

while true {} and loop {} generate the exact same LLVM IR even in debug mode (modulo debug info an unnecessary alloca outside the loop) on current stable: https://play.rust-lang.org/?gist=c7b70df19ed198055e813dc7f080a2ce&version=stable&backtrace=0
Even if rustc's codegen was dumber, the IR would be the same after some minor clean-up optimization passes.

Your experiments might have been too simplistic (e.g., no surrounding code that could be optimized further by replacing an infinite loop with unreachable) to trigger problems. But if you have an example at hand which is compiled correctly with loop {} and miscompiled with while true {}, it would make an interesting addition to the discussion over in #28728

You are right of course, inline is not the culprit, but it's still a bug that moving the function into another compilation unit changes behaviour.

I don't think that deserves to be called a separate bug. We wouldn't even be asking the question if not for the principal bug (infinite loop == UB). But this is mostly a semantic quibble. In any case, it's not worth a separate issue because the problem disappears if the infinite loop == UB issue is fixed, and the general principle that splitting code into several crates can influence optimizations is well known and unavoidable.

oli-obk · 2016-12-04T12:47:35Z

Your playground link does not use while true

hanna-kruppe · 2016-12-04T12:49:39Z

Ooops, sorry. Fixed.

oli-obk · 2016-12-04T12:49:43Z

See #37088 for an example that used to do ub with while true and no ub with loop.

Edit: i guess the mir optimizations changed that.

hanna-kruppe · 2016-12-04T12:54:05Z

There are multiple programs in #37088, all of them UB if I read the description right, some using loop and others using while true. Can you please put up playgrounds of the one program you mean, once with loop and once with while true?

oli-obk · 2016-12-04T13:00:49Z

I'm on mobile right now and can't reproduce it on the stable playground, so something has changed. I'll try with an older compiler once I'm back on a pc

steveklabnik · 2016-12-04T19:34:52Z

Maybe for producing a ! value in general, but in these cases we specifically have a panic, and it makes much more sense to me to emulate the panic=abort behavior than to get stuck on panics.

Almost every implementation and bit of docs I've ever seen on this use loop initially. As I said, maybe my impression here is wrong. I'm also not saying we shouldn't do this, exactly, but that I want to make sure it's actually the right call.

hanna-kruppe · 2016-12-04T20:06:45Z

@steveklabnik

Almost every implementation and bit of docs I've ever seen on this use loop initially.

What exactly do you mean by "this" here? no_std panic implementations? Functions that return !?

steveklabnik · 2016-12-04T20:22:46Z

Documentation and examples around no_std panic implementations.

alexcrichton · 2016-12-05T11:05:06Z

Ideally these intrinsics are forgotten details of the compiler as no std transitions to panic=abort. Unfortunately though doc tests are hard to write as such so we're forced to do something here. Can we just hide these functions and recommend panic=abort?

I don't really like working around this llvm bug but a bug is a bug and we should strive to document working code here.

hanna-kruppe · 2016-12-06T16:44:37Z

@alexcrichton I tried to build a no_std executable with panic=abort in place of these intrinsics and couldn't get it to work. Is that option supported today? If so, that indeed sounds like what we should be recommending, even if we can't doctest it (we could still have a run-pass test).

steveklabnik · 2016-12-06T20:12:58Z

Yeah, you still have to implement at least rust_begin_panic even with panic=abort.

alexcrichton · 2016-12-08T01:34:18Z

@rkruppe to get it to truly work you'll need to recompile libcore/libstd as well because all libraries need to be compiled with -Cpanic=abort to avoid the need for the intrinsics.

hanna-kruppe · 2016-12-08T08:48:18Z

Oh right, of course. Didn't notice that because I'm actually cross-compiling to a custom target so I'm building core as a normal dependency anyway (i.e., I have core = { path="..." } in Cargo.toml). I'll need to experiment a bit more with real #![no_std]` setup then, and then think about how to if and how to put that in the book.

hanna-kruppe · 2016-12-08T08:50:19Z

Alternatively, shouldn't one in principle be able to link to libpanic_unwind in a no-std context? Everything blew up when I tried that on a whim, but it seems like something that ought to work (but again that was in a cross-compiling #![no_core] context), and should remove the need for even rust_begin_panic.

alexcrichton · 2016-12-09T01:44:10Z

In theory, yes, linking panic_unwind manually should work, but there's likely some weirdness which would cause problems.

steveklabnik · 2016-12-22T21:37:47Z

So: what to do with this patch?

hanna-kruppe · 2016-12-23T12:19:24Z

First of all, I tried panic=abort + linking panic_abort and it still missed a lang item (panic_fmt), as @alexcrichton feared. There are also further issues with recommending panic=abort right now:

As far as I can tell, panic=abort is not explained anywhere in the book. I couldn't even find and detailed explanation of unwinding or panics in general. This makes it problematic to just write 'use panic=abort'.
Additionally, recommending one thing but doc-testing another thing has problems: the panic=abort route could regress without us noticing, and people who click the playground button will be confused.

In conclusion I'm inclined to argue this patch as-is (or at least a close variation, e.g. using libc abort instead of the intrinsic) represents the best we're gonna get for a while, i.e. until someone writes a chapter on panics and extends rustdoc to allow passing -C options to tests.

steveklabnik

Sorry for the delay here, @rkruppe . r=me after this nit.

steveklabnik · 2017-01-04T22:31:02Z

src/doc/book/lang-items.md

 #![no_std]
+use core::intrinsics::abort;


could this be use core::intrinsics...

steveklabnik · 2017-01-04T22:31:17Z

src/doc/book/lang-items.md

@@ -58,7 +55,7 @@ fn main(argc: isize, argv: *const *const u8) -> isize {
 }

 #[lang = "eh_personality"] extern fn rust_eh_personality() {}
-#[lang = "panic_fmt"] extern fn rust_begin_panic() -> ! { loop {} }
+#[lang = "panic_fmt"] extern fn rust_begin_panic() -> ! { unsafe { abort() } }


and here, intrinsics::abort? This is the standard style for importing functions.

Due to rust-lang#28728 loop {} is very risky and can lead to fun debugging experiences like in rust-lang#38136. Besides, aborting is probably better behavior than an infinite loop.

hanna-kruppe · 2017-01-04T22:54:42Z

Addressed nit.

steveklabnik · 2017-01-04T23:21:43Z

@bors: r+ rollup

thanks!

bors · 2017-01-04T23:21:44Z

📌 Commit 893f42a has been approved by steveklabnik

@steveklabnik

book: use abort() over loop {} for panic Due to rust-lang#28728 `loop {}` is very risky and can lead to fun debugging experiences such as rust-lang#38136. Besides, aborting is probably better behavior than an infinite loop. r? @steveklabnik

bors · 2017-01-09T13:00:09Z

⌛ Testing commit 893f42a with merge ec9ae8c...

bors · 2017-01-09T13:55:39Z

💔 Test failed - status-travis

alexcrichton · 2017-01-09T16:57:55Z

@bors: retry * sccache hang?

…

On Mon, Jan 9, 2017 at 5:55 AM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/190240467> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95IclnyXzaeBvC1Fy3lOpByw1R_38ks5rQjxdgaJpZM4LDJdg> .

bors · 2017-01-09T17:06:36Z

⌛ Testing commit 893f42a with merge aa3ae13...

bors · 2017-01-09T17:18:52Z

💔 Test failed - status-appveyor

alexcrichton · 2017-01-09T21:17:25Z

@bors: retry * network failure

…

On Mon, Jan 9, 2017 at 9:18 AM, bors ***@***.***> wrote: 💔 Test failed - status-appveyor <https://ci.appveyor.com/project/rust-lang/rust/build/1.0.1476> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38138 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95GMB1s8kuickq2bzw6vL72jnvwT1ks5rQmv-gaJpZM4LDJdg> .

bors · 2017-01-10T06:50:35Z

⌛ Testing commit 893f42a with merge 78c892d...

@steveklabnik

book: use abort() over loop {} for panic Due to #28728 `loop {}` is very risky and can lead to fun debugging experiences such as #38136. Besides, aborting is probably better behavior than an infinite loop. r? @steveklabnik

bors · 2017-01-10T08:49:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: steveklabnik
Pushing 78c892d to master...

rust-highfive assigned steveklabnik Dec 3, 2016

steveklabnik suggested changes Jan 4, 2017

View reviewed changes

book: use abort() over loop {} for panic

893f42a

Due to rust-lang#28728 loop {} is very risky and can lead to fun debugging experiences like in rust-lang#38136. Besides, aborting is probably better behavior than an infinite loop.

hanna-kruppe force-pushed the no_std-no_loop branch from 6687dcc to 893f42a Compare January 4, 2017 22:54

steveklabnik approved these changes Jan 4, 2017

View reviewed changes

steveklabnik mentioned this pull request Jan 4, 2017

Rollup of 11 pull requests #38834

Closed

frewsxcv mentioned this pull request Jan 9, 2017

Rollup of 19 pull requests #38943

Closed

bors merged commit 893f42a into rust-lang:master Jan 10, 2017

book: use abort() over loop {} for panic #38138

book: use abort() over loop {} for panic #38138

Conversation

hanna-kruppe commented Dec 3, 2016

steveklabnik commented Dec 4, 2016

oli-obk commented Dec 4, 2016 • edited Loading

hanna-kruppe commented Dec 4, 2016 • edited Loading

oli-obk commented Dec 4, 2016 • edited Loading

hanna-kruppe commented Dec 4, 2016 • edited Loading

oli-obk commented Dec 4, 2016

hanna-kruppe commented Dec 4, 2016

oli-obk commented Dec 4, 2016 • edited Loading

hanna-kruppe commented Dec 4, 2016

oli-obk commented Dec 4, 2016

steveklabnik commented Dec 4, 2016

hanna-kruppe commented Dec 4, 2016

steveklabnik commented Dec 4, 2016

alexcrichton commented Dec 5, 2016

hanna-kruppe commented Dec 6, 2016 • edited Loading

steveklabnik commented Dec 6, 2016

alexcrichton commented Dec 8, 2016

hanna-kruppe commented Dec 8, 2016

hanna-kruppe commented Dec 8, 2016

alexcrichton commented Dec 9, 2016

steveklabnik commented Dec 22, 2016

hanna-kruppe commented Dec 23, 2016

steveklabnik left a comment

Choose a reason for hiding this comment

steveklabnik Jan 4, 2017

Choose a reason for hiding this comment

steveklabnik Jan 4, 2017

Choose a reason for hiding this comment

hanna-kruppe commented Jan 4, 2017 • edited Loading

steveklabnik commented Jan 4, 2017

bors commented Jan 4, 2017

bors commented Jan 9, 2017

bors commented Jan 9, 2017

alexcrichton commented Jan 9, 2017 via email

bors commented Jan 9, 2017

bors commented Jan 9, 2017

alexcrichton commented Jan 9, 2017 via email

bors commented Jan 10, 2017

bors commented Jan 10, 2017

oli-obk commented Dec 4, 2016 •

edited

Loading

hanna-kruppe commented Dec 4, 2016 •

edited

Loading

oli-obk commented Dec 4, 2016 •

edited

Loading

hanna-kruppe commented Dec 4, 2016 •

edited

Loading

oli-obk commented Dec 4, 2016 •

edited

Loading

hanna-kruppe commented Dec 6, 2016 •

edited

Loading

hanna-kruppe commented Jan 4, 2017 •

edited

Loading