Support for Result<T, JsValue> can leak stack space
#1963
Comments
|
I'm not really sure of the best way to fix this, but my thinking is that we should fix this relatively quickly (ish) and probably do so with these changes:
Not great all around unfortunately :( |
|
Whatever solution we decide on should also work for |
|
I'm just speaking out of my ass here, but if the problem is that the wasm code to restore the stack pointer never runs, would it be possible to configure a function with something named #[no_return], which can tell the calling function to clean up before passing the values to the #[no_return] function? |
|
@gnodarse I noticed something odd in the docs for I thought it might be possible to use LLVM intrinsics to manipulate the stack pointer directly. We don't need to run any destructors for the supported Result types AFAIK, so this might work. It might be awkward for general use of throw_val, since you need to call the intrinsic before throw_val. Similar thread, @llvm.stacksave/@llvm.stackrestore intrinsics. However, it doesn't seem to work. Code hereextern "C" {
#[link_name = "llvm.stacksave"]
fn stacksave() -> *mut i8;
#[link_name = "llvm.stackrestore"]
fn stackrestore(saved: *mut i8);
}
pub fn whatever() {
let x = unsafe { stacksave() };
foo(&mut [0u8; 2000]);
unsafe { stackrestore(x) };
wasm_bindgen::throw_val(unsafe { mem::transmute(0) })
}
#[inline(never)]
pub fn foo(a: &mut [u8]) {
a[0] = 1;
}(func $thing::whatever::hafada7663f9ba4bb (type 0)
(local i32)
(global.set 0
(local.tee 0
(i32.sub
(global.get 0)
(i32.const 2000))))
(call $thing::foo::h9889bff68136a647
(call $memset
(local.get 0)
(i32.const 0)
(i32.const 2000))
(i32.const 2000))
(call $wasm_bindgen::throw_val::h22d553dc64b7a4ba
(i32.const 0))
(unreachable))It's pretty obvious that rustc doesn't care that we called stacksave before using the array, it put the alloca first, so stacksave/restore does nothing and is eliminated. Not sure how to get around this really. ; thing::whatever
; Function Attrs: noreturn nounwind
define hidden void @_ZN5thing8whatever17hafada7663f9ba4bbE() unnamed_addr #1 {
start:
%_6 = alloca [2000 x i8], align 1
%_8 = tail call i8* @llvm.stacksave()
%0 = getelementptr inbounds [2000 x i8], [2000 x i8]* %_6, i32 0, i32 0
call void @llvm.lifetime.start.p0i8(i64 2000, i8* nonnull %0)
call void @llvm.memset.p0i8.i32(i8* nonnull align 1 %0, i8 0, i32 2000, i1 false)
%_3.0 = bitcast [2000 x i8]* %_6 to [0 x i8]*
; call thing::foo
call void @_ZN5thing3foo17h9889bff68136a647E([0 x i8]* nonnull align 1 %_3.0, i32 2000)
call void @llvm.lifetime.end.p0i8(i64 2000, i8* nonnull %0)
tail call void @llvm.stackrestore(i8* %_8)
; call wasm_bindgen::throw_val
tail call void @_ZN12wasm_bindgen9throw_val17h22d553dc64b7a4baE(i32 0)
unreachableBeyond that, you can't get stack size down to zero just by putting |
|
Something I think might work is the following: #[wasm_bindgen]
extern "C" {
pub type WasmResult;
#[wasm_bindgen(constructor)]
pub fn new(value: JsValue, isError: bool) -> WasmResult;
}And then in generated glue code for methods that return a use js_sys::Error as JsError;
#[wasm_bindgen]
fn get_result() -> Result<String, JsError> {
let err = JsError::new("message");
Err(err)
}
// ... transformed into
#[wasm_bindgen(imagine_i_remember_what_glue_signatures_look_like)]
extern "C" fn get_result() -> JsValue {
match get_result_inner() {
Ok(val) => WasmResult::new(val.into(), false).into(),
Err(err) => WasmResult::new(err.into(), true).into(),
}
}Note this exported function always returns, and doesn't actually throw anything, because we have an unwrap function to call once we're out of the Wasm stack: class WasmResult {
constructor(value, isError) {
this.value = value;
this.isError = isError;
}
unwrap() {
if (this.isError) {
throw this.value;
}
return this.value;
}
}
export function get_result() {
let result = wasm.get_result(); // WasmResult
let taken = takeObject(result);
return taken.unwrap();
}If you create an actual JsError from Rust, it will get a nice stack trace at creation time, and this will be preserved when it is re-thrown. Regular values like strings won't get that and you'll see a new trace from the unwrap point which is still close enough because it'll have the function name ('get_result') in it. |
|
It won't work for throw_val, unfortunately. Is that still a hard requirement? I could see |
|
Indeed yeah something like that should work! Somehow we need to get wasm to fully return and then have the JS shim figure out what to throw. There's various permutations on how that could work precisely, but so long as it works seems fine by me! |
|
Cool! Do you want me to put together a PR or would whipping my work into shape be more work than just letting @Pauan do it? |
|
I think Pauan isn't working much on wasm-bindgen any more, so feel free to send a PR @cormacrelf! |
|
Lucky to have found this issue as I've been suspecting of such. The proposed wrapping returned in For the |
|
(Edit, also the panic/catch_unwind doesn't work. You need to throw the error from the glue code. Catch_unwind also uses stack space, likely less than a big stack array would, but non zero. There is a WASM proposal to have exception handling, and I haven't read it in any detail but it appears to implement unwinding natively. |
|
@cormacrelf on a quick browse on mobile the PR is looking great! I'll try to have a proper look and test in the near-ish future. re: the panic+catch_unwind, I was under the impression that panics were already fully supported in wasm but started doubting this after posting the idea, forgot to actually test, haven't looked the related wasm proposal. |
|
Panicking is supported. No other assembly language LLVM outputs supports exceptions, frankly an incredibly high level feature, so I assume panicking is implemented in WASM just as it is on x86 or ARM. Think of panic unwinding like returning Err but slightly optimised as no conversions are done on the way up, so function calls for which a panic would require dropping the same objects can share a landing pad. I was just speculating about how if exceptions were available, LLVM might be able to avoid implementing panics "manually", but I realise now that doing so would ignore the responsibility to Drop things as a panic unwinds. I don't know if the exceptions proposal is capable of doing that, and if not, then I have no idea what it is useful for outside GC languages compiled to wasm. |
Given code that looks like this:
we can build and run it with:
and run it with:
to yield:
The problem here is apparent when we diassemble the wasm output:
Here the problem we can see is that the export and function
barallocate 16 bytes of stack space, but this stack space is never released. This means that our global stack pointer is constantly decreasing, and will never get bumped back up.The tl;dr; is that every time an error is thrown/return from a Rust/wasm function it runs the risk of not reclaiming used stack space, and this is clearly a bug we need to fix!
The text was updated successfully, but these errors were encountered: