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

Rust's optimization broken some code since 1.80 #134958

Closed
Norgerman opened this issue Dec 31, 2024 · 5 comments
Closed

Rust's optimization broken some code since 1.80 #134958

Norgerman opened this issue Dec 31, 2024 · 5 comments
Labels
C-gub Category: the reverse of a compiler bug is generally UB

Comments

@Norgerman
Copy link

Norgerman commented Dec 31, 2024

I tried this code:

use js_sys::Float32Array;
use wasm_bindgen::prelude::wasm_bindgen;

#[repr(C)]
#[derive(Debug, Copy, Clone, Default)]
#[wasm_bindgen]
pub struct Mat4 {
    a1: f32,
    a2: f32,
    a3: f32,
    a4: f32,
    b1: f32,
    b2: f32,
    b3: f32,
    b4: f32,
    c1: f32,
    c2: f32,
    c3: f32,
    c4: f32,
    d1: f32,
    d2: f32,
    d3: f32,
    d4: f32,
}

impl Mat4 {
    fn one() -> Self {
        Self {
            a1: 1.,
            b2: 1.,
            c3: 1.,
            d4: 1.,
            ..Default::default()
        }
    }
}

unsafe impl Zeroable for Mat4 {}
unsafe impl Pod for Mat4 {}

// returns Mat4::one when use release mode (with optimization), debug mode works fine.
pub fn from_js(value: &Float32Array) -> Mat4 {
    let mat = [Mat4::one()];
    unsafe {
        let ptr: *const f32 = &mat[0].a1;
        let dest = Float32Array::view(std::slice::from_raw_parts(ptr, 16));
        dest.set(&value, 0);
    }
    mat[0]
}

// returns current value.
pub fn from_js2(value: &Float32Array) -> Mat4 {
    let mat = Mat4::one();
    unsafe {
        let ptr: *const f32 = &mat.a1;
        let dest = Float32Array::view(std::slice::from_raw_parts(ptr, 16));
        dest.set(&value, 0);
    }
    mat
}

// is the actually broken code without simplify.
pub fn from_js3(value: &Float32Array) -> Mat4 {
    let mat = [Mat4::one()];
    unsafe {
        let dest = Float32Array::view(cast_slice(&mat));
        dest.set(&value, 0);
    }
    mat[0]
}

Current this bug can reproduce with wasm_bindgen & js_sys.

The code broken in release mode since 1.80(specially nightly-05-04).

In release mode from_js is different from from_js2.

  • from_js drop memcpy(in debug mode, will do the copy) just modify the output with Mat4::one
  • from_js2 mat with Mat4::one, and do memcpy to output.
  • in llvm-ir code from_js3 is alias of from_js

llvm-ir attached.

@_ZN9rust_wasm8from_js317h614dde9a38e9443bE = unnamed_addr alias void (ptr, ptr), ptr @_ZN9rust_wasm7from_js17h8e9edadfd73d2621E

; rust_wasm::from_js
; Function Attrs: uwtable
define void @_ZN9rust_wasm7from_js17h8e9edadfd73d2621E(ptr dead_on_unwind noalias nocapture noundef writable sret([64 x i8]) align 4 dereferenceable(64) %_0, ptr noalias noundef readonly align 4 dereferenceable(4) %value) unnamed_addr #5 personality ptr @__CxxFrameHandler3 {
start:
  %dest = alloca [4 x i8], align 4
  %mat = alloca [64 x i8], align 4
  call void @llvm.lifetime.start.p0(i64 64, ptr nonnull %mat)
  store float 1.000000e+00, ptr %_0, align 4
  %0 = getelementptr inbounds i8, ptr %_0, i64 4
  %1 = getelementptr inbounds i8, ptr %_0, i64 20
  tail call void @llvm.memset.p0.i64(ptr noundef nonnull align 4 dereferenceable(16) %0, i8 0, i64 16, i1 false)
  store float 1.000000e+00, ptr %1, align 4
  %2 = getelementptr inbounds i8, ptr %_0, i64 24
  %3 = getelementptr inbounds i8, ptr %_0, i64 40
  tail call void @llvm.memset.p0.i64(ptr noundef nonnull align 4 dereferenceable(16) %2, i8 0, i64 16, i1 false)
  store float 1.000000e+00, ptr %3, align 4
  %4 = getelementptr inbounds i8, ptr %_0, i64 44
  %5 = getelementptr inbounds i8, ptr %_0, i64 60
  tail call void @llvm.memset.p0.i64(ptr noundef nonnull align 4 dereferenceable(16) %4, i8 0, i64 16, i1 false)
  store float 1.000000e+00, ptr %5, align 4
  call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 4 dereferenceable(64) %mat, ptr noundef nonnull align 4 dereferenceable(64) %_0, i64 64, i1 false)
  call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %dest)
; call js_sys::Float32Array::view
  %6 = call noundef i32 @_ZN6js_sys12Float32Array4view17hc0104ce26e3ee0a4E(ptr noalias noundef nonnull readonly align 4 %mat, i64 noundef 16)
  store i32 %6, ptr %dest, align 4
