falconre / falcon

Binary Analysis Framework in Rust
Apache License 2.0
551 stars 45 forks source link

Expression: Drop dedup from scalars and scalars_mut #56

Closed emmanuel099 closed 4 years ago

emmanuel099 commented 4 years ago

As the function documentation states, all scalars used in the expression will be returned. Therefore, the dedup is not correct as the caller expects all scalar instances. This is especially true when mutating the scalars, as otherwise some scalars which appear multiple times with same name will be missed.

E.g. x + y * x should give [x, y, x] instead of [x, y] -> misses second x instance with dedup!

If the caller only requires the names of the scalars appearing in the expression, then the caller could simply call dedup.

My use-case for mutating all scalars of an expression is the SSA transformation of IL expressions:

impl SSARename for il::Expression {
    fn rename_scalars(&mut self, renamer: &mut ScalarRenamer) -> Result<()> {
        for scalar in self.scalars_mut() {
            let ssa_name = renamer.get_ssa_name(scalar)?;
            scalar.set_name(ssa_name);
        }

        Ok(())
    }
}
endeav0r commented 4 years ago

Seems reasonable to me