search

immunant / c2rust

Migrate C code to Rust
https://c2rust.com/
Other
3.79k stars 219 forks source link

regression from v0.18.0 to v.0.18.0+409 with regards to wrapping_mul #1024

Open mewmew opened 10 months ago

mewmew commented 10 months ago

A regression was observed between v0.18.0 vs. v.0.18.0+409 (0e9e4048b141e3592f76772767e057235c7fbeef) of c2rust with regards to wrapping_mul.

A test case has been uploaded to https://github.com/mewspring/c2rust_issues/tree/master/issue-wrapping_mul which showcases the regression.

In particular, v0.18.0 uses wrapping_mul to handle multiplication of uint32_t expressions, whereas v0.18.0+409 fails to use wrapping_mul for uint32_t expressions and instead uses the * operations which traps on overflow (i.e. panicked at 'attempt to multiply with overflow').

Diff between v0.18.0 and v0.18.0+409 Rust output.

diff -u -r issue-xxx_v0.18.0/out/src/rnd.rs issue-xxx_v0.18.0+409/out/src/rnd.rs
--- issue-xxx_v0.18.0/out/src/rnd.rs    2023-09-10 15:13:50.682381536 +0200
+++ issue-xxx_v0.18.0+409/out/src/rnd.rs    2023-09-10 15:11:26.769049377 +0200
@@ -16,9 +16,8 @@
 pub unsafe extern "C" fn get_rand_seed() -> int32_t {
     let INCREMENT: uint32_t = 1 as libc::c_int as uint32_t;
     let MULTIPLIER: uint32_t = 0x15a4e35 as libc::c_int as uint32_t;
-    cur_rand_seed = MULTIPLIER
-        .wrapping_mul(cur_rand_seed as uint32_t)
-        .wrapping_add(INCREMENT) as int32_t;
+    cur_rand_seed = (MULTIPLIER * cur_rand_seed as uint32_t).wrapping_add(INCREMENT)
+        as int32_t;
     let mut ret: int32_t = abs(cur_rand_seed);
     return ret;
 }

Steps to reproduce

v.0.18.0

Transpile C code to Rust (with c2rust v.0.18.0 installed)

git clone https://github.com/mewspring/c2rust_issues
cd c2rust_issues/issue-wrapping_mul/issue-wrapping_mul_v0.18.0
./gen_rust.sh

Run C program

cd c2rust_issues/issue-wrapping_mul/issue-wrapping_mul_v0.18.0
make
./issue-wrapping_mul
# Output:
#    get_rand_seed: 0x7AB30485

Run Rust program

cd c2rust_issues/issue-wrapping_mul/issue-wrapping_mul_v0.18.0/out
cargo run
# Output:
#    get_rand_seed: 0x7AB30485

v.0.18.0+409

Transpile C code to Rust (with c2rust v.0.18.0+409 installed)

git clone https://github.com/mewspring/c2rust_issues
cd c2rust_issues/issue-wrapping_mul/issue-wrapping_mul_v0.18.0+409
./gen_rust.sh

Run C program

cd c2rust_issues/issue-wrapping_mul/issue-wrapping_mul_v0.18.0+409
make
./issue-wrapping_mul
# Output:
#    get_rand_seed: 0x7AB30485

Run Rust program

cd c2rust_issues/issue-wrapping_mul/issue-wrapping_mul_v0.18.0+409/out
cargo run
# Output:
#    thread 'main' panicked at 'attempt to multiply with overflow', src/rnd.rs:19:21
mewmew commented 10 months ago

