fix: 32 bits architecture crash

[erwanvivien]

When using imagequant on 32 bit env I had this bug:
crossbeam expects pointers (data) to be aligned on 16 bits (because of this repr(C, align(16))) but on 32 bits arch, we can have pointers (data) aligned on 8 bits

[kornelski]

I don't understand the issue. The alignment is in bytes. It applies to a struct that contains 32-bit/4-byte floats, which usually get 4-byte alignment. I override it to be aligned to 16 bytes. Every 16-byte-aligned pointer is also a valid 4-byte-aligned pointer.

[erwanvivien]

You are right in what you said, although it's not the problem.

The problem is that I can have 8byte aligned pointers on 32bits arch (0xABC8), although, crossbeam and probably every write that at some point rely on std, are asked that pointer is aligned on data alignment (16 here)

So my pointer 0xABC8 cannot write because it's not aligned on 16 bits

I know what you are doing with the repr and I didn't modify that, I only make it so the repr is added when using any simd (apple or intel)

[erwanvivien]

I indeed in my first message, mixed up bits & bytes, I hope that my new explanation is better

[kornelski]

I understand the change you're proposing, but I don't understand reasoning behind it. I still don't follow how is crossbeam related? Do you have a demo code that demonstrates the problem? How do you get the misaligned pointer? How come alignment of an internal data type affects anything for you?

I also don't see why any Rust code would ignore alignment of a data type declared in Rust. I should be able to put align(128) on every single thing everywhere in my code, and still have it work on every platform. C has a default pessimistic alignment for malloc, but Rust doesn't have such thing.

For example, in Rust it's invalid to cast &[u8] slice to a an &[f_pixel] slice. For such operations you'd need to use allocator with matching Layout, or slice::align_to, or manually do pointer arithmetic to fix up for std::mem::align_of::<f_pixel>(). Rust libraries should be doing that.

[erwanvivien]

I will make a demo, but on 32 bits (wasm) I managed to do it

It's crossbeam related in that it's where the error appears, in a crossbeam::send, where write internal is called and this write asserts previous behaviour

Now that you say it, it's possible that it's a Rust error but I doubt it

[erwanvivien]

In debug:

align_of::<T>() = 16 ptr as usize % align_of::<T>() = 8

Stack trace:

gifski.js:449 panicked at 'unsafe precondition(s) violated: ptr::copy_nonoverlapping requires that both pointer arguments are aligned and non-null and the specified memory ranges do not overlap', /Users/erwanvivien/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/panicking.rs:123:5 Stack: Error at imports.wbg.__wbg_new_abda76e883ba8a5f (http://localhost:3000/gifski.js:437:21) at console_error_panic_hook::hook::h6ea0ca6124b87dfa (http://localhost:3000/gifski_bg.wasm:wasm-function[288]:0x624e2) at core::ops::function::Fn::call::h606f3ad5aeb8a36e (http://localhost:3000/gifski_bg.wasm:wasm-function[1803]:0xb3040) at std::panicking::rust_panic_with_hook::h32f0ac609b07af1d (http://localhost:3000/gifski_bg.wasm:wasm-function[672]:0x8f304) at std::panicking::begin_panic_handler::{{closure}}::h47c05f2127154d50 (http://localhost:3000/gifski_bg.wasm:wasm-function[1025]:0xa54c3) at std::sys_common::backtrace::__rust_end_short_backtrace::h6619479061195d23 (http://localhost:3000/gifski_bg.wasm:wasm-function[1507]:0xb12dc) at rust_begin_unwind (http://localhost:3000/gifski_bg.wasm:wasm-function[1312]:0xadfe3) at core::panicking::panic_nounwind_fmt::h00293eb3a12d8dfe (http://localhost:3000/gifski_bg.wasm:wasm-function[1437]:0xb04cc) at core::panicking::panic_nounwind::he233d5796126338b (http://localhost:3000/gifski_bg.wasm:wasm-function[1329]:0xae5a6) at core::slice::sort::insertion_sort_shift_left::hb3f69a1fabc1b9e9 (http://localhost:3000/gifski_bg.wasm:wasm-function[220]:0x54ff7) 

[erwanvivien]

https://github.com/erwanvivien/gifski/tree/wasm-test

To reproduce: change Cargo.toml imagequant to use real imagequant

imagequant = "4.1"

then ./wasm-pack.sh && npx serve -c serve.json pkg and open console

[erwanvivien]

Any update on this PR? I think this PR is very non-destructive and should be considered :)

[kornelski]

I haven't had time to check this yet — that repo is far from being a minimal reproducer, and has tons of other code, WASM threads, polyfills, hacks, etc.

I do not want to make a change that doesn't seem to make sense. I don't see a logical reason why this alignment in this library would be wrong. It works correctly on 32-bit x86, as well as on a few other platforms.

If there is a struct with a large alignment, it's simply not legal for a pointer to exist to this strut with smaller alignment. When the pointer isn't aligned properly, that's not the fault of the struct, that's a bug in code that created this pointer. So you're not dealing here with an issue in libimagequant. It more looks like a bug in the environment using the library, which could be a bug in the WASM compiler or WASM runtime.

[kornelski]

I haven't got time to set it up in wasm, so I'll just merge it as is.

[erwanvivien]

Thanks Kornelski, a side note: I tried to write the code for Wasm SIMD (in pal.rs), it gave poor results

My simple benchmark showed from 33s (no SIMD) to 39s (with SIMD), and the need to recompile with specific flags to enable them, so it's not worth I think

[erwanvivien]

#[cfg(target_arch = "wasm32")] #[inline(always)] pub fn diff(&self, other: &f_pixel) -> f32 { unsafe { use std::arch::wasm32; let px = wasm32::f32x4(self.0.a, self.0.r, self.0.g, self.0.b); let py = wasm32::f32x4(other.0.a, other.0.r, other.0.g, other.0.b); // y.a - x.a let alphas = wasm32::f32x4_splat(other.0.a - self.0.a); let onblack = wasm32::f32x4_sub(px, py); // x - y let onwhite = wasm32::f32x4_add(onblack, alphas); // x - y + (y.a - x.a) let onblack = wasm32::f32x4_mul(onblack, onblack); let onwhite = wasm32::f32x4_mul(onwhite, onwhite); let max = wasm32::f32x4_max(onwhite, onblack); let mut max_store = [0f32; 4]; wasm32::v128_store(max_store.as_mut_ptr() as *mut _, max); // add rgb, not a max_store[1] + max_store[2] + max_store[3] } }

Here's the whole patch: wasm32-simd.patch