`static mut` translation adds UB, but could be safe

[Dante-Broggi]

Thread locals are currently translated as static mut which is unsafe, but more importantly are accessed by &mut which adds UB compared to C.

As a minimal example this:

static __thread int x = 0;
void bar(int *y) {
    int *z = &x;
    *z = *y;
}
void foo() { bar(&x); }

Currently translates on the website (https://c2rust.com) as:

#[thread_local]
static mut x: libc::c_int = 0 as libc::c_int;
#[no_mangle]
pub unsafe extern "C" fn bar(mut y: *mut libc::c_int) {
    let mut z: *mut libc::c_int = &mut x;
    *z = *y;
}
#[no_mangle]
pub unsafe extern "C" fn foo() {
    bar(&mut x);
}

Which creates 2 &mut x references and uses the first after the construction of the second, leading to UB absent in the original C code. But it could use translate as:

use core::cell::Cell;
#[thread_local]
static x: Cell<libc::c_int> = Cell::new(0 as libc::c_int);
#[no_mangle]
pub unsafe extern "C" fn bar(mut y: *mut libc::c_int) {
    let mut z: *mut libc::c_int = x.as_ptr();
    *z = *y;
}
#[no_mangle]
pub unsafe extern "C" fn foo() {
    bar(x.as_ptr());
}

This translation would prevent the UB from creating multiple &mut to the same static, and moreover makes the definition of x itself completely safe. It would even work with pointer typed static __thread variables because unlike normal statics, #[thread_local] static does not need to be Sync.

[kkysen]

What we should be doing, more generally as well, is always using addr_of_mut! instead of &mut so that we never create temporary references where we don't need to. Replacing the &muts here with addr_of_mut!s gets rid of the UB, as per miri. See #301 for more on this.

That said, a translation using Cell seems quite smart here, as then accessing the static is fully safe, as using .as_ptr() only doesn't incur the T: Copy restriction .get() adds. Most of this is due to {Cell,UnsafeCell}::as_ptr being basically the same as addr_of_mut!. We can see if this approach works.

[kkysen]

I realized that the __thread and #[thread_local] don't actually play a role here. The same UB can be reproduced without them:

static int x = 0;
void bar(int *y) {
    int *z = &x;
    *z = *y;
}
void foo() { bar(&x); }

playground link

static mut x: libc::c_int = 0 as libc::c_int;
#[no_mangle]
pub unsafe extern "C" fn bar(mut y: *mut libc::c_int) {
    let mut z: *mut libc::c_int = &mut x;
    *z = *y;
}
#[no_mangle]
pub unsafe extern "C" fn foo() {
    bar(&mut x);
}

[Dante-Broggi]

The UB is indeed independent of __thread but the fix of using Cell requires it.

[Rua]

#1355 addresses the general issue, by using raw borrows everywhere instead of regular borrows. Using Cell and other Sync-ifiers could be added later.