-
Notifications
You must be signed in to change notification settings - Fork 824
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
* vm.rs - calling Ctx
data_finalizer
upon destruction
#561
* vm.rs - calling Ctx
data_finalizer
upon destruction
#561
Conversation
…struction"](wasmerio/wasmer#561) * updating crates
Ummm... tests are broken on windows, so we have to take a deeper look. |
bors try |
tryBuild failed |
(still investigating) |
bors try |
tryBuild failed |
Hey @YaronWittenstein I found the issue on our side! We are breaking the contract on The reason is that we do something like let ctx = Box::new(std::mem::uninit());
something_else.val = Box::into_raw(ctx);
something_else.val = real_ctx; // this is invalid because it causes the uninit Ctx to Drop And the solution is to change the last line to let mut real_ctx = real_ctx;
std::mem::swap(&mut real_ctx, &mut something_else.val);
std::mem::forget(real_ctx); // tell Rust to not Drop our garbage Ctx I have a fix locally that I need to clean up -- and actually I need to port it to the new MaybeUninit style for Rust 1.38 -- but your PR was passing all the tests I tried on my computer last night. Thanks! I'll ping you again when its merged into master |
576: fix Drop of uninit Ctx; use MaybeUninit r=MarkMcCaskey a=MarkMcCaskey I'm somewhat concerned that we need to call `intrinsics::panic_if_uninhabited::<T>();` or something to tell the compiler that we know it's safe. We just reinterpret the pointer later as initialized which is safe in theory due to `repr(transparent)` this should unblock #561 Co-authored-by: Mark McCaskey <[email protected]>
576: fix Drop of uninit Ctx; use MaybeUninit r=MarkMcCaskey a=MarkMcCaskey I'm somewhat concerned that we need to call `intrinsics::panic_if_uninhabited::<T>();` or something to tell the compiler that we know it's safe. We just reinterpret the pointer later as initialized which is safe in theory due to `repr(transparent)` this should unblock #561 Co-authored-by: Mark McCaskey <[email protected]>
576: fix Drop of uninit Ctx; use MaybeUninit r=MarkMcCaskey a=MarkMcCaskey I'm somewhat concerned that we need to call `intrinsics::panic_if_uninhabited::<T>();` or something to tell the compiler that we know it's safe. We just reinterpret the pointer later as initialized which is safe in theory due to `repr(transparent)` this should unblock #561 Co-authored-by: Mark McCaskey <[email protected]>
576: fix Drop of uninit Ctx; use MaybeUninit r=MarkMcCaskey a=MarkMcCaskey I'm somewhat concerned that we need to call `intrinsics::panic_if_uninhabited::<T>();` or something to tell the compiler that we know it's safe. We just reinterpret the pointer later as initialized which is safe in theory due to `repr(transparent)` this should unblock #561 Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>
576: fix Drop of uninit Ctx; use MaybeUninit r=MarkMcCaskey a=MarkMcCaskey I'm somewhat concerned that we need to call `intrinsics::panic_if_uninhabited::<T>();` or something to tell the compiler that we know it's safe. We just reinterpret the pointer later as initialized which is safe in theory due to `repr(transparent)` this should unblock #561 Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>
bors try |
It seems that the
data_finalizer
field of theCtx
is never being called upon instance destruction.It seems a bit tricky to assert that in though
@syrusakbary @Hywan