[WARP] Move symbol and comment application to when matched functions …

plugins/warp/src/cache/function.rs

@@ -1,17 +1,42 @@
+use crate::convert::{comment_to_bn_comment, to_bn_symbol_at_address};
+use binaryninja::binary_view::BinaryViewExt;
 use binaryninja::function::{Function as BNFunction, FunctionUpdateType};
+use binaryninja::symbol::SymbolType;
 use warp::signature::function::Function;
 
-/// Inserts a function match into the cache.
+// TODO: Rename this?
+/// Inserts a function match into the cache. This also has the side effect of setting persisted function
+/// information, such as the matched function symbol.
 ///
 /// IMPORTANT: This will mark the function as needing updates, if you intend to fill in functions with
 /// no match (i.e. `None`), then you must change this function to prevent marking that as needing updates.
 /// However, it's perfectly valid to remove a match and need to update the function still, so be careful.
 pub fn insert_cached_function_match(function: &BNFunction, matched_function: Option<Function>) {
+    let view = function.view();
+    let function_start = function.start();
     // NOTE: If we expect to run match_function multiple times on a function, we should move this elsewhere.
     // Mark the function as needing updates so that reanalysis occurs on the function, and we apply the match.
     function.mark_updates_required(FunctionUpdateType::FullAutoFunctionUpdate);
+    if let Some(auto_sym) = view.symbol_by_address(function_start) {
+        // TODO: If we ever create non library function symbols we will need to remove this check (see: `to_bn_symbol_at_address`).
+        if auto_sym.sym_type() == SymbolType::LibraryFunction {
+            // NOTE: This will also mark for full auto function update, one thing to note is that the
+            // requirement to call this is that this function not be called in the associated function's analysis.
+            view.undefine_auto_symbol(&auto_sym);
+        }
+    }
     match matched_function {
         Some(matched_function) => {
+            // Define the new matched function symbol, this can safely be done here as the symbol itself
+            // will be persisted, unlike function type information or variable information.
+            let new_sym = to_bn_symbol_at_address(&view, &matched_function.symbol, function_start);
+            view.define_auto_symbol(&new_sym);
+            // TODO: How to clear the comments? They are just persisted.
+            // TODO: Also they generate an undo action, i hate implicit undo actions so much.
+            for comment in &matched_function.comments {
+                let bn_comment = comment_to_bn_comment(&function, comment.clone());
+                function.set_comment_at(bn_comment.addr, &bn_comment.comment);
+            }
             function.store_metadata("warp_matched_function", &matched_function.to_bytes(), false);
         }
         None => {

plugins/warp/src/plugin/ffi/function.rs

@@ -67,15 +67,15 @@ pub unsafe extern "C" fn BNWARPFunctionApply(
     let analysis_function = Function::from_raw(analysis_function);
     match function.is_null() {
         false => {
-            // Set the matched function to `function` and return previous.
+            // Set the matched function to `function`.
             let matched_function = ManuallyDrop::new(Arc::from_raw(function));
             insert_cached_function_match(
                 &analysis_function,
                 Some(matched_function.as_ref().clone()),
             )
         }
         true => {
-            // We are removing the previous match and returning it.
+            // We are removing the previous match.
             insert_cached_function_match(&analysis_function, None)
         }
     };

plugins/warp/src/plugin/workflow.rs

@@ -3,9 +3,7 @@ use crate::cache::{
     cached_function_guid, insert_cached_function_match, try_cached_function_guid,
     try_cached_function_match,
 };
-use crate::convert::{
-    comment_to_bn_comment, platform_to_target, to_bn_symbol_at_address, to_bn_type,
-};
+use crate::convert::{platform_to_target, to_bn_type};
 use crate::matcher::{Matcher, MatcherSettings};
 use crate::{get_warp_tag_type, relocatable_regions};
 use binaryninja::architecture::RegisterId;
@@ -192,29 +190,20 @@ pub fn run_matcher(view: &BinaryView) {
 }
 
 pub fn insert_workflow() {
+    // TODO: Note: because of symbol persistence function symbol is applied in `insert_cached_function_match`.
+    // TODO: Comments are also applied there, they are "user" like, persisted and make undo actions.
     // "Hey look, it's a plier" ~ Josh 2025
     let apply_activity = |ctx: &AnalysisContext| {
         let view = ctx.view();
         let function = ctx.function();
         if let Some(matched_function) = try_cached_function_match(&function) {
-            view.define_auto_symbol(&to_bn_symbol_at_address(
-                &view,
-                &matched_function.symbol,
-                function.symbol().address(),
-            ));
             // core.function.propagateAnalysis will assign user type info to auto, so we must not apply
             // otherwise we will wipe over user type info.
             if !function.has_user_type() {
                 if let Some(func_ty) = &matched_function.ty {
                     function.set_auto_type(&to_bn_type(&function.arch(), func_ty));
                 }
             }
-            // TODO: How to clear the comments? They are just persisted.
-            // TODO: Also they generate an undo action, i hate implicit undo actions so much.
-            for comment in matched_function.comments {
-                let bn_comment = comment_to_bn_comment(&function, comment);
-                function.set_comment_at(bn_comment.addr, &bn_comment.comment);
-            }
             if let Some(mlil) = ctx.mlil_function() {
                 for variable in matched_function.variables {
                     let decl_addr = ((function.start() as i64) + variable.offset) as u64;