falconre / falcon

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

Add arithmetic shift right expression #94

Closed emmanuel099 closed 3 years ago

emmanuel099 commented 3 years ago

This change is motivated by the fact, that SMT solvers have builtin support for arithmetic shift right which is more efficient than the combination of shl, sub and shr.

endeav0r commented 3 years ago

Oh wow, sorry, did not realize so much time had passed. Time flies sometimes I guess.

I purposely left ASR our originally because I like having as few operations as possible. However, you're correct this is a pretty basic operation.

I will throw out three ideas in my preference from what I like the most to least. Let me know what you think.

1) We put Expression::Ashr behind a feature flag. We change the sra so it maps either to its current form, or Expression::ashr in your PR, based on the feature flag.

2) We merge this, and I add a function to Expression called something along the lines of, simple_form. This would eliminate expression types which can be mapped to simpler expression types. For example, this would also map cmpneq(x, y) to xor(1, cmpeq(x, y)).

3) We merge it as is, and we embrace the greatness that is arithmetic right shifts.

emmanuel099 commented 3 years ago
  1. What if we let Expression::ashr itself decide if to return Expresion::Ashr or the combination of shl, sub and shr? This would avoid the code duplication in the MIPS and x86 translator and would IMHO make the code much easier to read.
  2. I'm not in favor of this approach. Would simple_form also eliminate redundant operations like Expression::Or or Expression::Xor (which can be rewritten as a combination of conjunctions and negations)? - I'm afraid that simple_form makes IL expressions much harder to read.
endeav0r commented 3 years ago
  1. What if we let Expression::ashr itself decide if to return Expresion::Ashr or the combination of shl, sub and shr? This would avoid the code duplication in the MIPS and x86 translator and would IMHO make the code much easier to read.

Can we do this, but with Expression::Ashr behind a feature flag until 0.6, and once 0.6 goes live Expression::Ashr becomes part of the IL? When the feature is enabled, Expression::Ashr exists in the IL and Expression::ashr returns Expression::Ashr. When the feature is not enabled, it returns the combinations of shl/sub/shr. This does come with the understanding that 0.6 probably won't happen for a while, so this would be feature-flagged for the foreseeable future.

This is the last time I'll ask. If you still don't like this, we'll just go ahead, merge, and bump 0.6. This is not a huge deal, and I'm ok with this.

emmanuel099 commented 3 years ago

Sorry for the late reply, I'll update my PR tomorrow. I like your idea to hide it behind a feature flag until 0.6.