Skip to content
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

Add register usage analysis and instruction combine pass #140

Merged
merged 11 commits into from May 17, 2018

Conversation

@HMPerson1
Copy link
Collaborator

@HMPerson1 HMPerson1 commented May 11, 2018

TODO:

  • Export the register usage information info into RadecoFunction
  • An pass that turns b=a+2; c=b-2 => c=b so that the "preserves" analysis is more accurate
    • b=a+b; c=b-2 => c=b+0
    • c=b+0 => c=b
    • b=a+0xfffffffffffffffc => b=a-4
@HMPerson1 HMPerson1 force-pushed the HMPerson1:fn_param_anal branch from 4cd17b9 to 0bc33fe May 11, 2018
type Reg = String;

#[derive(Debug, Clone)]
struct CallingConvention {

This comment has been minimized.

@chinmaydd

chinmaydd May 11, 2018
Contributor

This naming is going to cause problems after arch-rs integration.


/// Calls `patch_fn`, `dce::collect`, and `analyze_fn` on every function,
/// callees first
fn go(&mut self, rmod: &mut RadecoModule) -> () {

This comment has been minimized.

@chinmaydd

chinmaydd May 11, 2018
Contributor

analyze() better than go() maybe?

This comment has been minimized.

@HMPerson1

HMPerson1 May 11, 2018
Author Collaborator

It does more than just analysis though; it also patches call sites.

This comment has been minimized.

@chinmaydd

chinmaydd May 11, 2018
Contributor

Alright, I just thought it would be better if we follow some convention. A lot of analyses expose run and passes such as dce provide a more verbose method name like collect(). Not a problem!

/// Using the callconv info we've gathered so far, patch-up call sites to
/// to remove arguments that the callee doesn't read and make values in
/// callee-saved registers be preserved across the call.
fn patch_fn(&self, ssa: &mut SSAStorage) -> () {

This comment has been minimized.

@chinmaydd

chinmaydd May 11, 2018
Contributor

Better if we return a Result?

This comment has been minimized.

@HMPerson1

HMPerson1 May 11, 2018
Author Collaborator

Return Ok(()) if every call site was patched and return Err(<something>) if any call site could not be patched? But then every function containing an indirect call would return Err, because we can't patch those because we don't know anything about what the callee is.

This comment has been minimized.

@HMPerson1

HMPerson1 May 11, 2018
Author Collaborator

Also, we can't tell the difference between an indirect call and a malformed OpCall SSA node; both just have comments as the call target.

This comment has been minimized.

@chinmaydd

chinmaydd May 11, 2018
Contributor

Hmm. I was wondering if information about which call sites were successfully patched would be of use to the analyzer later. I think this is fine for now.

@HMPerson1 HMPerson1 changed the title [WIP] Add calling convention analysis [WIP] Add register usage analysis May 11, 2018
@HMPerson1 HMPerson1 force-pushed the HMPerson1:fn_param_anal branch 3 times, most recently from 632ba68 to 2946587 May 11, 2018
@chinmaydd
Copy link
Contributor

@chinmaydd chinmaydd commented May 15, 2018

LGTM! 👍

@HMPerson1 HMPerson1 changed the title [WIP] Add register usage analysis Add register usage analysis and instruction combine pass May 15, 2018
@HMPerson1
Copy link
Collaborator Author

@HMPerson1 HMPerson1 commented May 15, 2018

This is ready for merging now @chinmaydd @sushant94

HMPerson1 added 11 commits May 10, 2018
@HMPerson1 HMPerson1 force-pushed the HMPerson1:fn_param_anal branch from 6ac97dc to 26c3431 May 17, 2018
/// This analysis is super conservative; for example, if a function preserves a
/// register by pushing it onto the stack and popping it back right before
/// returning, it is considered to be read and not preserved because we can't
/// guarantee that that stack location is never subsequently read or modified.

This comment has been minimized.

@sushant94

sushant94 May 17, 2018
Collaborator

Open an issue for this so that we can track progress and eventually have a more precise analysis, either in this pass or as a separate one.

This comment has been minimized.

@sushant94

sushant94 May 17, 2018
Collaborator

Also, add the issue number in the comment to track it.

@sushant94 sushant94 merged commit b5c8043 into radareorg:master May 17, 2018
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@HMPerson1 HMPerson1 deleted the HMPerson1:fn_param_anal branch May 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.