Division by nonzero


#1

I’ve noticed that compiling this code

#[no_mangle]
pub fn d1(x: u64, y: u64) -> u64 { x / (y + 1) }

with:
-C opt-level=3 -C target-cpu=native

I get:

d1:
	subq	$40, %rsp
	movq	%rdx, %r8
	addq	$1, %r8
	je	.LBB3_5
	movq	%rcx, %rax
	orq	%r8, %rax
	shrq	$32, %rax
	je	.LBB3_2
	xorl	%edx, %edx
	movq	%rcx, %rax
	divq	%r8
	jmp	.LBB3_4
.LBB3_2:
	xorl	%edx, %edx
	movl	%ecx, %eax
	divl	%r8d
.LBB3_4:
	addq	$40, %rsp
	retq
.LBB3_5:
	leaq	panic_loc.3(%rip), %rcx
	callq	_ZN4core9panicking5panic17h69e14964566759a9E
	ud2

In the function d1 the y argument is unsigned, so y+1 can’t be zero, so what’s the point of that panic section?

Edit: Is the panic meant to catch the case where y== u32::MAX?


#2

Right, it’s jumping to that panic BB only when the addq $1, %r8 sets the zero flag, which means you wrapped around the u64.


#3

Unsigned arithmetic can wrap around, and is well defined to do so, unlike signed. So, yes, it needs to check because it could be 0 on the bottom after the increment.

You could try using saturating_add and see if the potential panic goes away.


#4

That’s a C-ism. In Rust, unsigned and signed overflow is treated equally – either as a debug assert or a modular/2’s-complement wrap around.


#5

Unfortunately, no.

example::d1:
        addq    $1, %rsi
        movq    $-1, %rcx
        cmovaeq %rsi, %rcx
        testq   %rcx, %rcx
        je      .LBB1_5
        movq    %rdi, %rax
        orq     %rcx, %rax
        shrq    $32, %rax
        je      .LBB1_2
        xorl    %edx, %edx
        movq    %rdi, %rax
        divq    %rcx
        retq
.LBB1_2:
        xorl    %edx, %edx
        movl    %edi, %eax
        divl    %ecx
        retq
.LBB1_5:
        pushq   %rbp
        movq    %rsp, %rbp
        leaq    panic_loc.2(%rip), %rdi
        callq   core::panicking::panic@PLT

It actually looks like the saturating_add is potentially missing an LLVM attribute to indicate that it doesn’t wrap. As a result, the generated asm contains the useless testq and je instructions.


#6

This seems fit for a bug report.


#7

I’ll do that in a bit and paste the link here (unless someone is already doing this).