More rust cleanup

- Fixed the issue with PDB types, this has caused me an insane amount of grief
- Fixed LLIL visit_tree missing LLIL_LOAD expressions
- Added LLIL visitor test
- Made all WARP file pickers use the rfd crate

Author: emesare

plugins/pdb-ng/src/struct_grouper.rs

@@ -360,7 +360,7 @@ pub fn group_structure(
             warn!("{} Could not resolve structure groups: {}", name, e);
             for member in members {
                 structure.insert(
-                    &member.ty.clone(),
+                    &member.ty,
                     member.name.clone(),
                     member.offset,
                     false,
@@ -389,7 +389,7 @@ fn apply_groups(
 
                 if offset > member.offset {
                     structure.insert(
-                        &member.ty.clone(),
+                        &member.ty,
                         member.name.clone(),
                         0,
                         false,
@@ -398,7 +398,7 @@ fn apply_groups(
                     );
                 } else {
                     structure.insert(
-                        &member.ty.clone(),
+                        &member.ty,
                         member.name.clone(),
                         member.offset - offset,
                         false,
@@ -449,6 +449,7 @@ fn resolve_struct_groups(members: Vec<MemberSize>) -> Result<Vec<ResolvedGroup>>
         max_width = max_width.max(member.offset + member.width);
     }
 
+    // TODO: has overlapping is probably failing.
     if !has_overlapping {
         // Nothing overlaps, just add em directly
         return Ok(members

plugins/pdb-ng/src/type_parser.rs

@@ -2075,7 +2075,7 @@ impl<'a, S: Source<'a> + 'a> PDBParserInstance<'a, S> {
     fn is_name_anonymous(name: &QualifiedName) -> bool {
         match name.items.last() {
             Some(item) if item == "<anonymous-tag>" => true,
-            Some(item) if item.starts_with("<unnamed-") => true,
+            Some(item) if item.contains("<unnamed-") => true,
             _ => false,
         }
     }
@@ -2210,6 +2210,7 @@ impl<'a, S: Source<'a> + 'a> PDBParserInstance<'a, S> {
         // No? We're just going to do something close and leave it to the users to figure out the rest
         // There's no way I'm digging through all nonsense
 
+        // Glenn: "Never mind this got deleted... MS has done it again"
         // After a quick GitHub discussion (https://github.com/MicrosoftDocs/cpp-docs/issues/4152)
         // I've determined this is unknowable.
         // Microsoft does it again!!!!

plugins/warp/benches/convert.rs

@@ -25,7 +25,7 @@ pub fn type_conversion_benchmark(c: &mut Criterion) {
                 c.bench_function("type conversion all types", |b| {
                     b.iter(|| {
                         for ty in &types {
-                            from_bn_type(&bv, &ty.type_object(), u8::MAX);
+                            from_bn_type(&bv, &ty.ty, u8::MAX);
                         }
                     })
                 });

plugins/warp/src/lib.rs

@@ -128,6 +128,7 @@ pub fn basic_block_guid<A: Architecture, M: FunctionMutability>(
 
     let is_variant_instr = |instr: &LowLevelILInstruction<A, M, NonSSA<RegularNonSSA>>| {
         let is_variant_expr = |expr: &LowLevelILExpressionKind<A, M, NonSSA<RegularNonSSA>>| {
+            // TODO: Checking the section here is slow, we should gather all section ranges outside of this.
             match expr {
                 LowLevelILExpressionKind::ConstPtr(op)
                     if !view.sections_at(op.value()).is_empty() =>

plugins/warp/src/plugin/load.rs

@@ -10,8 +10,12 @@ impl Command for LoadSignatureFile {
             return;
         };
 
-        let Some(file) =
-            binaryninja::interaction::get_open_filename_input("Load Signature File", "*.sbin")
+        // NOTE: Because we only can consume signatures from a specific directory, we don't need to use the interaction API.
+        // If we did need to load signature files from a project than this would need to change.
+        let Some(file) = rfd::FileDialog::new()
+            .add_filter("Signature Files", &["sbin"])
+            .set_file_name(format!("{}.sbin", view.file().filename()))
+            .pick_file()
         else {
             return;
         };

plugins/warp/src/plugin/types.rs

@@ -7,10 +7,13 @@ pub struct LoadTypes;
 
 impl Command for LoadTypes {
     fn action(&self, view: &BinaryView) {
-        let Some(file) = binaryninja::interaction::get_open_filename_input(
-            "Apply Signature File Types",
-            "*.sbin",
-        ) else {
+        // NOTE: Because we only can consume signatures from a specific directory, we don't need to use the interaction API.
+        // If we did need to load signature files from a project than this would need to change.
+        let Some(file) = rfd::FileDialog::new()
+            .add_filter("Signature Files", &["sbin"])
+            .set_file_name(format!("{}.sbin", view.file().filename()))
+            .pick_file()
+        else {
             return;
         };
 

rust/src/low_level_il/expression.rs

@@ -130,14 +130,14 @@ where
     where
         T: FnMut(&LowLevelILExpression<'func, A, M, SSA, ValueExpr>) -> VisitorAction,
     {
-        // First visit the current expression.
-        let info = self.kind();
+        // Visit the current expression.
         match f(self) {
-            VisitorAction::Descend => {}
-            action => return action,
+            VisitorAction::Descend => {
+                // Recursively visit sub expressions.
+                self.kind().visit_sub_expressions(|e| e.visit_tree(f))
+            }
+            action => action,
         }
-        // Recursively visit sub expressions.
-        info.visit_sub_expressions(|e| e.visit_tree(f))
     }
 }
 
@@ -165,14 +165,14 @@ where
             &LowLevelILExpression<'func, A, M, NonSSA<LiftedNonSSA>, ValueExpr>,
         ) -> VisitorAction,
     {
-        // First visit the current expression.
-        let info = self.kind();
+        // Visit the current expression.
         match f(self) {
-            VisitorAction::Descend => {}
-            action => return action,
+            VisitorAction::Descend => {
+                // Recursively visit sub expressions.
+                self.kind().visit_sub_expressions(|e| e.visit_tree(f))
+            }
+            action => action,
         }
-        // Recursively visit sub expressions.
-        info.visit_sub_expressions(|e| e.visit_tree(f))
     }
 }
 
@@ -200,14 +200,14 @@ where
             &LowLevelILExpression<'func, A, M, NonSSA<RegularNonSSA>, ValueExpr>,
         ) -> VisitorAction,
     {
-        // First visit the current expression.
-        let info = self.kind();
+        // Visit the current expression.
         match f(self) {
-            VisitorAction::Descend => {}
-            action => return action,
+            VisitorAction::Descend => {
+                // Recursively visit sub expressions.
+                self.kind().visit_sub_expressions(|e| e.visit_tree(f))
+            }
+            action => action,
         }
-        // Recursively visit sub expressions.
-        info.visit_sub_expressions(|e| e.visit_tree(f))
     }
 }
 
@@ -592,7 +592,11 @@ where
             UnimplMem(ref op) => {
                 visit!(op.mem_expr());
             }
-            _ => {}
+            Load(ref op) => {
+                visit!(op.source_mem_expr());
+            }
+            // Do not have any sub expressions.
+            Pop(_) | Reg(_) | RegSplit(_) | Const(_) | ConstPtr(_) | Flag(_) | FlagBit(_) | ExternPtr(_) | FlagCond(_) | FlagGroup(_) | Unimpl(_) | Undef(_) => {}
         }
 
         VisitorAction::Sibling

rust/src/low_level_il/instruction.rs

@@ -20,7 +20,6 @@ use binaryninjacore_sys::BNGetLowLevelILByIndex;
 use binaryninjacore_sys::BNGetLowLevelILIndexForInstruction;
 use binaryninjacore_sys::BNLowLevelILInstruction;
 use std::fmt::{Debug, Display, Formatter};
-
 use super::VisitorAction;
 
 #[repr(transparent)]
@@ -340,7 +339,8 @@ where
                 // TODO: Visit when we support expression lists
             }
             Value(e) => visit!(e),
-            _ => {}
+            // Do not have any sub expressions.
+            Nop(_) | NoRet(_) | Goto(_) | Syscall(_) | Bp(_) | Trap(_) | Undef(_) => {}
         }
 
         VisitorAction::Sibling

rust/tests/low_level_il.rs

@@ -7,7 +7,7 @@ use binaryninja::low_level_il::expression::{
 use binaryninja::low_level_il::instruction::{
     InstructionHandler, LowLevelILInstructionKind, LowLevelInstructionIndex,
 };
-use binaryninja::low_level_il::LowLevelILRegister;
+use binaryninja::low_level_il::{LowLevelILRegister, VisitorAction};
 use rstest::*;
 use std::path::PathBuf;
 
@@ -168,3 +168,55 @@ fn test_llil_info(_session: &Session) {
         _ => panic!("Expected Ret"),
     }
 }
+
+#[rstest]
+fn test_llil_visitor(_session: &Session) {
+    let out_dir = env!("OUT_DIR").parse::<PathBuf>().unwrap();
+    let view = binaryninja::load(out_dir.join("atox.obj")).expect("Failed to create view");
+    let platform = view.default_platform().unwrap();
+
+    // Sample function: __crt_strtox::c_string_character_source<char>::validate
+    let sample_function = view.function_at(&platform, 0x2bd80).unwrap();
+    let llil_function = sample_function.low_level_il().unwrap();
+    let llil_basic_blocks = llil_function.basic_blocks();
+    let llil_basic_block_iter = llil_basic_blocks.iter();
+
+    let mut basic_blocks_visited = 0;
+    let mut instructions_visited: Vec<LowLevelInstructionIndex> = vec![];
+    let mut expressions_visited: Vec<LowLevelExpressionIndex> = vec![];
+    for basic_block in llil_basic_block_iter {
+        basic_blocks_visited += 1;
+        for instr in basic_block.iter() {
+            instructions_visited.push(instr.index);
+            expressions_visited.push(instr.expr_idx());
+            instr.visit_tree(&mut |expr| {
+                expressions_visited.push(expr.index);
+                VisitorAction::Descend
+            });
+        }
+    }
+
+    assert_eq!(basic_blocks_visited, 10);
+    // This is a flag instruction removed in LLIL.
+    instructions_visited.push(LowLevelInstructionIndex(38));
+    for instr_idx in 0..41 {
+        if instructions_visited.iter().find(|x| x.0 == instr_idx).is_none() {
+            panic!("Instruction with index {:?} not visited", instr_idx);
+        };
+    }
+    // These are NOP's
+    expressions_visited.push(LowLevelExpressionIndex(24));
+    expressions_visited.push(LowLevelExpressionIndex(54));
+    expressions_visited.push(LowLevelExpressionIndex(62));
+    expressions_visited.push(LowLevelExpressionIndex(87));
+    // These are some flag things
+    expressions_visited.push(LowLevelExpressionIndex(114));
+    expressions_visited.push(LowLevelExpressionIndex(115));
+    expressions_visited.push(LowLevelExpressionIndex(116));
+    expressions_visited.push(LowLevelExpressionIndex(121));
+    for expr_idx in 0..127 {
+        if expressions_visited.iter().find(|x| x.0 == expr_idx).is_none() {
+            panic!("Expression with index {:?} not visited", expr_idx);
+        };
+    }
+}