New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Control Flow Structuring #160

Merged
merged 10 commits into from Jun 10, 2018

Conversation

Projects
None yet
3 participants
@HMPerson1
Collaborator

HMPerson1 commented May 24, 2018

The output AST should be correct, but it's probably completely unreadable. Refinements to make the AST better will be added in the next few weeks.

  • Data structures
  • Compute reaching conditions
    • Graph slice
  • Structure acyclic regions
  • Region-ify loops
    • Funnel abnormal entries
    • Funnel abnormal exits
      • Nearest Common Dominator
  • Structure loops
  • Cleanup

HMPerson1 added some commits May 24, 2018

@HMPerson1 HMPerson1 changed the title from [WIP] Control Flow Structuring to Control Flow Structuring Jun 5, 2018

@HMPerson1

This comment has been minimized.

Collaborator

HMPerson1 commented Jun 5, 2018

@@ -0,0 +1,171 @@
use std::fmt;

This comment has been minimized.

@sushant94

sushant94 Jun 6, 2018

Collaborator

Please add a high level description of this module in doc comments so that it shows up in rustdocs.

Things to include:

  • What does this module provide, where does it fit into the control flow restructuring work.
  • High-level API design and how this module is designed to be used.
use std::ptr;
use typed_arena::Arena;
pub struct BaseCondition<'cd, T: 'cd>(&'cd BaseConditionS<'cd, T>);

This comment has been minimized.

@sushant94

sushant94 Jun 6, 2018

Collaborator

Not going to repeat this for all the structs introduced, add doc comments explaining what the struct is for, what API does it expose to its users.

Additionally, for public functions to be called by the structs users include what pre-conditions must hold on its arguments and what the expected result is.

As normal comments include: if there are some tricky implementation details, thing that aren't obvious and likely to break if someone else makes some changes to it.

This comment has been minimized.

@HMPerson1

HMPerson1 Jun 6, 2018

Collaborator

I think that this module is going to need to change a lot in order to implement refinements (i.e. if (a) doA(); if (!a) doB(); ==> if (a) doA(); else doB();) so I'm going to hold off on documenting all the APIs of this module until those changes have happened.

@@ -0,0 +1,226 @@
//! Various graph algorithms

This comment has been minimized.

@sushant94

sushant94 Jun 6, 2018

Collaborator

This module seems to be a building block for CF restructuring. Please add some tests for this.

This comment has been minimized.

@HMPerson1

HMPerson1 Jun 6, 2018

Collaborator

graph_utils/tests.rs ?

// "initialize one stack for each u in U and mark u as re-entered"
let mut stacks: Vec<Stack<G>> = self.reentered.iter().map(NonEmptyPath::new).collect();
// <invariant: forall stack in stacks, reentered.contains(stack.start)>

This comment has been minimized.

@sushant94

sushant94 Jun 6, 2018

Collaborator

Add an assertion here to check for this

This comment has been minimized.

@sushant94

sushant94 Jun 6, 2018

Collaborator

Probably a debug_assert!

@@ -0,0 +1,181 @@
//! https://doi.org/10.1016/0196-6774(92)90063-I

This comment has been minimized.

@sushant94

sushant94 Jun 6, 2018

Collaborator

Explain what this module is as doc comments, context as to why this is important and a requirement for CF restructuring. May be outline the algorithm, like the very high-level details / intuition (cause it is pay-walled and not everyone can access that link).

This comment has been minimized.

@HMPerson1

HMPerson1 Jun 6, 2018

Collaborator

This isn't really an important requirement for CF structuring; it's used in exactly one place (funneling loop exits) and its return value could be substituted with an already-known value (the loop header) without changing the correctness of the structuring algorithm. Its only purpose is to make the condition cascade at the loop exit cleaner.

Also, I frankly have no idea how the algorithm works. I just translated the pseudo-code from the paper into Rust and it seems to work. 😅

@@ -0,0 +1,466 @@
//! Recovers high-level control-flow constructs from a control-flow graph.
//! Implements the algorithm described in
//! [*No More Gotos*](https://doi.org/10.14722/ndss.2015.23185)

This comment has been minimized.

@sushant94

sushant94 Jun 6, 2018

Collaborator

Include a comment for every submodule, what they provide and how it helps this algorithm. Essentially giving a high level picture of how the different parts of this module work together.

This comment has been minimized.

@HMPerson1

HMPerson1 Jun 6, 2018

Collaborator

Does that have to be inline here or can it just be in the documentation for each module?

}
}
}
println!("entries: {:?}", entry_map);

This comment has been minimized.

@sushant94

sushant94 Jun 6, 2018

Collaborator

Forgot this one :)

prev_out_cond = Some(self.cctx.mk_simple(SimpleCondition(format!(
"{} != {}",
struct_var, prev_entry_num
)))); // XXX

This comment has been minimized.

@sushant94

sushant94 Jun 6, 2018

Collaborator

What are these XXX for? If its something incomplete or a todo, please include a comment. Similar ones a few lines above.

This comment has been minimized.

@HMPerson1

HMPerson1 Jun 6, 2018

Collaborator

The XXX means it needs to be replaced with actual values from SimpleCAST when that gets merged.

@XVilka

This comment has been minimized.

Member

XVilka commented Jun 8, 2018

So, is this one ready to be merged?

@HMPerson1

This comment has been minimized.

Collaborator

HMPerson1 commented Jun 8, 2018

I've done everything I can with this that doesn't immediately depend on c_simple_ast, so I'm ok with merging this now.

ping @sushant94

@HMPerson1 HMPerson1 changed the title from Control Flow Structuring to Initial Control Flow Structuring Jun 8, 2018

@sushant94

This comment has been minimized.

Collaborator

sushant94 commented Jun 10, 2018

SGTM Merging

@sushant94 sushant94 merged commit 9fba405 into radareorg:master Jun 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@HMPerson1 HMPerson1 deleted the HMPerson1:ctrl_flow_struct branch Jun 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment