search

falconre / falcon

Binary Analysis Framework in Rust
Apache License 2.0
549 stars 47 forks source link

Aarch64 bad64 #108

Closed endeav0r closed 3 years ago

endeav0r commented 3 years ago

@kawadakk If it's Ok with you I would love to spend some time merging this into Falcon.

kawadakk commented 3 years ago

@endeav0r Of course! Let me know if there's something I can help you with.

https://github.com/falconre/falcon/blob/ba2ce82c370768dd22f6b7ab97cb9ffe865325d5/lib/translator/aarch64/semantics.rs#L7

https://github.com/falconre/falcon/blob/ba2ce82c370768dd22f6b7ab97cb9ffe865325d5/lib/translator/aarch64/semantics.rs#L104-L106

By the way, after spending a very hard time debugging an issue with Expression's methods returning ErrorKind::Sort, I took the liberty of escalating all unexpected errors to panics, following the common convention. I hope you don't mind.

endeav0r commented 3 years ago

We'll probably want to turn those back to errors. Right now we can do things like, "Lift as much of this program as possible, but if a function fails to lift... whatever, keep going." We can't do that with unwrap/panic.

I see now yrp's bad64 bindings require nighty, so first step is trying to bring those back to stable if I can.

kawadakk commented 3 years ago

"Lift as much of this program as possible, but if a function fails to lift... whatever, keep going."

Actually that's exactly what our internal application does by catch_unwind. Replacing explicit unwrap and unreachable! calls with return Err(_) doesn't completely eliminate panics that are caused by an internal logic error. In general, it just adds another case for downstream crates to treat as an internal error, which is quite unnecessary.

endeav0r commented 3 years ago

"Lift as much of this program as possible, but if a function fails to lift... whatever, keep going."

Actually that's exactly what our internal application does by catch_unwind. Replacing explicit unwrap and unreachable! calls with return Err(_) doesn't completely eliminate panics that are caused by an internal logic error. In general, it just adds another case for downstream crates to treat as an internal error, which is quite unnecessary.

I am reading, and learning. My take on the concept here is: There are errors that may be reasonably expected, and there are errors that will only exist if the code's internal logic is incorrect. If the code is incorrect, it should be fixed, whereas if something outside the code is causing an error it should be handled and dealt with. catch_unwind provides a mechanism for allowing people to unwrap on the former case, and then catch it. This means when your code is wrong you don't break everything, but you also can be a bit more... less explicit than when there's a reasonable error-case that may actually be expected.

I... like the idea. It's not wrong. I would like to think about it for a few days, not because I disagree with the premise, but because it's different from how the rest of the codebase exists.

endeav0r commented 3 years ago

I just threw https://github.com/yrp604/bad64/pull/5 into bad64, but bad64 won't rely on std, which means no impl std::error::Error.

Falcon has relied on error-chain, which was quick-and-easy a few years ago, but is kind of an abomination for a project this size. Without the impl of the Error trait in bad64, which I'm not going to ask yrp to do, we're going to need good and proper errors.

This will hurt, but thanks @kawadakk. I believe you're going to force me to do good things for the project I have neglected.

endeav0r commented 3 years ago

This is a priority for me, so I wanted to provide an update.

We're going to ignore #109, and apply @kawadakk 's fix for that issue, and work to upstream it so bad64 emits proper rust errors.

I have been working this PR across falcon-z3, finch, raptor, and falconre. I'm going to update all crates at the same time. I expect this PR to come through by Sunday.

As for catch_unwind, ok, we'll do it and leave it as is. If it causes problems, we'll deal with it when it causes problems.

endeav0r commented 3 years ago

Waiting/hoping yrp is happy with https://github.com/yrp604/bad64/pull/6 .

I have a good, working falcon locally (against those bad64 changes), as well as a falcon-z3, finch, and raptor. We should expect a cascade of changes hopefully by the end of this weekend.

endeav0r commented 3 years ago

This PR is now https://github.com/falconre/falcon/pull/111 , which is the same branch but in falconre/falcon