Fix invalid 128-bit division on 32-bit target (#41228) #41250
Conversation
…g#41228. Added test cases to cover all special-cased branches of udivmodti4.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Looks good. Thanks for the quick fix @kennytm! |
|
@bors r+ |
|
📌 Commit 71a9e10 has been approved by |
| @@ -180,7 +180,7 @@ pub mod reimpls { | |||
| sr = sr.wrapping_add(1); | |||
|
|
|||
| // 1 <= sr <= u64::bits() - 1 | |||
There was a problem hiding this comment.
Actually, is the comment still correct?
There was a problem hiding this comment.
And the C code is actually assuming it when executing the shift, it sets the low value to 0 because it knows that n gets shifted at least by 65 bits.
There was a problem hiding this comment.
@nikomatsakis The comment is about the range of sr, not the amount of shift, so it is still correct.
|
⌛ Testing commit 71a9e10 with merge 9f151fd... |
|
💔 Test failed - status-appveyor |
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`.
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`.
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`.
The bug of #41228 is a typo, this line:
rust/src/libcompiler_builtins/lib.rs
Line 183 in 1dca19a
The 64 should be 128.
(Compare with https://github.com/rust-lang-nursery/compiler-builtins/blob/280d19f1127aa80739f4179152b11a5f7d36d79f/src/int/udiv.rs#L213-L214:
Or compare with the C implementation https://github.com/llvm-mirror/compiler-rt/blob/master/lib/builtins/udivmodti4.c#L113-L116
)
Added a bunch of randomly generated division test cases to try to cover every described branch of
udivmodti4.