[MIPS] Fix symbols being filtered out from target side of diff if target object contains .NON_MATCHING markers (#250)

* Fix filtering out symbols from the target side that have a symbol with the same name and a `.NON_MATCHING` suffix.

- Target side: Show all the symbols except the `.NON_MATCHING` ones.
- Base side: Ignore all the `.NON_MATCHING` symbols and also ignore the ones with the same name without the suffix

* fmt

* comment

* tests

* fmt tests

* maybe this could fix wasm?

* Fix wasm?

* fmt

* Move `DiffSide` to `diff` mod

* Update the stuff the advisories CI told me to

Author: AngheloAlf

objdiff-cli/src/cmd/report.rs

@@ -8,8 +8,8 @@ use objdiff_core::{
         Report, ReportCategory, ReportItem, ReportItemMetadata, ReportUnit, ReportUnitMetadata,
     },
     config::path::platform_path,
-    diff, obj,
-    obj::{SectionKind, SymbolFlag},
+    diff,
+    obj::{self, SectionKind, SymbolFlag},
 };
 use prost::Message;
 use rayon::iter::{IntoParallelRefIterator, ParallelIterator};
@@ -177,14 +177,16 @@ fn report_object(
         .target_path
         .as_ref()
         .map(|p| {
-            obj::read::read(p.as_ref(), diff_config).with_context(|| format!("Failed to open {p}"))
+            obj::read::read(p.as_ref(), diff_config, diff::DiffSide::Target)
+                .with_context(|| format!("Failed to open {p}"))
         })
         .transpose()?;
     let base = object
         .base_path
         .as_ref()
         .map(|p| {
-            obj::read::read(p.as_ref(), diff_config).with_context(|| format!("Failed to open {p}"))
+            obj::read::read(p.as_ref(), diff_config, diff::DiffSide::Base)
+                .with_context(|| format!("Failed to open {p}"))
         })
         .transpose()?;
     let result =

objdiff-core/src/arch/mips.rs

@@ -12,7 +12,7 @@ use rabbitizer::{
 
 use crate::{
     arch::{Arch, RelocationOverride, RelocationOverrideTarget},
-    diff::{DiffObjConfig, MipsAbi, MipsInstrCategory, display::InstructionPart},
+    diff::{DiffObjConfig, DiffSide, MipsAbi, MipsInstrCategory, display::InstructionPart},
     obj::{
         InstructionArg, InstructionArgValue, InstructionRef, Relocation, RelocationFlags,
         ResolvedInstructionRef, ResolvedRelocation, Section, Symbol, SymbolFlag, SymbolFlagSet,
@@ -27,6 +27,7 @@ pub struct ArchMips {
     pub ri_gp_value: i32,
     pub paired_relocations: Vec<BTreeMap<u64, i64>>,
     pub ignored_symbols: BTreeSet<usize>,
+    pub diff_side: DiffSide,
 }
 
 const EF_MIPS_ABI: u32 = 0x0000F000;
@@ -38,7 +39,7 @@ const EF_MIPS_MACH_5900: u32 = 0x00920000;
 const R_MIPS15_S3: u32 = 119;
 
 impl ArchMips {
-    pub fn new(object: &object::File) -> Result<Self> {
+    pub fn new(object: &object::File, diff_side: DiffSide) -> Result<Self> {
         let mut abi = Abi::O32;
         let mut isa_extension = None;
         match object.flags() {
@@ -124,7 +125,11 @@ impl ArchMips {
             let Ok(name) = obj_symbol.name() else { continue };
             if let Some(prefix) = name.strip_suffix(".NON_MATCHING") {
                 ignored_symbols.insert(obj_symbol.index().0);
-                if let Some(target_symbol) = object.symbol_by_name(prefix) {
+                // Only remove the prefixless symbols if we are on the Base side of the diff,
+                // to allow diffing against target objects that contain `.NON_MATCHING` markers.
+                if diff_side == DiffSide::Base
+                    && let Some(target_symbol) = object.symbol_by_name(prefix)
+                {
                     ignored_symbols.insert(target_symbol.index().0);
                 }
             }
@@ -137,6 +142,7 @@ impl ArchMips {
             ri_gp_value,
             paired_relocations,
             ignored_symbols,
+            diff_side,
         })
     }
 

objdiff-core/src/arch/mod.rs

@@ -17,7 +17,7 @@ use object::Endian as _;
 
 use crate::{
     diff::{
-        DiffObjConfig,
+        DiffObjConfig, DiffSide,
         display::{ContextItem, HoverItem, InstructionPart},
     },
     obj::{
@@ -418,15 +418,18 @@ pub trait Arch: Any + Debug + Send + Sync {
     }
 }
 
-pub fn new_arch(object: &object::File) -> Result<Box<dyn Arch>> {
+pub fn new_arch(object: &object::File, diff_side: DiffSide) -> Result<Box<dyn Arch>> {
     use object::Object as _;
+    // Avoid unused warnings on non-mips builds
+    let _ = diff_side;
+
     Ok(match object.architecture() {
         #[cfg(feature = "ppc")]
         object::Architecture::PowerPc | object::Architecture::PowerPc64 => {
             Box::new(ppc::ArchPpc::new(object)?)
         }
         #[cfg(feature = "mips")]
-        object::Architecture::Mips => Box::new(mips::ArchMips::new(object)?),
+        object::Architecture::Mips => Box::new(mips::ArchMips::new(object, diff_side)?),
         #[cfg(feature = "x86")]
         object::Architecture::I386 | object::Architecture::X86_64 => {
             Box::new(x86::ArchX86::new(object)?)

objdiff-core/src/diff/mod.rs

@@ -807,3 +807,11 @@ fn find_section(
         s.kind == section_kind && s.name == name && !matches.iter().any(|m| m.right == Some(i))
     })
 }
+
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub enum DiffSide {
+    /// The target/expected side of the diff.
+    Target,
+    /// The base side of the diff.
+    Base,
+}

objdiff-core/src/jobs/objdiff.rs

@@ -6,7 +6,7 @@ use typed_path::Utf8PlatformPathBuf;
 
 use crate::{
     build::{BuildConfig, BuildStatus, run_make},
-    diff::{DiffObjConfig, MappingConfig, ObjectDiff, diff_objs},
+    diff::{DiffObjConfig, DiffSide, MappingConfig, ObjectDiff, diff_objs},
     jobs::{Job, JobContext, JobResult, JobState, start_job, update_status},
     obj::{Object, read},
 };
@@ -117,7 +117,7 @@ fn run_build(
                 &cancel,
             )?;
             step_idx += 1;
-            match read::read(target_path.as_ref(), &config.diff_obj_config) {
+            match read::read(target_path.as_ref(), &config.diff_obj_config, DiffSide::Target) {
                 Ok(obj) => Some(obj),
                 Err(e) => {
                     first_status = BuildStatus {
@@ -141,7 +141,7 @@ fn run_build(
         Some(base_path) if second_status.success => {
             update_status(context, format!("Loading base {base_path}"), step_idx, total, &cancel)?;
             step_idx += 1;
-            match read::read(base_path.as_ref(), &config.diff_obj_config) {
+            match read::read(base_path.as_ref(), &config.diff_obj_config, DiffSide::Base) {
                 Ok(obj) => Some(obj),
                 Err(e) => {
                     second_status = BuildStatus {

objdiff-core/src/obj/read.rs

@@ -12,7 +12,7 @@ use object::{Object as _, ObjectSection as _, ObjectSymbol as _};
 
 use crate::{
     arch::{Arch, RelocationOverride, RelocationOverrideTarget, new_arch},
-    diff::DiffObjConfig,
+    diff::{DiffObjConfig, DiffSide},
     obj::{
         FlowAnalysisResult, Object, Relocation, RelocationFlags, Section, SectionData, SectionFlag,
         SectionKind, Symbol, SymbolFlag, SymbolKind,
@@ -925,21 +925,25 @@ fn do_combine_sections(
 }
 
 #[cfg(feature = "std")]
-pub fn read(obj_path: &std::path::Path, config: &DiffObjConfig) -> Result<Object> {
+pub fn read(
+    obj_path: &std::path::Path,
+    config: &DiffObjConfig,
+    diff_side: DiffSide,
+) -> Result<Object> {
     let (data, timestamp) = {
         let file = std::fs::File::open(obj_path)?;
         let timestamp = filetime::FileTime::from_last_modification_time(&file.metadata()?);
         (unsafe { memmap2::Mmap::map(&file) }?, timestamp)
     };
-    let mut obj = parse(&data, config)?;
+    let mut obj = parse(&data, config, diff_side)?;
     obj.path = Some(obj_path.to_path_buf());
     obj.timestamp = Some(timestamp);
     Ok(obj)
 }
 
-pub fn parse(data: &[u8], config: &DiffObjConfig) -> Result<Object> {
+pub fn parse(data: &[u8], config: &DiffObjConfig, diff_side: DiffSide) -> Result<Object> {
     let obj_file = object::File::parse(data)?;
-    let mut arch = new_arch(&obj_file)?;
+    let mut arch = new_arch(&obj_file, diff_side)?;
     let split_meta = parse_split_meta(&obj_file)?;
     let (mut sections, section_indices) =
         map_sections(arch.as_ref(), &obj_file, split_meta.as_ref())?;

objdiff-core/tests/arch_arm.rs

@@ -6,7 +6,12 @@ mod common;
 #[cfg(feature = "arm")]
 fn read_arm() {
     let diff_config = diff::DiffObjConfig { ..Default::default() };
-    let obj = obj::read::parse(include_object!("data/arm/LinkStateItem.o"), &diff_config).unwrap();
+    let obj = obj::read::parse(
+        include_object!("data/arm/LinkStateItem.o"),
+        &diff_config,
+        diff::DiffSide::Base,
+    )
+    .unwrap();
     insta::assert_debug_snapshot!(obj);
     let symbol_idx =
         obj.symbols.iter().position(|s| s.name == "_ZN13LinkStateItem12OnStateLeaveEi").unwrap();
@@ -20,7 +25,9 @@ fn read_arm() {
 #[cfg(feature = "arm")]
 fn read_thumb() {
     let diff_config = diff::DiffObjConfig { ..Default::default() };
-    let obj = obj::read::parse(include_object!("data/arm/thumb.o"), &diff_config).unwrap();
+    let obj =
+        obj::read::parse(include_object!("data/arm/thumb.o"), &diff_config, diff::DiffSide::Base)
+            .unwrap();
     insta::assert_debug_snapshot!(obj);
     let symbol_idx = obj
         .symbols
@@ -37,7 +44,12 @@ fn read_thumb() {
 #[cfg(feature = "arm")]
 fn combine_text_sections() {
     let diff_config = diff::DiffObjConfig { combine_text_sections: true, ..Default::default() };
-    let obj = obj::read::parse(include_object!("data/arm/enemy300.o"), &diff_config).unwrap();
+    let obj = obj::read::parse(
+        include_object!("data/arm/enemy300.o"),
+        &diff_config,
+        diff::DiffSide::Base,
+    )
+    .unwrap();
     let symbol_idx = obj.symbols.iter().position(|s| s.name == "Enemy300Draw").unwrap();
     let diff = diff::code::no_diff_code(&obj, symbol_idx, &diff_config).unwrap();
     insta::assert_debug_snapshot!(diff.instruction_rows);

objdiff-core/tests/arch_mips.rs

@@ -6,7 +6,9 @@ mod common;
 #[cfg(feature = "mips")]
 fn read_mips() {
     let diff_config = diff::DiffObjConfig { mips_register_prefix: true, ..Default::default() };
-    let obj = obj::read::parse(include_object!("data/mips/main.c.o"), &diff_config).unwrap();
+    let obj =
+        obj::read::parse(include_object!("data/mips/main.c.o"), &diff_config, diff::DiffSide::Base)
+            .unwrap();
     insta::assert_debug_snapshot!(obj);
     let symbol_idx = obj.symbols.iter().position(|s| s.name == "ControlEntry").unwrap();
     let diff = diff::code::no_diff_code(&obj, symbol_idx, &diff_config).unwrap();
@@ -19,9 +21,19 @@ fn read_mips() {
 #[cfg(feature = "mips")]
 fn cross_endian_diff() {
     let diff_config = diff::DiffObjConfig::default();
-    let obj_be = obj::read::parse(include_object!("data/mips/code_be.o"), &diff_config).unwrap();
+    let obj_be = obj::read::parse(
+        include_object!("data/mips/code_be.o"),
+        &diff_config,
+        diff::DiffSide::Base,
+    )
+    .unwrap();
     assert_eq!(obj_be.endianness, object::Endianness::Big);
-    let obj_le = obj::read::parse(include_object!("data/mips/code_le.o"), &diff_config).unwrap();
+    let obj_le = obj::read::parse(
+        include_object!("data/mips/code_le.o"),
+        &diff_config,
+        diff::DiffSide::Base,
+    )
+    .unwrap();
     assert_eq!(obj_le.endianness, object::Endianness::Little);
     let left_symbol_idx = obj_be.symbols.iter().position(|s| s.name == "func_00000000").unwrap();
     let right_symbol_idx =
@@ -42,6 +54,11 @@ fn cross_endian_diff() {
 #[cfg(feature = "mips")]
 fn filter_non_matching() {
     let diff_config = diff::DiffObjConfig::default();
-    let obj = obj::read::parse(include_object!("data/mips/vw_main.c.o"), &diff_config).unwrap();
+    let obj = obj::read::parse(
+        include_object!("data/mips/vw_main.c.o"),
+        &diff_config,
+        diff::DiffSide::Base,
+    )
+    .unwrap();
     insta::assert_debug_snapshot!(obj.symbols);
 }

objdiff-core/tests/arch_ppc.rs

@@ -10,7 +10,9 @@ mod common;
 #[cfg(feature = "ppc")]
 fn read_ppc() {
     let diff_config = diff::DiffObjConfig::default();
-    let obj = obj::read::parse(include_object!("data/ppc/IObj.o"), &diff_config).unwrap();
+    let obj =
+        obj::read::parse(include_object!("data/ppc/IObj.o"), &diff_config, diff::DiffSide::Base)
+            .unwrap();
     insta::assert_debug_snapshot!(obj);
     let symbol_idx =
         obj.symbols.iter().position(|s| s.name == "Type2Text__10SObjectTagFUi").unwrap();
@@ -24,7 +26,12 @@ fn read_ppc() {
 #[cfg(feature = "ppc")]
 fn read_dwarf1_line_info() {
     let diff_config = diff::DiffObjConfig::default();
-    let obj = obj::read::parse(include_object!("data/ppc/m_Do_hostIO.o"), &diff_config).unwrap();
+    let obj = obj::read::parse(
+        include_object!("data/ppc/m_Do_hostIO.o"),
+        &diff_config,
+        diff::DiffSide::Base,
+    )
+    .unwrap();
     let line_infos = obj
         .sections
         .iter()
@@ -38,7 +45,12 @@ fn read_dwarf1_line_info() {
 #[cfg(feature = "ppc")]
 fn read_extab() {
     let diff_config = diff::DiffObjConfig::default();
-    let obj = obj::read::parse(include_object!("data/ppc/NMWException.o"), &diff_config).unwrap();
+    let obj = obj::read::parse(
+        include_object!("data/ppc/NMWException.o"),
+        &diff_config,
+        diff::DiffSide::Base,
+    )
+    .unwrap();
     insta::assert_debug_snapshot!(obj);
 }
 
@@ -47,12 +59,18 @@ fn read_extab() {
 fn diff_ppc() {
     let diff_config = diff::DiffObjConfig::default();
     let mapping_config = diff::MappingConfig::default();
-    let target_obj =
-        obj::read::parse(include_object!("data/ppc/CDamageVulnerability_target.o"), &diff_config)
-            .unwrap();
-    let base_obj =
-        obj::read::parse(include_object!("data/ppc/CDamageVulnerability_base.o"), &diff_config)
-            .unwrap();
+    let target_obj = obj::read::parse(
+        include_object!("data/ppc/CDamageVulnerability_target.o"),
+        &diff_config,
+        diff::DiffSide::Target,
+    )
+    .unwrap();
+    let base_obj = obj::read::parse(
+        include_object!("data/ppc/CDamageVulnerability_base.o"),
+        &diff_config,
+        diff::DiffSide::Base,
+    )
+    .unwrap();
     let diff =
         diff::diff_objs(Some(&target_obj), Some(&base_obj), None, &diff_config, &mapping_config)
             .unwrap();
@@ -90,7 +108,12 @@ fn diff_ppc() {
 #[cfg(feature = "ppc")]
 fn read_vmx128_coff() {
     let diff_config = diff::DiffObjConfig { combine_data_sections: true, ..Default::default() };
-    let obj = obj::read::parse(include_object!("data/ppc/vmx128.obj"), &diff_config).unwrap();
+    let obj = obj::read::parse(
+        include_object!("data/ppc/vmx128.obj"),
+        &diff_config,
+        diff::DiffSide::Base,
+    )
+    .unwrap();
     insta::assert_debug_snapshot!(obj);
     let symbol_idx =
         obj.symbols.iter().position(|s| s.name == "?FloatingPointExample@@YAXXZ").unwrap();