panic! -> abort #80

japaric · 2016-09-30T22:36:23Z

closes #79

japaric · 2016-09-30T22:36:31Z

r? @alexcrichton

@homunkulus try

homunkulus · 2016-09-30T22:36:34Z

⌛ Trying commit 88862e4 with merge 88862e4...

alexcrichton · 2016-09-30T22:38:07Z

r+ from me

Want to add this to the CI though to ensure there are no references to the panic symbol?

japaric · 2016-09-30T22:41:25Z

@alexcrichton sure thing

japaric · 2016-10-01T04:00:08Z

Hmm, so, with these changes, librustc_builtins.rlib still has undefined references to panic when built in release mode though I don't see any branch/jump instructions to it in the disassembly.

But, without these changes, if I build intrinsics.rs in release with LTO enabled then I can reproduce the linker errors seen in #79. And the errors disappear when I apply the changes in this PR. Would that suffice as a test?

japaric · 2016-10-01T04:08:54Z

And the errors disappear when I apply the changes in this PR

Actually just the changes in this PR are not enough to remove all the panics. I'm going to homu try this so you can see the errors.

japaric · 2016-10-01T04:12:10Z

@homunkulus try

homunkulus · 2016-10-01T04:12:12Z

⌛ Trying commit 7c3b64f with merge 7c3b64f...

homunkulus · 2016-10-01T04:32:58Z

💔 Test failed - status-travis

alexcrichton · 2016-10-03T07:06:19Z

Panics are also inserted for / and % operations by default to check if the rhs would cause a fault (e.g. 0 or overflow w/ INT_MIN). Maybe that's happening here?

japaric · 2016-10-04T00:22:14Z

Hmm, does that means that we have to create wrappers around primitives that intrinsics::abort on bad inputs (division by zero) or e.g. use intrinsics::unchecked_div on good inputs?

alexcrichton · 2016-10-04T03:33:52Z

Ideally unchecked_div I think because otherwise this would rely on optimizations to remove references to the symbols.

japaric · 2016-10-06T01:47:38Z

@homunkulus try

japaric · 2016-10-06T01:50:49Z

@homunkulus try

homunkulus · 2016-10-06T01:50:54Z

⌛ Trying commit ee24d69 with merge ee24d69...

homunkulus · 2016-10-06T02:14:52Z

☀️ Test successful - status-appveyor, status-travis
State: approved= try=True

japaric · 2016-10-06T02:18:34Z

@alexcrichton I added a few intrinsics::abort here and there where division by zero may occur and the linker errors are gone ...

alexcrichton · 2016-10-06T16:44:44Z

Hm yeah that works for release mode, but for debug mode the symbols would stick around?

japaric · 2016-10-07T15:22:50Z

I changed all the division and modulo operations with macros that abort on bad inputs. This makes the LTO work with the dev profile iff the debug-assertions are turned off ...

If we want LTO to work with debug assertions on, we'll have to change all the operations that may overflow with an abort on overflow version

japaric · 2016-10-07T15:23:26Z

@homunkulus try

homunkulus · 2016-10-07T15:23:29Z

⌛ Trying commit 34cd40f with merge 5766097...

panic! -> abort closes #79

homunkulus · 2016-10-07T16:06:49Z

☀️ Test successful - status-appveyor, status-travis
State: approved= try=True

alexcrichton · 2016-10-07T16:10:41Z

Cargo.toml

@@ -25,4 +25,7 @@ compiler-rt = { path = "compiler-rt" }
 c = []
 weak = ["rlibc/weak"]

+[profile.dev]
+debug-assertions = false


It seems desirable to leave this on, right? It's caught bugs in overflows and such?

alexcrichton · 2016-10-07T16:12:22Z

src/lib.rs

