das-labor / panopticon

A libre cross-platform disassembler.
https://panopticon.re
GNU General Public License v3.0
1.43k stars 78 forks source link

completely remove graph_algos #336

Closed m4b closed 6 years ago

m4b commented 6 years ago

@flanfly Only thing that remains of graph_algos is in abstract_interp

Seems you're working on data-flow esque stuff, so perhaps I'll leave that to you, especially if you add a new impl for neo (it's also apparently only used in QT currently)?

If that's alright, I won't mess with abstract_interp so we don't step on each others toes; if you're working on something else, let me know.

Also I'll rebase this branch off of bincode as though its master, in case this keeps going, so no need to delete (until it gets merged into master, that is) ;)

Anyway:

  1. Totally removes graph_algos from core!
  2. Also removes longstanding pointless imports field from project
  3. re-adds to_dot impls using petgraphs version

Also, hilariously, adding this patch to disassembler (which I assumed was safe and would be a quick allocation-less gain in disassembler) causes the benchmarks to spiral out of control and use all memory on my system :sob:

diff --git a/amd64/src/architecture.rs b/amd64/src/architecture.rs
index 2d056e6..19e7e65 100644
--- a/amd64/src/architecture.rs
+++ b/amd64/src/architecture.rs
@@ -55,14 +55,16 @@ impl Architecture for Amd64 {
     }

     fn decode(reg: &Region, start: u64, cfg: &Self::Configuration) -> Result<Match<Self>> {
+        const MAX: usize = 16;
         let data = reg.iter();
-        let mut buf: Vec<u8> = vec![];
+        let mut buf: [u8; MAX] = [0; MAX];
         let mut i = data.seek(start);
         let p = start;
-
+        let mut len = 0;
         while let Some(Some(b)) = i.next() {
-            buf.push(b);
-            if buf.len() == 15 {
+            buf[len] = b;
+            len += 1;
+            if len == MAX {
                 break;
             }
         }

Anyway, I think I want to work on the disassembler because:

  1. I think there could be some easy perf gains (ostensibly like above, I suspect that's some weird compiler/benchmark bug?)
  2. I don't really understand
m4b commented 6 years ago

Also I don't think theres a huge rush to merge this if you're working on other stuff, just wanted you to know its there; shouldn't be nearly as conflict filled as last one

flanfly commented 6 years ago

I'm working on speeding up the data flow and abstract interpretation stuff. Nothing really merge worthy, yet. I just try to get a feel for the requirements and how to structure the data to improve performance.

Also, hilariously, adding this patch to disassembler (which I assumed was safe and would be a quick allocation-less gain in disassembler) causes the benchmarks to spiral out of control and use all memory on my system 😭

Odd. No idea, sorry.

Anyway, I think I want to work on the disassembler because:

Go for it :)

flanfly commented 6 years ago

Anyway, I think I want to work on the disassembler because:

Ah, In case you mean core/disassembler.rs: I honestly want to get rid of that. It's slow, huge and (for me) not much help when writing disassemblers. You either have a complex architecture like x86 where it doesn't help or a simple one like AVR where it doesn't improve readability much. That said I will of course merge improvements to Disassembler.

m4b commented 6 years ago

Ok I was going to ask about that actually. I’ll keep it in mind as an unwelcome citizen if I start refactoring.

Are you ok with changes to the disassemble trait itself ? It seems pretty complex. I also wonder if we could add any lifetime based savings to it to reduce intermediate allocations, since it only exists inside of function constructor so the lifetime scope won’t affect anything.

Similarly, the Region api is pretty complex, and I’m wondering if there’s room for simplification there and also reduction of intermediate allocations.

flanfly commented 6 years ago

Sure, just keep in mind you will have to modify the AVR & MOS 6502 disassembler for every change you make. I'm not sure it's worth the trouble. The Region API it trash too 😢 , go ahead

m4b commented 6 years ago

Ya I’m aware. I would only make changes lightly :laughing: also would prefer to have tests functioning again before messing with the deepest internals a bunch

Speaking of tests and disassembler, I think it would simplify so many things if we had a fake, simple, public and canonical test architecture we use in every test. That way don’t have to define architecture and disassembler in every test that involves it.

Just like: disassemble<Faux>

flanfly commented 6 years ago

I wrote one for the neo tests. I'll merge this weekend.