bytecodealliance / wasm-tools

CLI and Rust libraries for low-level manipulation of WebAssembly modules
Apache License 2.0
1.32k stars 241 forks source link

How to compare Operator and Type, these type not implement eq #780

Open hunjixin opened 2 years ago

hunjixin commented 2 years ago

i want to assert Operator and Type but failed

    assert_eq!(
            get_function_body(&edit_raw_wasm, 0).as_slice(),
            &vec![I32Const{value:2}, Call{function_index:1}, GlobalGet{global_index:1}, Call{function_index:2}, End][..]
        );
    pub fn resolve_type_idx(&self, t: &Type) ->Option<u32> {
        for (index, ty) in self.types_map.iter().enumerate() {
            if matches!(t, ty) {
                return Some(index as u32);
            }
        }
        return None;
    }

type defined like this

#[derive(Debug, Clone)]
pub enum Type {
    /// The type is for a function.
    Func(FuncType),
}

#[derive(Clone)]
pub struct BrTable<'a> {
    pub(crate) reader: crate::BinaryReader<'a>,
    pub(crate) cnt: u32,
    pub(crate) default: u32,
}

#[derive(Debug, Clone)]
pub enum Type {
    /// The type is for a function.
    Func(FuncType),
}

#[derive(Clone)]
pub struct BrTable<'a> {
    pub(crate) reader: crate::BinaryReader<'a>,
    pub(crate) cnt: u32,
    pub(crate) default: u32,
}

macro_rules! define_operator {
    ($(@$proposal:ident $op:ident $({ $($payload:tt)* })? => $visit:ident)*) => {
        /// Instructions as defined [here].
        ///
        /// [here]: https://webassembly.github.io/spec/core/binary/instructions.html
        #[derive(Debug, Clone)]
        #[allow(missing_docs)]
        pub enum Operator<'a> {
            $(
                $op $({ $($payload)* })?,
            )*
        }
    }
}

macro_rules! define_operator {
    ($(@$proposal:ident $op:ident $({ $($payload:tt)* })? => $visit:ident)*) => {
        /// Instructions as defined [here].
        ///
        /// [here]: https://webassembly.github.io/spec/core/binary/instructions.html
        #[derive(Debug, Clone)]
        #[allow(missing_docs)]
        pub enum Operator<'a> {
            $(
                $op $({ $($payload)* })?,
            )*
        }
    }
}

could make this enum can be equal

alexcrichton commented 2 years ago

I'd personally be hesitant to add equality to these types since they're quite large and have a lot of meaning behind them which isn't obvious from the static structure. For example the type_index field of a CallIndirect could point to two different types which are actually the same and be "considered equal" but they're not statically equal in the derive(PartialEq) sense.

Could you elaborate a bit more on why you'd like to have PartialEq bounds on these types?

hunjixin commented 2 years ago

@alexcrichton use wasmparser and wasmencoding to edit wasm, but in test i need to check the result and expect like below, get_function_body use wasmparser to get a opertor slice to asset, but Operator cannot be equal, so have any suggestion except check them manually?

assert_eq!(
            get_function_body(&edit_raw_wasm, 0).as_slice(),
            &vec![I32Const{value:2}, Call{function_index:1}, GlobalGet{global_index:1}, Call{function_index:2}, End][..]
        );
Robbepop commented 2 years ago

As @alexcrichton already stated actual equality can only be computed given some context. The context in this case is the respective Wasm module resources. Even two CallIndirect instructions with the same type_index fields may actually point to different function types when used in different context. So actual equality cannot be computed in isolation. So I guess your best bet is to implement equality check manually with respect to the semantics you need for it. Given the for_each_operator macro this shouldn't be too much work either.

hunjixin commented 2 years ago

@Robbepop I'm not familiar with macros and tried but failed, can you help me see how to write this macro_rule?

https://github.com/hunjixin/fvm-wasm-instrument/blob/refrator/fvm_instrucments/src/utils/operator_eq.rs

alexcrichton commented 2 years ago

I don't think it makes sense to try to make everything in wasmparser both Eq and PartialEq. In the long run this just doesn't seem like the right crate to have the layer of abstraction for equating wasm binaries. For example crates like walrus may be better for something like this.

Why are you checking equality in the first place? The flat Vec<Operator> representation is not an efficient representation of wasm instructions and if nothing is perf-critical I'd recommend going through the text format and testing equality on those strings.