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.

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

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

Open
mewmew opened this issue Sep 10, 2023 · 8 comments
Open

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

mewmew opened this issue Sep 10, 2023 · 8 comments

Comments

@mewmew
Copy link

mewmew commented Sep 10, 2023

A regression was observed between v0.18.0 vs. v.0.18.0+409 (0e9e404) 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
Copy link
Author

mewmew commented Sep 10, 2023

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 (

c_ast::BinOp::Multiply if is_unsigned_integral_type => {
)

            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
Copy link
Contributor

kkysen commented Sep 11, 2023

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.

@kkysen kkysen closed this as completed Sep 11, 2023
@mewmew
Copy link
Author

mewmew commented Sep 11, 2023

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
Copy link
Author

mewmew commented Sep 11, 2023

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 (#1024 (comment)).

@kkysen
Copy link
Contributor

kkysen commented Sep 12, 2023

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

@kkysen kkysen reopened this Sep 12, 2023
@mewmew
Copy link
Author

mewmew commented Sep 12, 2023

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
Copy link
Author

mewmew commented Mar 31, 2024

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
Copy link
Contributor

kkysen commented Apr 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants