Add debug_assert_nounwind to unchecked_{add,sub,neg,mul,shl,shr} methods (haunted PR)

[clarfonthey]

This ensures that these preconditions are actually checked in debug mode, and hopefully should let people know if they messed up. I've also replaced the calls (I could find) in the code that use these intrinsics directly with those that use these methods, so that the asserts actually apply.

More discussions on people misusing these methods in the tracking issue: #85122.

[rustbot]

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

[clarfonthey]

I have honestly no idea why this is failing due to… an error in the typenum crate? What's even going on there?

[clarfonthey]

@rustbot author

[Noratrieb]

and yet another attempt at finding undefined behavior that finds miscompilcations instead. awesome!

[saethlin]

(I'm looking into this and shitposting about it)

[saethlin]

Right-shifts of integers with all bits set is being incorrectly evaluated by const eval as a shift left. Here's a minimized reproducer:

const RSHIFT: u8 = u8::MAX >> 1;

fn main() {
    // At runtime we get the right value
    // Need an intermediate variable or we get const promoted
    let v = u8::MAX;
    println!("runtime: {}", v >> 1);
    println!("const: {}", RSHIFT);
}

In this branch, it prints

runtime: 127
const: 254

[clarfonthey]

Thank you so much for helping investigate this. I had slowly been running tests locally but was truly boggled by what I was getting.

[saethlin]

I think the root cause here is just a copy-paste error. One character difference but a whole world away...

[bors]

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

[clarfonthey]

I'll try and update this soon to resolve the conflicts, but I probably won't have it working soon.

One of the unstable-in-stable lints was really bugging me by being difficult to resolve, and that's why I started working on #117571 instead. So, I'm going to mark this as blocked by that unless someone else can help determine why unchecked_neg is just breaking inside the pointer methods.

[clarfonthey]

@rustbot blocked

[clarfonthey]

IMO cg_clif no longer needs to add this trap if the standard library already does the abort.

Isn't this wrong, since this isn't an always-run check? I'm not familiar with cg_clif at all so I don't know the purpose of this trap.

[bjorn3]

cg_clif is meant for debug mode. The purpose of the trap would have been to act as sanity check for UB, which is exactly what this PR will do when debug assertions are enabled. Not emitting the check is harmless as cg_clif doesn't exploit the UB anyway. (in cranelift overflow is defined as wrapping without a way to opt into UB on overflow)

[Noratrieb]

@bors retry

[bors]

⌛ Trying commit 9c4c647 with merge a8a9d85...

[saethlin]

I think the previous try build got lost somehow with the tree closure and ghcr.io outage. When I was checking on the queue I noticed bors still though the try build was running even though it definitely finished: https://bors.rust-lang.org/queue/rust

I'll try to keep an eye on this

[Noratrieb]

I'm going to close this PR tentatively to try to stop bors from doing weird things and restoring sanity. Will open it again later. (https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/bors.20troubles)

[Noratrieb]

It's out of the queue now. Should be fine now, hopefully.

[Noratrieb]

oh no oh no oh no it's back in the queue, not even marked as try.

[clarfonthey]

I'm fine with opening a new PR as an option if you want to go with that one.

[Noratrieb]

that's probably the easiest solution

[clarfonthey]

#121571

[RalfJung]

@Nilstrieb Is there a bors issue somewhere tracking what the heck is happening here? If not, could you open one?

[Noratrieb]

sure