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

Instead of applying symbols and comments in the applier step, we will do it when the matched function is identified.

This has the side effect that if you turn off the apply activity names and comments will still be applied, more work to be done later.

Author: emesare

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;