; invoke js_sys::Float32Array::set
  invoke void @_ZN6js_sys12Float32Array3set17he863ee21f8d02321E(ptr noalias noundef nonnull readonly align 4 dereferenceable(4) %dest, ptr noalias noundef nonnull readonly align 4 dereferenceable(4) %value, i32 noundef 0)
          to label %bb2 unwind label %funclet_bb4

funclet_bb4:                                      ; preds = %start
  %cleanuppad = cleanuppad within none []
  %_2.i.i.i.i = icmp ugt i32 %6, 131
  br i1 %_2.i.i.i.i, label %bb1.i.i.i.i, label %"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit"

bb1.i.i.i.i:                                      ; preds = %funclet_bb4
; call wasm_bindgen::__wbindgen_object_drop_ref
  call void @_ZN12wasm_bindgen26__wbindgen_object_drop_ref17h7b23851dfe1382a1E(i32 noundef %6) #19 [ "funclet"(token %cleanuppad) ]
  br label %"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit"

"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit": ; preds = %funclet_bb4, %bb1.i.i.i.i
  cleanupret from %cleanuppad unwind to caller

bb2:                                              ; preds = %start
  %_2.i.i.i.i2 = icmp ugt i32 %6, 131
  br i1 %_2.i.i.i.i2, label %bb1.i.i.i.i3, label %"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit4"

bb1.i.i.i.i3:                                     ; preds = %bb2
; call wasm_bindgen::__wbindgen_object_drop_ref
  call void @_ZN12wasm_bindgen26__wbindgen_object_drop_ref17h7b23851dfe1382a1E(i32 noundef %6) #19, !noalias !20
  br label %"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit4"

"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit4": ; preds = %bb2, %bb1.i.i.i.i3
  call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %dest)
  call void @llvm.lifetime.end.p0(i64 64, ptr nonnull %mat)
  ret void
}

; rust_wasm::from_js2
; Function Attrs: uwtable
define void @_ZN9rust_wasm8from_js217hcd7ecb72bc48b9a8E(ptr dead_on_unwind noalias nocapture noundef writable writeonly sret([64 x i8]) align 4 dereferenceable(64) %_0, ptr noalias noundef readonly align 4 dereferenceable(4) %value) unnamed_addr #5 personality ptr @__CxxFrameHandler3 {
start:
  %dest = alloca [4 x i8], align 4
  %mat = alloca [64 x i8], align 4
  call void @llvm.lifetime.start.p0(i64 64, ptr nonnull %mat)
  store float 1.000000e+00, ptr %mat, align 4
  %0 = getelementptr inbounds i8, ptr %mat, i64 4
  %1 = getelementptr inbounds i8, ptr %mat, i64 20
  call void @llvm.memset.p0.i64(ptr noundef nonnull align 4 dereferenceable(16) %0, i8 0, i64 16, i1 false)
  store float 1.000000e+00, ptr %1, align 4
  %2 = getelementptr inbounds i8, ptr %mat, i64 24
  %3 = getelementptr inbounds i8, ptr %mat, i64 40
  call void @llvm.memset.p0.i64(ptr noundef nonnull align 4 dereferenceable(16) %2, i8 0, i64 16, i1 false)
  store float 1.000000e+00, ptr %3, align 4
  %4 = getelementptr inbounds i8, ptr %mat, i64 44
  %5 = getelementptr inbounds i8, ptr %mat, i64 60
  call void @llvm.memset.p0.i64(ptr noundef nonnull align 4 dereferenceable(16) %4, i8 0, i64 16, i1 false)
  store float 1.000000e+00, ptr %5, align 4
  call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %dest)
; call js_sys::Float32Array::view
  %6 = call noundef i32 @_ZN6js_sys12Float32Array4view17hc0104ce26e3ee0a4E(ptr noalias noundef nonnull readonly align 4 %mat, i64 noundef 16)
  store i32 %6, ptr %dest, align 4
; invoke js_sys::Float32Array::set
  invoke void @_ZN6js_sys12Float32Array3set17he863ee21f8d02321E(ptr noalias noundef nonnull readonly align 4 dereferenceable(4) %dest, ptr noalias noundef nonnull readonly align 4 dereferenceable(4) %value, i32 noundef 0)
          to label %bb2 unwind label %funclet_bb4

funclet_bb4:                                      ; preds = %start
  %cleanuppad = cleanuppad within none []
  %_2.i.i.i.i = icmp ugt i32 %6, 131
  br i1 %_2.i.i.i.i, label %bb1.i.i.i.i, label %"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit"

bb1.i.i.i.i:                                      ; preds = %funclet_bb4
; call wasm_bindgen::__wbindgen_object_drop_ref
  call void @_ZN12wasm_bindgen26__wbindgen_object_drop_ref17h7b23851dfe1382a1E(i32 noundef %6) #19 [ "funclet"(token %cleanuppad) ]
  br label %"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit"

"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit": ; preds = %funclet_bb4, %bb1.i.i.i.i
  cleanupret from %cleanuppad unwind to caller

bb2:                                              ; preds = %start
  %_2.i.i.i.i2 = icmp ugt i32 %6, 131
  br i1 %_2.i.i.i.i2, label %bb1.i.i.i.i3, label %"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit4"

