Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wrong result for i128 division on 32-bit targets #41228

Closed
japaric opened this issue Apr 11, 2017 · 8 comments
Closed

wrong result for i128 division on 32-bit targets #41228

japaric opened this issue Apr 11, 2017 · 8 comments
Labels
P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@japaric
Copy link
Member

japaric commented Apr 11, 2017

STR

#![feature(i128_type)]

fn main() {
    let x: i128 = -87559967289969187895646876466835277875;
    let y: i128 = 84285771033834995895337664386045050880;
    let z = x / y;

    println!("{}", x);
    println!("{}", y);
    println!("{}", z);
}
$ cargo run
-87559967289969187895646876466835277875
84285771033834995895337664386045050880
-1

$ cross run --target i686-unknown-linux-gnu
-87559967289969187895646876466835277875
84285771033834995895337664386045050880
-12387103594680423864962518064495067137

This is also wrong at least on i686-pc-windows-msvc. Haven't checked whether the related intrinsic is written in Rust or in C atm. But the implementation in rust-lang-nursery/compiler-builtins gives the right result.

Meta

$ rustc -V
rustc 1.18.0-nightly (3b5754e5c 2017-04-10)

cc @est31
@nagisa
Copy link
Member

nagisa commented Apr 11, 2017

cc me

@est31
Copy link
Member

est31 commented Apr 12, 2017

Haven't checked whether the related intrinsic is written in Rust or in C atm.

All i128 intrinsics on 32 bit platforms are written in Rust.

@kennytm
Copy link
Member

kennytm commented Apr 12, 2017

-12387103594680423864962518064495067137 == 0xf6ae54a3ec7a6f99_ffffffffffffffff.

Apparently the top 64 bits are corrupt.

@arielb1 arielb1 added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 12, 2017
@kennytm
Copy link
Member

kennytm commented Apr 12, 2017 •
edited
Loading

Simplified:

#![feature(i128_type)]

#[inline(never)]
fn check(x: u128, y: u128) {
    assert_eq!(1, x / y);
}

fn main() {
    check(3 << 64 | 1, 3 << 64);
}

This should be a bug in the "XX/H0" case.

The error also disappears with -O, but this may simply be an effect of constant evaluation.

Edit: Further reduction shows the error comes from rustc's compiler_builtins.

#![feature(compiler_builtins_lib)]
#![feature(i128_type)]

extern crate compiler_builtins;

use std::ptr::null_mut;

fn main() {
    let z = compiler_builtins::reimpls::u128_div_mod(3 << 64 | 1, 3 << 64, null_mut());
    assert_eq!(1, z);
}

kennytm added a commit to kennytm/rust that referenced this issue Apr 12, 2017
@kennytm
Fixed invalid 128-bit division on 32-bit target. Fixed issue rust-lan…
71a9e10 
…g#41228.

Added test cases to cover all special-cased branches of udivmodti4.
@kennytm kennytm mentioned this issue Apr 12, 2017
Merged
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 13, 2017
@frewsxcv
Rollup merge of rust-lang#41250 - kennytm:fix-41228, r=nikomatsakis
74e9d55 
Fix invalid 128-bit division on 32-bit target (rust-lang#41228)

The bug of rust-lang#41228 is a typo, this line: https://github.com/rust-lang/rust/blob/1dca19ae3fd195fa517e326a39bfee729da7cadb/src/libcompiler_builtins/lib.rs#L183

```rust
            // 1 <= sr <= u64::bits() - 1
            q = n.wrapping_shl(64u32.wrapping_sub(sr));
```

The **64** should be **128**.

(Compare with https://github.com/rust-lang-nursery/compiler-builtins/blob/280d19f1127aa80739f4179152b11a5f7d36d79f/src/int/udiv.rs#L213-L214:

```rust
            // 1 <= sr <= <hty!($ty)>::bits() - 1
            q = n << (<$ty>::bits() - sr);
```

Or compare with the C implementation https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/udivmodti4.c#L113-L116

```c
        /* 1 <= sr <= n_udword_bits - 1 */
        /* q.all = n.all << (n_utword_bits - sr); */
        q.s.low = 0;
        q.s.high = n.s.low << (n_udword_bits - sr);
```
)

Added a bunch of randomly generated division test cases to try to cover every described branch of `udivmodti4`.
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 13, 2017
@frewsxcv
Rollup merge of rust-lang#41250 - kennytm:fix-41228, r=nikomatsakis
8fb849d 
Fix invalid 128-bit division on 32-bit target (rust-lang#41228)

The bug of rust-lang#41228 is a typo, this line: https://github.com/rust-lang/rust/blob/1dca19ae3fd195fa517e326a39bfee729da7cadb/src/libcompiler_builtins/lib.rs#L183

```rust
            // 1 <= sr <= u64::bits() - 1
            q = n.wrapping_shl(64u32.wrapping_sub(sr));
```

The **64** should be **128**.

(Compare with https://github.com/rust-lang-nursery/compiler-builtins/blob/280d19f1127aa80739f4179152b11a5f7d36d79f/src/int/udiv.rs#L213-L214:

```rust
            // 1 <= sr <= <hty!($ty)>::bits() - 1
            q = n << (<$ty>::bits() - sr);
```

Or compare with the C implementation https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/udivmodti4.c#L113-L116

```c
        /* 1 <= sr <= n_udword_bits - 1 */
        /* q.all = n.all << (n_utword_bits - sr); */
        q.s.low = 0;
        q.s.high = n.s.low << (n_udword_bits - sr);
```
)

Added a bunch of randomly generated division test cases to try to cover every described branch of `udivmodti4`.
frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 13, 2017
@frewsxcv
Rollup merge of rust-lang#41250 - kennytm:fix-41228, r=nikomatsakis
6dfd8f6 
Fix invalid 128-bit division on 32-bit target (rust-lang#41228)

The bug of rust-lang#41228 is a typo, this line: https://github.com/rust-lang/rust/blob/1dca19ae3fd195fa517e326a39bfee729da7cadb/src/libcompiler_builtins/lib.rs#L183

```rust
            // 1 <= sr <= u64::bits() - 1
            q = n.wrapping_shl(64u32.wrapping_sub(sr));
```

The **64** should be **128**.

(Compare with https://github.com/rust-lang-nursery/compiler-builtins/blob/280d19f1127aa80739f4179152b11a5f7d36d79f/src/int/udiv.rs#L213-L214:

```rust
            // 1 <= sr <= <hty!($ty)>::bits() - 1
            q = n << (<$ty>::bits() - sr);
```

Or compare with the C implementation https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/udivmodti4.c#L113-L116

```c
        /* 1 <= sr <= n_udword_bits - 1 */
        /* q.all = n.all << (n_utword_bits - sr); */
        q.s.low = 0;
        q.s.high = n.s.low << (n_udword_bits - sr);
```
)

Added a bunch of randomly generated division test cases to try to cover every described branch of `udivmodti4`.
@nikomatsakis
Copy link
Contributor

@kennytm @japaric was this fixed by #41250 ? Should we close?

@arielb1 arielb1 added the P-high High priority label Apr 13, 2017
@nagisa
Copy link
Member

nagisa commented Apr 13, 2017

I’ll confirm tomorrow.

@nagisa
Copy link
Member

nagisa commented Apr 21, 2017

Fixed on nightly. Closing.

Sorry for the delay.

@nagisa nagisa closed this as completed Apr 21, 2017
@nikomatsakis
Copy link
Contributor

Thanks @nagisa for investigating :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@kennytm @nikomatsakis @nagisa @arielb1 @japaric @est31