From 2d4ec981765841092bf3b5e9249d510b3d844ded Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Tue, 20 Jun 2023 11:50:47 +0200 Subject: [PATCH 1/6] Move Artifact Regster FrameInfo --- lib/compiler/src/engine/artifact.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index 47e9e110c39..4b3531a3927 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -301,7 +301,7 @@ impl Artifact { finished_dynamic_function_trampolines.into_boxed_slice(); let signatures = signatures.into_boxed_slice(); - Ok(Self { + let mut artifact = Self { id: Default::default(), artifact, allocated: Some(AllocatedArtifact { @@ -312,7 +312,11 @@ impl Artifact { frame_info_registration: Some(Mutex::new(None)), finished_function_lengths, }), - }) + }; + + artifact.register_frame_info(); + + Ok(artifact) } /// Check if the provided bytes look like a serialized `ArtifactBuild`. @@ -379,7 +383,7 @@ impl Artifact { /// Register thie `Artifact` stack frame information into the global scope. /// /// This is required to ensure that any traps can be properly symbolicated. - pub fn register_frame_info(&self) { + pub fn register_frame_info(&mut self) { if let Some(frame_info_registration) = self .allocated .as_ref() @@ -531,8 +535,6 @@ impl Artifact { .map_err(InstantiationError::Link)? .into_boxed_slice(); - self.register_frame_info(); - let handle = VMInstance::new( allocator, module, From 178c315fd106512b8ce971479bc3c71c24988b8b Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Mon, 26 Jun 2023 11:44:19 +0200 Subject: [PATCH 2/6] Removed Mutex for GlobalFrameInfoRegistration as it's no longer needed --- lib/compiler/src/engine/artifact.rs | 74 +++++++++++++++-------------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index 4b3531a3927..c07d0e4c19d 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -18,7 +18,6 @@ use enumset::EnumSet; use std::mem; use std::sync::atomic::{AtomicUsize, Ordering::SeqCst}; use std::sync::Arc; -use std::sync::Mutex; #[cfg(feature = "static-artifact-create")] use wasmer_object::{emit_compilation, emit_data, get_object_for_target, Object}; #[cfg(any(feature = "static-artifact-create", feature = "static-artifact-load"))] @@ -38,12 +37,12 @@ use wasmer_vm::{FunctionBodyPtr, MemoryStyle, TableStyle, VMSharedSignatureIndex use wasmer_vm::{InstanceAllocator, StoreObjects, TrapHandlerFn, VMConfig, VMExtern, VMInstance}; pub struct AllocatedArtifact { + /// Some(_) only if this is not a deserialized static artifact + frame_info_registration: Option, finished_functions: BoxedSlice, finished_function_call_trampolines: BoxedSlice, finished_dynamic_function_trampolines: BoxedSlice, signatures: BoxedSlice, - /// Some(_) only if this is not a deserialized static artifact - frame_info_registration: Option>>, finished_function_lengths: BoxedSlice, } @@ -309,7 +308,7 @@ impl Artifact { finished_function_call_trampolines, finished_dynamic_function_trampolines, signatures, - frame_info_registration: Some(Mutex::new(None)), + frame_info_registration: None, finished_function_lengths, }), }; @@ -384,45 +383,48 @@ impl Artifact { /// /// This is required to ensure that any traps can be properly symbolicated. pub fn register_frame_info(&mut self) { - if let Some(frame_info_registration) = self + if self .allocated .as_ref() .expect("It must be allocated") .frame_info_registration - .as_ref() + .is_some() { - let mut info = frame_info_registration.lock().unwrap(); + return; // already done + } - if info.is_some() { - return; - } + let finished_function_extents = self + .allocated + .as_ref() + .expect("It must be allocated") + .finished_functions + .values() + .copied() + .zip( + self.allocated + .as_ref() + .expect("It must be allocated") + .finished_function_lengths + .values() + .copied(), + ) + .map(|(ptr, length)| FunctionExtent { ptr, length }) + .collect::>() + .into_boxed_slice(); - let finished_function_extents = self - .allocated - .as_ref() - .expect("It must be allocated") - .finished_functions - .values() - .copied() - .zip( - self.allocated - .as_ref() - .expect("It must be allocated") - .finished_function_lengths - .values() - .copied(), - ) - .map(|(ptr, length)| FunctionExtent { ptr, length }) - .collect::>() - .into_boxed_slice(); - - let frame_infos = self.artifact.get_frame_info_ref(); - *info = register_frame_info( - self.artifact.create_module_info(), - &finished_function_extents, - frame_infos.clone(), - ); - } + let frame_info_registration = &mut self + .allocated + .as_mut() + .expect("It must be allocated") + .frame_info_registration; + + let frame_infos = self.artifact.get_frame_info_ref(); + + *frame_info_registration = register_frame_info( + self.artifact.create_module_info(), + &finished_function_extents, + frame_infos.clone(), + ); } /// Returns the functions allocated in memory or this `Artifact` From cdfad468940200c3e825b9b80fb9795afd4e99e1 Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Mon, 26 Jun 2023 16:46:19 +0200 Subject: [PATCH 3/6] Moved GlobalFrameInfo to codeMemory (fixes #3793) --- lib/compiler/src/engine/artifact.rs | 41 +++++++++++++++++++++----- lib/compiler/src/engine/code_memory.rs | 9 ++++++ lib/compiler/src/engine/inner.rs | 11 +++++++ 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index c07d0e4c19d..9da9b5016ad 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -37,7 +37,8 @@ use wasmer_vm::{FunctionBodyPtr, MemoryStyle, TableStyle, VMSharedSignatureIndex use wasmer_vm::{InstanceAllocator, StoreObjects, TrapHandlerFn, VMConfig, VMExtern, VMInstance}; pub struct AllocatedArtifact { - /// Some(_) only if this is not a deserialized static artifact + frame_info_registered: bool, + // frame_info_registered is not staying there but transfered to InnerEnginge / CodeMemory frame_info_registration: Option, finished_functions: BoxedSlice, finished_function_call_trampolines: BoxedSlice, @@ -304,16 +305,20 @@ impl Artifact { id: Default::default(), artifact, allocated: Some(AllocatedArtifact { + frame_info_registered: false, + frame_info_registration: None, finished_functions, finished_function_call_trampolines, finished_dynamic_function_trampolines, signatures, - frame_info_registration: None, finished_function_lengths, }), }; - artifact.register_frame_info(); + artifact.internal_register_frame_info(); + if let Some(frame_info) = artifact.transfert_frame_info_registration() { + engine_inner.register_frame_info(frame_info); + } Ok(artifact) } @@ -381,14 +386,18 @@ impl ArtifactCreate for Artifact { impl Artifact { /// Register thie `Artifact` stack frame information into the global scope. /// - /// This is required to ensure that any traps can be properly symbolicated. + /// This is not required anymore as it's done automaticaly when creating by 'Artifact::from_parts' + #[deprecated(since = "4.1.0", note = "done automaticaly by Artifact::from_parts")] pub fn register_frame_info(&mut self) { + self.internal_register_frame_info() + } + + fn internal_register_frame_info(&mut self) { if self .allocated .as_ref() .expect("It must be allocated") - .frame_info_registration - .is_some() + .frame_info_registered { return; // already done } @@ -425,6 +434,23 @@ impl Artifact { &finished_function_extents, frame_infos.clone(), ); + + self.allocated + .as_mut() + .expect("It must be allocated") + .frame_info_registered = true; + } + + /// The GlobalFrameInfoRegistration needs to be transfered to EngineInner if + /// deprecated register_frame_info has been used. + pub fn transfert_frame_info_registration(&mut self) -> Option { + let frame_info_registration = &mut self + .allocated + .as_mut() + .expect("It must be allocated") + .frame_info_registration; + + frame_info_registration.take() } /// Returns the functions allocated in memory or this `Artifact` @@ -895,6 +921,8 @@ impl Artifact { id: Default::default(), artifact, allocated: Some(AllocatedArtifact { + frame_info_registered: false, + frame_info_registration: None, finished_functions: finished_functions.into_boxed_slice(), finished_function_call_trampolines: finished_function_call_trampolines .into_boxed_slice(), @@ -902,7 +930,6 @@ impl Artifact { .into_boxed_slice(), signatures: signatures.into_boxed_slice(), finished_function_lengths, - frame_info_registration: None, }), }) } diff --git a/lib/compiler/src/engine/code_memory.rs b/lib/compiler/src/engine/code_memory.rs index 2cb3ef79995..f9715c1921f 100644 --- a/lib/compiler/src/engine/code_memory.rs +++ b/lib/compiler/src/engine/code_memory.rs @@ -3,6 +3,7 @@ //! Memory management for executable code. use super::unwind::UnwindRegistry; +use crate::GlobalFrameInfoRegistration; use wasmer_types::{CompiledFunctionUnwindInfo, CustomSection, FunctionBody}; use wasmer_vm::{Mmap, VMFunctionBody}; @@ -19,6 +20,8 @@ const DATA_SECTION_ALIGNMENT: usize = 64; /// Memory manager for executable code. pub struct CodeMemory { + // frame info is placed first, to ensure it's dropped before the mmap + frame_info_registration: Option, unwind_registry: UnwindRegistry, mmap: Mmap, start_of_nonexecutable_pages: usize, @@ -31,6 +34,7 @@ impl CodeMemory { unwind_registry: UnwindRegistry::new(), mmap: Mmap::new(), start_of_nonexecutable_pages: 0, + frame_info_registration: None, } } @@ -207,6 +211,11 @@ impl CodeMemory { let body_ptr = byte_ptr as *mut [VMFunctionBody]; unsafe { &mut *body_ptr } } + + /// Register the frame info, so it's free when the mememory gets freed + pub fn register_frame_info(&mut self, frame_info: GlobalFrameInfoRegistration) { + self.frame_info_registration = Some(frame_info); + } } fn round_up(size: usize, multiple: usize) -> usize { diff --git a/lib/compiler/src/engine/inner.rs b/lib/compiler/src/engine/inner.rs index 21679f004be..05c934a43c3 100644 --- a/lib/compiler/src/engine/inner.rs +++ b/lib/compiler/src/engine/inner.rs @@ -7,6 +7,8 @@ use crate::Artifact; use crate::BaseTunables; #[cfg(not(target_arch = "wasm32"))] use crate::CodeMemory; +#[cfg(not(target_arch = "wasm32"))] +use crate::GlobalFrameInfoRegistration; #[cfg(feature = "compiler")] use crate::{Compiler, CompilerConfig}; #[cfg(not(target_arch = "wasm32"))] @@ -442,6 +444,15 @@ impl EngineInner { pub fn signatures(&self) -> &SignatureRegistry { &self.signatures } + + #[cfg(not(target_arch = "wasm32"))] + /// Register the frame info for the code memory + pub(crate) fn register_frame_info(&mut self, frame_info: GlobalFrameInfoRegistration) { + self.code_memory + .last_mut() + .unwrap() + .register_frame_info(frame_info); + } } #[cfg(feature = "compiler")] From d216a65e2a4d82dbcde595297cdca8a534c748f5 Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Tue, 27 Jun 2023 10:54:15 +0200 Subject: [PATCH 4/6] Update the API and added more comments --- lib/compiler/src/engine/artifact.rs | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index 9da9b5016ad..d0ce3e05019 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -37,8 +37,14 @@ use wasmer_vm::{FunctionBodyPtr, MemoryStyle, TableStyle, VMSharedSignatureIndex use wasmer_vm::{InstanceAllocator, StoreObjects, TrapHandlerFn, VMConfig, VMExtern, VMInstance}; pub struct AllocatedArtifact { + // This shows if the frame info has been regestered already or not. + // Because the 'GlobalFrameInfoRegistration' ownership can be transfered to EngineInner + // this bool is needed to track the status, as 'frame_info_registration' will be None + // after the ownership is transfered. frame_info_registered: bool, - // frame_info_registered is not staying there but transfered to InnerEnginge / CodeMemory + // frame_info_registered is not staying there but transfered to CodeMemory from EngineInner + // using 'Artifact::transfert_frame_info_registration' method + // so the GloabelFrameInfo and MMap stays in sync and get dropped at the same time frame_info_registration: Option, finished_functions: BoxedSlice, finished_function_call_trampolines: BoxedSlice, @@ -316,7 +322,7 @@ impl Artifact { }; artifact.internal_register_frame_info(); - if let Some(frame_info) = artifact.transfert_frame_info_registration() { + if let Some(frame_info) = artifact.internal_transfert_frame_info_registration() { engine_inner.register_frame_info(frame_info); } @@ -387,7 +393,10 @@ impl Artifact { /// Register thie `Artifact` stack frame information into the global scope. /// /// This is not required anymore as it's done automaticaly when creating by 'Artifact::from_parts' - #[deprecated(since = "4.1.0", note = "done automaticaly by Artifact::from_parts")] + #[deprecated( + since = "4.0.0", + note = "done automaticaly by Artifact::from_parts, use 'transfert_frame_info_registration' if you use this method" + )] pub fn register_frame_info(&mut self) { self.internal_register_frame_info() } @@ -442,8 +451,15 @@ impl Artifact { } /// The GlobalFrameInfoRegistration needs to be transfered to EngineInner if - /// deprecated register_frame_info has been used. + /// register_frame_info has been used. + #[deprecated(since = "4.0.0", note = "done automaticaly by Artifact::from_parts.")] pub fn transfert_frame_info_registration(&mut self) -> Option { + self.internal_transfert_frame_info_registration() + } + + fn internal_transfert_frame_info_registration( + &mut self, + ) -> Option { let frame_info_registration = &mut self .allocated .as_mut() From 4bcbb41e1456366238851ab8912b9289c8488c5b Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Tue, 27 Jun 2023 14:41:31 +0200 Subject: [PATCH 5/6] Fixed spelling mystake --- lib/compiler/src/engine/artifact.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index d0ce3e05019..0e05d851f7a 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -43,7 +43,7 @@ pub struct AllocatedArtifact { // after the ownership is transfered. frame_info_registered: bool, // frame_info_registered is not staying there but transfered to CodeMemory from EngineInner - // using 'Artifact::transfert_frame_info_registration' method + // using 'Artifact::transfer_frame_info_registration' method // so the GloabelFrameInfo and MMap stays in sync and get dropped at the same time frame_info_registration: Option, finished_functions: BoxedSlice, @@ -322,7 +322,7 @@ impl Artifact { }; artifact.internal_register_frame_info(); - if let Some(frame_info) = artifact.internal_transfert_frame_info_registration() { + if let Some(frame_info) = artifact.internal_transfer_frame_info_registration() { engine_inner.register_frame_info(frame_info); } @@ -395,7 +395,7 @@ impl Artifact { /// This is not required anymore as it's done automaticaly when creating by 'Artifact::from_parts' #[deprecated( since = "4.0.0", - note = "done automaticaly by Artifact::from_parts, use 'transfert_frame_info_registration' if you use this method" + note = "done automaticaly by Artifact::from_parts, use 'transfer_frame_info_registration' if you use this method" )] pub fn register_frame_info(&mut self) { self.internal_register_frame_info() @@ -453,13 +453,11 @@ impl Artifact { /// The GlobalFrameInfoRegistration needs to be transfered to EngineInner if /// register_frame_info has been used. #[deprecated(since = "4.0.0", note = "done automaticaly by Artifact::from_parts.")] - pub fn transfert_frame_info_registration(&mut self) -> Option { - self.internal_transfert_frame_info_registration() + pub fn transfer_frame_info_registration(&mut self) -> Option { + self.internal_transfer_frame_info_registration() } - fn internal_transfert_frame_info_registration( - &mut self, - ) -> Option { + fn internal_transfer_frame_info_registration(&mut self) -> Option { let frame_info_registration = &mut self .allocated .as_mut() From 3bab6e5942a1ee059d539128519906fa238f2a92 Mon Sep 17 00:00:00 2001 From: ptitSeb Date: Tue, 27 Jun 2023 15:04:02 +0200 Subject: [PATCH 6/6] Rename method to take_frame_info_registration --- lib/compiler/src/engine/artifact.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/compiler/src/engine/artifact.rs b/lib/compiler/src/engine/artifact.rs index 0e05d851f7a..7489240a85e 100644 --- a/lib/compiler/src/engine/artifact.rs +++ b/lib/compiler/src/engine/artifact.rs @@ -43,7 +43,7 @@ pub struct AllocatedArtifact { // after the ownership is transfered. frame_info_registered: bool, // frame_info_registered is not staying there but transfered to CodeMemory from EngineInner - // using 'Artifact::transfer_frame_info_registration' method + // using 'Artifact::take_frame_info_registration' method // so the GloabelFrameInfo and MMap stays in sync and get dropped at the same time frame_info_registration: Option, finished_functions: BoxedSlice, @@ -322,7 +322,7 @@ impl Artifact { }; artifact.internal_register_frame_info(); - if let Some(frame_info) = artifact.internal_transfer_frame_info_registration() { + if let Some(frame_info) = artifact.internal_take_frame_info_registration() { engine_inner.register_frame_info(frame_info); } @@ -395,7 +395,7 @@ impl Artifact { /// This is not required anymore as it's done automaticaly when creating by 'Artifact::from_parts' #[deprecated( since = "4.0.0", - note = "done automaticaly by Artifact::from_parts, use 'transfer_frame_info_registration' if you use this method" + note = "done automaticaly by Artifact::from_parts, use 'take_frame_info_registration' if you use this method" )] pub fn register_frame_info(&mut self) { self.internal_register_frame_info() @@ -453,11 +453,11 @@ impl Artifact { /// The GlobalFrameInfoRegistration needs to be transfered to EngineInner if /// register_frame_info has been used. #[deprecated(since = "4.0.0", note = "done automaticaly by Artifact::from_parts.")] - pub fn transfer_frame_info_registration(&mut self) -> Option { - self.internal_transfer_frame_info_registration() + pub fn take_frame_info_registration(&mut self) -> Option { + self.internal_take_frame_info_registration() } - fn internal_transfer_frame_info_registration(&mut self) -> Option { + fn internal_take_frame_info_registration(&mut self) -> Option { let frame_info_registration = &mut self .allocated .as_mut()