bb1.i.i.i.i3:                                     ; preds = %bb2
; call wasm_bindgen::__wbindgen_object_drop_ref
  call void @_ZN12wasm_bindgen26__wbindgen_object_drop_ref17h7b23851dfe1382a1E(i32 noundef %6) #19, !noalias !23
  br label %"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit4"

"_ZN4core3ptr41drop_in_place$LT$js_sys..Float32Array$GT$17hdb2dd2e4998ada5bE.exit4": ; preds = %bb2, %bb1.i.i.i.i3
  call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %dest)
  call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 4 dereferenceable(64) %_0, ptr noundef nonnull align 4 dereferenceable(64) %mat, i64 64, i1 false)
  call void @llvm.lifetime.end.p0(i64 64, ptr nonnull %mat)
  ret void
}

Meta

rustc --version --verbose:

rustc 1.83.0 (90b35a623 2024-11-26)
binary: rustc
commit-hash: 90b35a6239c3d8bdabc530a6a0816f7ff89a0aaf
commit-date: 2024-11-26
host: x86_64-pc-windows-msvc
release: 1.83.0
LLVM version: 19.1.1
Backtrace

RUST_BACKTRACE=1 cargo build --release
   Compiling proc-macro2 v1.0.92
   Compiling unicode-ident v1.0.14
   Compiling wasm-bindgen-shared v0.2.92
   Compiling bumpalo v3.16.0
   Compiling once_cell v1.20.2
   Compiling log v0.4.22
   Compiling wasm-bindgen v0.2.92
   Compiling cfg-if v1.0.0
   Compiling bytemuck v1.21.0
   Compiling quote v1.0.38
   Compiling syn v2.0.93
   Compiling wasm-bindgen-backend v0.2.92
   Compiling wasm-bindgen-macro-support v0.2.92
   Compiling wasm-bindgen-macro v0.2.92
   Compiling js-sys v0.3.69
   Compiling rust-wasm v0.1.0 (C:\Users\daodao\Desktop\rust-wasm)
    Finished `release` profile [optimized] target(s) in 9.50s

@Norgerman Norgerman added the C-bug Category: This is a bug. label Dec 31, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 31, 2024
@theemathas
Copy link
Contributor

theemathas commented Dec 31, 2024

Your code has undefined behavior, both in from_js and from_js2. This is because you used the & operator, which creates a &f32 immutable reference. And as per the reference:

the bytes pointed to by a shared reference, including transitively through other references (both shared and mutable) and Boxes, are immutable

You used a pointer derived from a &f32 to mutate the data, which is undefined behavior.

Additionally, it is currently undecided whether a *const f32 derived from a &f32 is allowed to access data outside that one f32 or not.

This is how I would write this code. (The &raw mut operator creates a *mut f32 without going through a reference first. And references have more rules that you need to obey in unsafe code.):

pub fn from_js_correct(value: &Float32Array) -> Mat4 {
    let mut mat = Mat4::one();
    unsafe {
        let ptr: *mut f32 = (&raw mut mat).cast();
        let dest = Float32Array::view_mut_raw(ptr, 16);
        dest.set(&value, 0);
    }
    mat
}

@Norgerman
Copy link
Author

Norgerman commented Dec 31, 2024

Use ptr here because removed unnecessary crate. Acutally code looks like this use bytemuck::cast_slice(&mat), no raw ptr here. But mutate immutable value by reference exists. The code is legacy code before TypedArray::from_mut_raw exists. rustc after 1.80 breaks it.

pub fn from_js(value: &Float32Array) -> Mat4 {
    let mat = [Mat4::one()];
    unsafe {
        let dest = Float32Array::view(bytemuck::cast_slice(&mat));
        dest.set(&value, 0);
    }
    mat[0]
}

Additionally, these code should be treated as UB. Is there any way to warn these ub? We need fix the legacy code when upgrade rust. Thanks.

@jieyouxu jieyouxu added C-gub Category: the reverse of a compiler bug is generally UB and removed C-bug Category: This is a bug. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 31, 2024
@theemathas
Copy link
Contributor

theemathas commented Dec 31, 2024

You should probably be using bytemuck::cast_slice_mut along with Float32Array::view_mut_raw. If you don't have access to view_mut_raw, you could maybe look at how the latest version of js_sys implements it I suppose. (I'm not familiar with wasm though.)

There is currently no reliable way to detect UB for code that interacts with FFI. Miri can be used for most code that doesn't use FFI.

@Norgerman
Copy link
Author

Norgerman commented Dec 31, 2024

When mutate immutable bytes with FFI, the generated code's behavior changed between various versions of compiler is acceptable(even between debug and release)?
Code without FFI seems works fine too.

@theemathas
Copy link
Contributor

It doesn't matter if you use FFI or not. If you're committing undefined behavior, there are no guarantees of any kind about what your program will do. It might corrupt everything. It might even "work fine".

@nikic nikic closed this as not planned Won't fix, can't repro, duplicate, stale Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-gub Category: the reverse of a compiler bug is generally UB
Projects
None yet
Development

No branches or pull requests

5 participants