+            let b = $b;
+
+            if b == 0 {
+                intrinsics::abort()


You may want to make these ::core::intrinsics::... to avoid imports in the other modules.

alexcrichton · 2016-10-07T16:12:58Z

If we want LTO to work with debug assertions on, we'll have to change all the operations that may overflow with an abort on overflow version

What's the error message if debug assertions are turned on?

japaric · 2016-10-07T16:25:16Z

What's the error message if debug assertions are turned on?

Building src/bin/intrinsics.rs for arm-unknown-linux-gnueabi using the dev profile (+debug-assertions) + LTO:

error: linking with `arm-linux-gnueabi-gcc` failed: exit code: 1
  |
  = note: "arm-linux-gnueabi-gcc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-L" "/rust/lib/rustlib/arm-unknown-linux-gnueabi/lib" "/target/arm-unknown-linux-gnueabi/debug/intrinsics.0.o" "-o" "/target/arm-unknown-linux-gnueabi/debug/intrinsics" "-Wl,--gc-sections" "-pie" "-nodefaultlibs" "-L" "/target/arm-unknown-linux-gnueabi/debug/deps" "-L" "/target/arm-unknown-linux-gnueabi/debug/build/rustc_builtins-3d8222040fb6ac11/out" "-L" "/rust/lib/rustlib/arm-unknown-linux-gnueabi/lib" "-Wl,-Bstatic" "-Wl,-Bdynamic" "/tmp/rustc.1PHTT1MGYLXp/librustc_builtins.rlib" "-l" "c" "-l" "m" "-l" "rt" "-l" "util"
  = note: /tmp/rustc.1PHTT1MGYLXp/librustc_builtins.rlib(rustc_builtins.0.o): In function `rustc_builtins::int::udiv::__udivmoddi4':
/checkout/src/int/udiv.rs:210: undefined reference to `core::panicking::panic::h7215d66c75dd9558'
/checkout/src/int/udiv.rs:217: undefined reference to `core::panicking::panic::h7215d66c75dd9558'
/checkout/src/int/udiv.rs:220: undefined reference to `core::panicking::panic::h7215d66c75dd9558'
/checkout/src/int/udiv.rs:220: undefined reference to `core::panicking::panic::h7215d66c75dd9558'
/checkout/src/int/udiv.rs:221: undefined reference to `core::panicking::panic::h7215d66c75dd9558'
/tmp/rustc.1PHTT1MGYLXp/librustc_builtins.rlib(rustc_builtins.0.o):/checkout/src/int/udiv.rs:198: more undefined references to `core::panicking::panic::h7215d66c75dd9558' follow
collect2: error: ld returned 1 exit status


error: aborting due to previous error

error: Could not compile `rustc_builtins`.

japaric · 2016-10-07T16:26:00Z

/checkout/src/int/udiv.rs:210

is: if sr > u32::bits() - 1 {

/checkout/src/int/udiv.rs:217

is sr += 1;

and so on.

alexcrichton · 2016-10-07T16:26:29Z

Oh those just need to be wrapping ops, right? Or explicitly .checked_foo with an abort?

japaric · 2016-10-07T16:46:52Z

It seems desirable to leave this on, right? It's caught bugs in overflows and such?

I don't know if these have helped catch bugs but we do have seen debug assertions related to overflows while testing the intrinsics.

Oh those just need to be wrapping ops, right? Or explicitly .checked_foo with an abort?

I prefer checked + abort. Although not as nice as panics, that way still tells you there's overflow going on so you can look at the offending code and decide if overflowing makes sense. OTOH, silent overflows sound like a bad default.

alexcrichton · 2016-10-07T16:49:28Z

Yeah I agree that we probably want to catch overflows as much as possible. Want to try out the checked + abort strategy?

japaric · 2016-10-07T16:50:56Z

Hmm, would it make sense to change all the arithmetic operations to checked+abort?

japaric · 2016-10-07T16:58:45Z

Hmm, it seems like the overflowing_, checked_ variants have panic!s inside them?

I changed the contents of the macros to .checked_*().unwrap_or_else(|| abort()) and I get:

error: linking with `arm-linux-gnueabi-gcc` failed: exit code: 1
  |
  = note: "arm-linux-gnueabi-gcc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-L" "/rust/lib/rustlib/arm-unknown-linux-gnueabi/lib" "/target/arm-unknown-linux-gnueabi/debug/intrinsics.0.o" "-o" "/target/arm-unknown-linux-gnueabi/debug/intrinsics" "-Wl,--gc-sections" "-pie" "-nodefaultlibs" "-L" "/target/arm-unknown-linux-gnueabi/debug/deps" "-L" "/target/arm-unknown-linux-gnueabi/debug/build/rustc_builtins-3d8222040fb6ac11/out" "-L" "/rust/lib/rustlib/arm-unknown-linux-gnueabi/lib" "-Wl,-Bstatic" "-Wl,-Bdynamic" "/tmp/rustc.ZoLec8ROwmlU/librustc_builtins.rlib" "-l" "c" "-l" "m" "-l" "rt" "-l" "util"
  = note: /tmp/rustc.ZoLec8ROwmlU/librustc_builtins.rlib(rustc_builtins.0.o): In function `core::num::{{impl}}::overflowing_div':
/buildslave/rust-buildbot/slave/nightly-dist-rustc-cross-host-linux/build/src/libcore/num/mod.rs:1013: undefined reference to `core::panicking::panic::h7215d66c75dd9558'
/buildslave/rust-buildbot/slave/nightly-dist-rustc-cross-host-linux/build/src/libcore/num/mod.rs:1013: undefined reference to `core::panicking::panic::h7215d66c75dd9558'
/tmp/rustc.ZoLec8ROwmlU/librustc_builtins.rlib(rustc_builtins.0.o): In function `core::num::{{impl}}::checked_div':
/buildslave/rust-buildbot/slave/nightly-dist-rustc-cross-host-linux/build/src/libcore/num/mod.rs:1691: undefined reference to `core::panicking::panic::h7215d66c75dd9558'
/tmp/rustc.ZoLec8ROwmlU/librustc_builtins.rlib(rustc_builtins.0.o): In function `core::num::{{impl}}::checked_rem':
/buildslave/rust-buildbot/slave/nightly-dist-rustc-cross-host-linux/build/src/libcore/num/mod.rs:1712: undefined reference to `core::panicking::panic::h7215d66c75dd9558'
/tmp/rustc.ZoLec8ROwmlU/librustc_builtins.rlib(rustc_builtins.0.o): In function `core::num::{{impl}}::checked_div':
/buildslave/rust-buildbot/slave/nightly-dist-rustc-cross-host-linux/build/src/libcore/num/mod.rs:1691: undefined reference to `core::panicking::panic::h7215d66c75dd9558'
/tmp/rustc.ZoLec8ROwmlU/librustc_builtins.rlib(rustc_builtins.0.o):/checkout/src/int/udiv.rs:210: more undefined references to `core::panicking::panic::h7215d66c75dd9558' follow
collect2: error: ld returned 1 exit status


error: aborting due to previous error

error: Could not compile `rustc_builtins`.

alexcrichton · 2016-10-07T17:08:15Z

Hmm, would it make sense to change all the arithmetic operations to checked+abort?

I think to get rid of references to the panic symbol, yeah.

Hmm, it seems like the overflowing_, checked_ variants have panic!s inside them?

Oh gah it looks like this is a bug in the standard library, they still use the standard / operators, which in turn have panics. I think we'd have to upstream a patch to use the intrinsics without panics in these cases.

japaric · 2016-10-07T19:39:11Z

Oh gah it looks like this is a bug in the standard library, they still use the standard / operators, which in turn have panics.

The implementation of overflowing_div, which is called by checked_div, looks like this:

        pub fn overflowing_div(self, rhs: Self) -> (Self, bool) {
            if self == Self::min_value() && rhs == -1 {
                (self, true)
            } else {
                unsafe {
                    (self / rhs, false)
                }
            }
        }

but we can't change the self / rhs to unchecked_div(self, rhs) because rhs = 0 would be UB and this function doesn't protect against division by zero. :-/

alexcrichton · 2016-10-07T19:45:23Z

Hm ok, perhaps a suite of checked ops could be added to this crate?

alexcrichton · 2016-10-07T19:45:36Z

Note that this should likely be fixed upstream as well

japaric · 2016-10-07T20:55:59Z

Ugh, I've been using macros to do the checked.unwrap_or(abort) thing but pretty much every single operation has to be replaced by that macro, which in results in very verbose code. I think that we instead should use newtypes with abort on bad inputs/overflow semantics to avoid obfuscating the code. But that's going to require a bunch of boilerplate instead.

Can we punt fixing debug (-debug-assertions) + LTO and land this PR as it's (modulo nits)?

alexcrichton · 2016-10-07T21:06:45Z

Hm so disabling debug assertions here won't actually work for everyone else. That is, the top-level project decides this, not the crate itself. Put another way this won't fix #79 (the intention, right?) because the debug-assertions = false will be ignored.

I'd probably recommend keeping the same form of style here and avoiding macros. It's already kinda hard to follow with macros generating functions :(.

Perhaps some new types could be added whose operators (like + and such) are overloaded to abort on overflows?

japaric · 2016-10-07T21:24:54Z

Hm so disabling debug assertions here won't actually work for everyone else.

Oh, yes. I'm aware. That what actually just for testing this on the debug-assertion-less scenario.

Put another way this won't fix #79 (the intention, right?)

No, this won't totally fix #79. But it does fix the LTO + release case which is, I think, the combination that most people care about. @FenrirWolf, are you mostly interested in profile.release + LTO? Or is profile.dev + LTO also important to you?

Perhaps some new types could be added whose operators (like + and such) are overloaded to abort on overflows?

Certainly that's a better fix and I've also suggested it above. What I wanted to do is fix profile.release + LTO now and then implement the newtype approach to fix the general case.

FenrirWolf · 2016-10-07T21:51:34Z

@japaric I haven't been using profile.dev + LTO since that combination was producing an LLVM error for a long time. Though now that you've reminded me, I just checked to see if that error still happens on the latest nightly, and it doesn't. So that's cool.

Either way, I've mainly been using profile.release + LTO. I can technically go without that too if need be.

alexcrichton · 2016-10-07T22:21:40Z

@japaric ok if this is targeted at fixing the release + LTO case, sounds good to me! Let's go ahead and land to then fixup the debug case later

closes #79

both prevent LLVM from optimizing away the intrinsics but the former doesn't produce an `intrinsics` binary that segfaults

japaric · 2016-10-07T23:30:04Z

@alexcrichton r?
@homunkulus try

homunkulus · 2016-10-07T23:30:10Z

⌛ Trying commit 96ddd4e with merge 96ddd4e...

alexcrichton · 2016-10-07T23:39:02Z

r+

homunkulus · 2016-10-07T23:55:06Z

☀️ Test successful - status-appveyor, status-travis
State: approved= try=True

japaric force-pushed the abort branch from 88862e4 to 7c3b64f Compare October 1, 2016 04:12

japaric force-pushed the abort branch from 7c3b64f to ee24d69 Compare October 6, 2016 01:47

japaric force-pushed the abort branch from ee24d69 to 34cd40f Compare October 7, 2016 15:19

japaric pushed a commit that referenced this pull request Oct 7, 2016

Auto merge of #80 - japaric:abort, r=<try>

5766097

panic! -> abort closes #79

alexcrichton reviewed Oct 7, 2016

View reviewed changes

Jorge Aparicio added 6 commits October 7, 2016 18:19

panic! -> abort

534d22e

closes #79

check that we don't have any call to panic in our implementations

a2ffc79

insert more abort() calls where division by zero may occur

a21fb95

get profile.dev (-debug-assertions) + LTO working

fd69beb

fix unsafe warnings

c824035

use test::black_box instead of ptr::*_volatile

96ddd4e

both prevent LLVM from optimizing away the intrinsics but the former doesn't produce an `intrinsics` binary that segfaults

japaric force-pushed the abort branch from 34cd40f to 96ddd4e Compare October 7, 2016 23:29

japaric merged commit c56faf2 into master Oct 7, 2016

japaric deleted the abort branch October 7, 2016 23:55

homunkulus mentioned this pull request Oct 7, 2016

Upstream #94

Closed

FenrirWolf mentioned this pull request Oct 7, 2016

LTO causes undefined references to core::panicking::panic #79

Closed

panic! -> abort #80

panic! -> abort #80

Conversation

japaric commented Sep 30, 2016

japaric commented Sep 30, 2016

homunkulus commented Sep 30, 2016

alexcrichton commented Sep 30, 2016

japaric commented Sep 30, 2016

japaric commented Oct 1, 2016

japaric commented Oct 1, 2016

japaric commented Oct 1, 2016

homunkulus commented Oct 1, 2016

homunkulus commented Oct 1, 2016

alexcrichton commented Oct 3, 2016

japaric commented Oct 4, 2016

alexcrichton commented Oct 4, 2016

japaric commented Oct 6, 2016

japaric commented Oct 6, 2016

homunkulus commented Oct 6, 2016

homunkulus commented Oct 6, 2016

japaric commented Oct 6, 2016

alexcrichton commented Oct 6, 2016

japaric commented Oct 7, 2016

japaric commented Oct 7, 2016

homunkulus commented Oct 7, 2016

homunkulus commented Oct 7, 2016

alexcrichton Oct 7, 2016

Choose a reason for hiding this comment

alexcrichton Oct 7, 2016

Choose a reason for hiding this comment

alexcrichton commented Oct 7, 2016

japaric commented Oct 7, 2016

japaric commented Oct 7, 2016

alexcrichton commented Oct 7, 2016

japaric commented Oct 7, 2016

alexcrichton commented Oct 7, 2016

japaric commented Oct 7, 2016

japaric commented Oct 7, 2016 • edited Loading

alexcrichton commented Oct 7, 2016

japaric commented Oct 7, 2016

alexcrichton commented Oct 7, 2016

alexcrichton commented Oct 7, 2016

japaric commented Oct 7, 2016

alexcrichton commented Oct 7, 2016

japaric commented Oct 7, 2016

FenrirWolf commented Oct 7, 2016

alexcrichton commented Oct 7, 2016

japaric commented Oct 7, 2016

homunkulus commented Oct 7, 2016

alexcrichton commented Oct 7, 2016

homunkulus commented Oct 7, 2016

japaric commented Oct 7, 2016 •

edited

Loading