Replace unsafe RefCells with RwLocks

ergrelet · Mar 10, 2024 · 3584efc · 3584efc
1 parent 0ee1e17
commit 3584efc
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 57 deletions.
diff --git a/resym/src/ui_components/module_tree.rs b/resym/src/ui_components/module_tree.rs
@@ -10,6 +10,7 @@ use crate::{
 };
 
 /// UI component in charge of rendering a tree of PDB modules
+/// Warning: not thread-safe, use only in single-threaded contexts
 pub struct ModuleTreeComponent {
     /// Tree data
     module_tree_view: ModuleTreeView,

diff --git a/resym_core/src/backend.rs b/resym_core/src/backend.rs
@@ -11,7 +11,6 @@ use rayon::{
 #[cfg(all(not(feature = "rayon"), not(target_arch = "wasm32")))]
 use std::thread::{self, JoinHandle};
 use std::{
-    cell::RefCell,
     collections::{BTreeSet, HashMap},
     io,
     sync::Arc,
@@ -173,7 +172,7 @@ fn worker_thread_routine(
     rx_worker: Receiver<BackendCommand>,
     frontend_controller: Arc<impl FrontendController + Send + Sync + 'static>,
 ) -> Result<()> {
-    let mut pdb_files: HashMap<PDBSlot, RefCell<PdbFile<PDBDataSource>>> = HashMap::new();
+    let mut pdb_files: HashMap<PDBSlot, PdbFile<PDBDataSource>> = HashMap::new();
     while let Ok(command) = rx_worker.recv() {
         match command {
             #[cfg(not(target_arch = "wasm32"))]
@@ -185,11 +184,8 @@ fn worker_thread_routine(
                     Ok(loaded_pdb_file) => {
                         frontend_controller
                             .send_command(FrontendCommand::LoadPDBResult(Ok(pdb_slot)))?;
-                        if let Some(pdb_file) = pdb_files.insert(pdb_slot, loaded_pdb_file.into()) {
-                            log::info!(
-                                "'{}' has been unloaded.",
-                                pdb_file.borrow().file_path.display()
-                            );
+                        if let Some(pdb_file) = pdb_files.insert(pdb_slot, loaded_pdb_file) {
+                            log::info!("'{}' has been unloaded.", pdb_file.file_path.display());
                         }
                         log::info!(
                             "'{}' has been loaded successfully!",
@@ -207,11 +203,8 @@ fn worker_thread_routine(
                     Ok(loaded_pdb_file) => {
                         frontend_controller
                             .send_command(FrontendCommand::LoadPDBResult(Ok(pdb_slot)))?;
-                        if let Some(pdb_file) = pdb_files.insert(pdb_slot, loaded_pdb_file.into()) {
-                            log::info!(
-                                "'{}' has been unloaded.",
-                                pdb_file.borrow().file_path.display()
-                            );
+                        if let Some(pdb_file) = pdb_files.insert(pdb_slot, loaded_pdb_file) {
+                            log::info!("'{}' has been unloaded.", pdb_file.file_path.display());
                         }
                         log::info!("'{}' has been loaded successfully!", pdb_name);
                     }
@@ -226,11 +219,8 @@ fn worker_thread_routine(
                     Ok(loaded_pdb_file) => {
                         frontend_controller
                             .send_command(FrontendCommand::LoadPDBResult(Ok(pdb_slot)))?;
-                        if let Some(pdb_file) = pdb_files.insert(pdb_slot, loaded_pdb_file.into()) {
-                            log::info!(
-                                "'{}' has been unloaded.",
-                                pdb_file.borrow().file_path.display()
-                            );
+                        if let Some(pdb_file) = pdb_files.insert(pdb_slot, loaded_pdb_file) {
+                            log::info!("'{}' has been unloaded.", pdb_file.file_path.display());
                         }
                         log::info!("'{}' has been loaded successfully!", pdb_name);
                     }
@@ -279,10 +269,7 @@ fn worker_thread_routine(
                     log::error!("Trying to unload an inexistent PDB");
                 }
                 Some(pdb_file) => {
-                    log::info!(
-                        "'{}' has been unloaded.",
-                        pdb_file.borrow().file_path.display()
-                    );
+                    log::info!("'{}' has been unloaded.", pdb_file.file_path.display());
                 }
             },
 
@@ -296,7 +283,7 @@ fn worker_thread_routine(
             ) => {
                 if let Some(pdb_file) = pdb_files.get(&pdb_slot) {
                     let reconstructed_type_result = reconstruct_type_by_index_command(
-                        &pdb_file.borrow(),
+                        pdb_file,
                         type_index,
                         primitives_flavor,
                         print_header,
@@ -319,7 +306,7 @@ fn worker_thread_routine(
             ) => {
                 if let Some(pdb_file) = pdb_files.get(&pdb_slot) {
                     let reconstructed_type_result = reconstruct_type_by_name_command(
-                        &pdb_file.borrow(),
+                        pdb_file,
                         &type_name,
                         primitives_flavor,
                         print_header,
@@ -340,7 +327,7 @@ fn worker_thread_routine(
             ) => {
                 if let Some(pdb_file) = pdb_files.get(&pdb_slot) {
                     let reconstructed_type_result = reconstruct_all_types_command(
-                        &pdb_file.borrow(),
+                        pdb_file,
                         primitives_flavor,
                         print_header,
                         print_access_specifiers,
@@ -359,7 +346,7 @@ fn worker_thread_routine(
             ) => {
                 if let Some(pdb_file) = pdb_files.get(&pdb_slot) {
                     let filtered_type_list = update_type_filter_command(
-                        &pdb_file.borrow(),
+                        pdb_file,
                         &search_filter,
                         case_insensitive_search,
                         use_regex,
@@ -380,7 +367,7 @@ fn worker_thread_routine(
                 for pdb_slot in pdb_slots {
                     if let Some(pdb_file) = pdb_files.get(&pdb_slot) {
                         let filtered_type_list = update_type_filter_command(
-                            &pdb_file.borrow(),
+                            pdb_file,
                             &search_filter,
                             case_insensitive_search,
                             use_regex,
@@ -407,7 +394,7 @@ fn worker_thread_routine(
             ) => {
                 if let Some(pdb_file) = pdb_files.get_mut(&pdb_slot) {
                     let reconstructed_module_result = reconstruct_module_by_index_command(
-                        &mut pdb_file.borrow_mut(),
+                        pdb_file,
                         module_index,
                         primitives_flavor,
                         print_header,
@@ -426,7 +413,7 @@ fn worker_thread_routine(
             ) => {
                 if let Some(pdb_file) = pdb_files.get(&pdb_slot) {
                     let module_list = list_modules_command(
-                        &pdb_file.borrow(),
+                        pdb_file,
                         &search_filter,
                         case_insensitive_search,
                         use_regex,
@@ -448,8 +435,8 @@ fn worker_thread_routine(
                 if let Some(pdb_file_from) = pdb_files.get(&pdb_from_slot) {
                     if let Some(pdb_file_to) = pdb_files.get(&pdb_to_slot) {
                         let type_diff_result = diff_type_by_name(
-                            &pdb_file_from.borrow(),
-                            &pdb_file_to.borrow(),
+                            pdb_file_from,
+                            pdb_file_to,
                             &type_name,
                             primitives_flavor,
                             print_header,
@@ -472,8 +459,8 @@ fn worker_thread_routine(
                 if let Some(pdb_file_from) = pdb_files.get(&pdb_from_slot) {
                     if let Some(pdb_file_to) = pdb_files.get(&pdb_to_slot) {
                         let module_diff_result = diff_module_by_path(
-                            &mut pdb_file_from.borrow_mut(),
-                            &mut pdb_file_to.borrow_mut(),
+                            pdb_file_from,
+                            pdb_file_to,
                             &module_path,
                             primitives_flavor,
                             print_header,
@@ -486,7 +473,7 @@ fn worker_thread_routine(
 
             BackendCommand::ListTypeCrossReferences(pdb_slot, type_index) => {
                 if let Some(pdb_file) = pdb_files.get(&pdb_slot) {
-                    let xref_list = list_type_xrefs_command(&mut pdb_file.borrow_mut(), type_index);
+                    let xref_list = list_type_xrefs_command(pdb_file, type_index);
                     frontend_controller
                         .send_command(FrontendCommand::ListTypeCrossReferencesResult(xref_list))?;
                 }
@@ -769,7 +756,7 @@ fn filter_modules_regular(
 }
 
 fn list_type_xrefs_command<'p, T>(
-    pdb_file: &mut PdbFile<'p, T>,
+    pdb_file: &PdbFile<'p, T>,
     type_index: pdb::TypeIndex,
 ) -> Result<Vec<(String, pdb::TypeIndex)>>
 where

diff --git a/resym_core/src/diffing.rs b/resym_core/src/diffing.rs
@@ -83,8 +83,8 @@ where
 }
 
 pub fn diff_module_by_path<'p, T>(
-    pdb_file_from: &mut PdbFile<'p, T>,
-    pdb_file_to: &mut PdbFile<'p, T>,
+    pdb_file_from: &PdbFile<'p, T>,
+    pdb_file_to: &PdbFile<'p, T>,
     module_path: &str,
     primitives_flavor: PrimitiveReconstructionFlavor,
     print_header: bool,

diff --git a/resym_core/src/pdb_file.rs b/resym_core/src/pdb_file.rs
@@ -9,7 +9,7 @@ use std::{
     collections::BTreeSet,
     io::{self, Read, Seek},
     path::PathBuf,
-    sync::Arc,
+    sync::{Arc, RwLock},
 };
 #[cfg(not(target_arch = "wasm32"))]
 use std::{fs::File, path::Path, time::Instant};
@@ -62,8 +62,8 @@ where
     pub type_information: pdb::TypeInformation<'p>,
     pub debug_information: pdb::DebugInformation<'p>,
     pub file_path: PathBuf,
-    pub xref_map: DashMap<pdb::TypeIndex, Vec<pdb::TypeIndex>>,
-    _pdb: pdb::PDB<'p, T>,
+    pub xref_map: RwLock<DashMap<pdb::TypeIndex, Vec<pdb::TypeIndex>>>,
+    pdb: RwLock<pdb::PDB<'p, T>>,
 }
 
 #[cfg(not(target_arch = "wasm32"))]
@@ -83,8 +83,8 @@ impl<'p> PdbFile<'p, File> {
             type_information,
             debug_information,
             file_path: pdb_file_path.to_owned(),
-            xref_map: DashMap::default(),
-            _pdb: pdb,
+            xref_map: DashMap::default().into(),
+            pdb: pdb.into(),
         };
         pdb_file.load_symbols()?;
 
@@ -111,8 +111,8 @@ impl<'p> PdbFile<'p, PDBDataSource> {
             type_information,
             debug_information,
             file_path: pdb_file_name.into(),
-            xref_map: DashMap::default(),
-            _pdb: pdb,
+            xref_map: DashMap::default().into(),
+            pdb: pdb.into(),
         };
         pdb_file.load_symbols()?;
 
@@ -137,8 +137,8 @@ impl<'p> PdbFile<'p, PDBDataSource> {
             type_information,
             debug_information,
             file_path: pdb_file_name.into(),
-            xref_map: DashMap::default(),
-            _pdb: pdb,
+            xref_map: DashMap::default().into(),
+            pdb: pdb.into(),
         };
         pdb_file.load_symbols()?;
 
@@ -371,7 +371,7 @@ where
     }
 
     pub fn reconstruct_module_by_path(
-        &mut self,
+        &self,
         module_path: &str,
         primitives_flavor: PrimitiveReconstructionFlavor,
     ) -> Result<String> {
@@ -389,7 +389,7 @@ where
     }
 
     pub fn reconstruct_module_by_index(
-        &mut self,
+        &self,
         module_index: usize,
         primitives_flavor: PrimitiveReconstructionFlavor,
     ) -> Result<String> {
@@ -398,12 +398,17 @@ where
             ResymCoreError::ModuleInfoNotFoundError(format!("Module #{} not found", module_index))
         })?;
 
-        let module_info = self._pdb.module_info(&module)?.ok_or_else(|| {
-            ResymCoreError::ModuleInfoNotFoundError(format!(
-                "No module information present for '{}'",
-                module.object_file_name()
-            ))
-        })?;
+        let module_info = self
+            .pdb
+            .write()
+            .expect("lock shouldn't be poisoned")
+            .module_info(&module)?
+            .ok_or_else(|| {
+                ResymCoreError::ModuleInfoNotFoundError(format!(
+                    "No module information present for '{}'",
+                    module.object_file_name()
+                ))
+            })?;
 
         // Populate our `TypeFinder`
         let mut type_finder = self.type_information.finder();
@@ -611,11 +616,16 @@ where
     }
 
     pub fn get_xrefs_for_type(
-        &mut self,
+        &self,
         type_index: pdb::TypeIndex,
     ) -> Result<Vec<(String, pdb::TypeIndex)>> {
         // Generate xref cache if empty
-        if self.xref_map.is_empty() {
+        if self
+            .xref_map
+            .read()
+            .expect("lock shouldn't be poisoned")
+            .is_empty()
+        {
             // Populate our `TypeFinder`
             let mut type_finder = self.type_information.finder();
             {
@@ -651,11 +661,18 @@ where
             }
 
             // Update cache
-            self.xref_map = xref_map;
+            if let Ok(mut xref_map_ref) = self.xref_map.write() {
+                *xref_map_ref = xref_map;
+            }
         }
 
         // Query xref cache
-        if let Some(xref_list) = self.xref_map.get(&type_index) {
+        if let Some(xref_list) = self
+            .xref_map
+            .read()
+            .expect("lock shouldn't be poisoned")
+            .get(&type_index)
+        {
             // Convert the xref list into a proper Name+TypeIndex tuple list
             let xref_type_list = xref_list
                 .iter()