At first, I thought this issue was related to the cast expression not being identified as unsigned in the test case (https://github.com/mewspring/c2rust_issues/blob/79fa28e70470a984e4a99b71fccf9e22309d9198/issue-wrapping_mul/issue-wrapping_mul_v0.18.0%2B409/rnd.c#L13)

    cur_rand_seed = (int32_t)((uint32_t)(MULTIPLIER * ((uint32_t)cur_rand_seed) + INCREMENT));

This would result in the use of * instead of wrapping_mul in convert_binary_operator (https://github.com/immunant/c2rust/blob/0e9e4048b141e3592f76772767e057235c7fbeef/c2rust-transpile/src/translator/operators.rs#L516)

            c_ast::BinOp::Multiply if is_unsigned_integral_type => {
                Ok(mk().method_call_expr(lhs, mk().path_segment("wrapping_mul"), vec![rhs]))
            }
            c_ast::BinOp::Multiply => {
                Ok(mk().binary_expr(BinOp::Mul(Default::default()), lhs, rhs))
            }

However, a follow-up test case (https://github.com/mewspring/c2rust_issues/tree/master/issue-wrapping_mul_source_fix) showed that this was not the source of the issue. Even when updating the C source code to use uint32_t as type of the cur_rand_seed variable (thus alleviating the need of the cast expression), the c2rust output of v0.18.0+409 was still incorrect.

Extracts from C source: https://github.com/mewspring/c2rust_issues/blob/79fa28e70470a984e4a99b71fccf9e22309d9198/issue-wrapping_mul_source_fix/issue-wrapping_mul_v0.18.0%2B409/rnd.c#L4

uint32_t cur_rand_seed = 0;

https://github.com/mewspring/c2rust_issues/blob/79fa28e70470a984e4a99b71fccf9e22309d9198/issue-wrapping_mul_source_fix/issue-wrapping_mul_v0.18.0%2B409/rnd.c#L13

    cur_rand_seed = MULTIPLIER * cur_rand_seed + INCREMENT;

Diff between v0.18.0 and v0.18.0+409 output (note, v0.18.0 output is correct):

diff -u -r issue-wrapping_mul_v0.18.0/out/src/rnd.rs issue-wrapping_mul_v0.18.0+409/out/src/rnd.rs
--- issue-wrapping_mul_v0.18.0/out/src/rnd.rs   2023-09-10 15:20:23.629045015 +0200
+++ issue-wrapping_mul_v0.18.0+409/out/src/rnd.rs   2023-09-10 15:21:22.242377869 +0200
@@ -16,7 +16,7 @@
 pub unsafe extern "C" fn get_rand_seed() -> uint32_t {
     let INCREMENT: uint32_t = 1 as libc::c_int as uint32_t;
     let MULTIPLIER: uint32_t = 0x15a4e35 as libc::c_int as uint32_t;
-    cur_rand_seed = MULTIPLIER.wrapping_mul(cur_rand_seed).wrapping_add(INCREMENT);
+    cur_rand_seed = (MULTIPLIER * cur_rand_seed).wrapping_add(INCREMENT);
     let mut ret: uint32_t = abs(cur_rand_seed as int32_t) as uint32_t;
     return ret;
 }

For some reason, the uint32_t type of cur_rand_seed does not seem to be enough to trigger the is_unsigned_integral_type case of convert_binary_operator, and as such, the wrapping_mul operand is not used even though it should be.

kkysen commented 10 months ago

Hi @mewmew, I wasn't able to reproduce this. I ran the gen_rust.shs from your repo with c2rust compiled from master (C2Rust v0.18.0+409 (0e9e4048b 2023-09-08)), but everything used .wrapping_mul, not *. I'll close this for now, but if you can find a test case that successfully reproduces the issue, you can re-open it.

mewmew commented 10 months ago

Hi @kkysen,

Thanks for taking a look at this. And also, of course for working on c2rust. It's quite an incredible transpiler that you've built!

Here is a Docker image that reproduces the issue, built from the official Arch Linux base-devel docker image.

docker pull mewbaz/c2rust-git-issue-wrapping_mul

Relevant docker out:

Step 6/25 : RUN rustup show
 ---> Running in 96584e402e34
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/user123/.rustup

installed toolchains
--------------------

nightly-2022-08-08-x86_64-unknown-linux-gnu
nightly-x86_64-unknown-linux-gnu (default)

active toolchain
----------------

nightly-x86_64-unknown-linux-gnu (default)
rustc 1.74.0-nightly (030e4d382 2023-09-10)

Step 7/25 : RUN c2rust --help
 ---> Running in fdb8f91e0685
C2Rust v0.18.0+409 (0e9e4048b 2023-09-08)
The C2Rust Project Developers <c2rust@immunant.com>

Step 19/25 : RUN c2rust transpile --emit-build-files compile_commands.json -o out --binary main -- -I/usr/lib/clang/16/include
 ---> Running in ecf9e1812c04
Transpiling main.c
Transpiling rnd.c
Removing intermediate container ecf9e1812c04
 ---> 979670a377d8
Step 20/25 : RUN cat out/src/rnd.rs
 ---> Running in 631e857af489
use ::libc;
extern "C" {
    fn abs(_: libc::c_int) -> libc::c_int;
}
pub type __int32_t = libc::c_int;
pub type __uint32_t = libc::c_uint;
pub type int32_t = __int32_t;
pub type uint32_t = __uint32_t;
#[no_mangle]
pub static mut cur_rand_seed: int32_t = 0 as libc::c_int;
#[no_mangle]
pub unsafe extern "C" fn set_rand_seed(mut s: int32_t) {
    cur_rand_seed = s;
}
#[no_mangle]
pub unsafe extern "C" fn get_rand_seed() -> int32_t {
    let INCREMENT: uint32_t = 1 as libc::c_int as uint32_t;
    let MULTIPLIER: uint32_t = 0x15a4e35 as libc::c_int as uint32_t;
    cur_rand_seed = (MULTIPLIER * cur_rand_seed as uint32_t).wrapping_add(INCREMENT)
        as int32_t;
    let mut ret: int32_t = abs(cur_rand_seed);
    return ret;
}

Step 24/25 : RUN c2rust transpile --emit-build-files compile_commands.json -o out --binary main -- -I/usr/lib/clang/16/include
 ---> Running in 8f9b19f7cbd9
Transpiling main.c
Transpiling rnd.c
Removing intermediate container 8f9b19f7cbd9
 ---> 76519b9517ae
Step 25/25 : RUN cat out/src/rnd.rs
 ---> Running in de3cc82b65d0
use ::libc;
extern "C" {
    fn abs(_: libc::c_int) -> libc::c_int;
}
pub type __int32_t = libc::c_int;
pub type __uint32_t = libc::c_uint;
pub type int32_t = __int32_t;
pub type uint32_t = __uint32_t;
#[no_mangle]
pub static mut cur_rand_seed: uint32_t = 0 as libc::c_int as uint32_t;
#[no_mangle]
pub unsafe extern "C" fn set_rand_seed(mut s: uint32_t) {
    cur_rand_seed = s;
}
#[no_mangle]
pub unsafe extern "C" fn get_rand_seed() -> uint32_t {
    let INCREMENT: uint32_t = 1 as libc::c_int as uint32_t;
    let MULTIPLIER: uint32_t = 0x15a4e35 as libc::c_int as uint32_t;
    cur_rand_seed = (MULTIPLIER * cur_rand_seed).wrapping_add(INCREMENT);
    let mut ret: uint32_t = abs(cur_rand_seed as int32_t) as uint32_t;
    return ret;
}

In particular, note that the multiplication operand * is used instead of wrapping_mul in both outputs:

    cur_rand_seed = (MULTIPLIER * cur_rand_seed as uint32_t).wrapping_add(INCREMENT)
        as int32_t;
    cur_rand_seed = (MULTIPLIER * cur_rand_seed).wrapping_add(INCREMENT);

With kindness, Robin

mewmew commented 10 months ago

I'll close this for now, but if you can find a test case that successfully reproduces the issue, you can re-open it.

There doesn't seem to be a button to re-open issues (for outside collaborators), once an issue has been closed by immunant.

Please re-open this issue as it is reproducible given the above docker image (https://github.com/immunant/c2rust/issues/1024#issuecomment-1713642503).

kkysen commented 10 months ago

I'll re-open it for now; I'll look into it more when I get back from RustConf and have time.

mewmew commented 10 months ago

I'll re-open it for now; I'll look into it more when I get back from RustConf and have time.

Sounds good. Enjoy RustConf!

mewmew commented 3 months ago

Hi @kkysen,

Hope you're well and enjoying the start of Spring.

I was wondering if you had a chance to look at this after you returned from RustCon?

Just wanted to confirm whether you could reproduce the issue with the above Docker images?

Cheers, Robin

kkysen commented 3 months ago

Hi @kkysen,

Hope you're well and enjoying the start of Spring.

I was wondering if you had a chance to look at this after you returned from RustCon?

Just wanted to confirm whether you could reproduce the issue with the above Docker images?

Cheers, Robin

Thanks! I hope you're well, too! Sorry, I totally forgot about this, that was a while ago. I'll try to look into it soon. Sorry to keep you waiting so long.