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

[WIP] Colorize function arguments and variables part 2 #10777

Merged
merged 7 commits into from Jul 20, 2018

Conversation

@cyanpencil
Copy link
Contributor

@cyanpencil cyanpencil commented Jul 18, 2018

NOTE: I put it as [WIP] as this needs reviewing, there are some part which are a bit hackish

Changes in this pr:

  1. Comments which indicate the type in disasm are colorized. However to do this, I was forced to add another R_META_TYPE, different from the normal comment, or ds_show_comments_right wouldn't be able to understand it was a special kind of comment that should be colorized

  2. Renamed new colors from func_arg* to func_var*, which makes more sense.

  3. Colorized local vars and args in disasm. This is the hackish bit. To achieve this, I had to pass the list of local vars to r_print_colorize_opcode(). I added a RList field to RPrint which gets populated with the vars. Then, inside r_print_colorize_opcode(), every argument is confronted with var names through strcmp. This is what needs reviewing the most: I fear this is slow, and not in the spirit of r2, passing variables around like this. But I really couldn't find a better way. Any help is appreciated.

  4. Manually modified each theme so that the new colors would fit with the style of each theme. This took some time, but it was worth it, r2 should be as beautiful as possible <3

Example screen of theme sepia:

2018-07-18-213705_925x589_scrot

This works even with custom renamed vars, as you can see from the screen custom_renamed_var.
Please if you have a bit of time cycle all colorschemes with R key to see if some colors mismatch (remember to use scr.color=3 though).

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 18, 2018

If you have any idea on how to improve the code, please don't hesitate to tell it, I'm very unsure of some parts especially inside r_print_colorize_opcode

@cyanpencil cyanpencil requested review from XVilka and radare Jul 18, 2018
@cyanpencil cyanpencil force-pushed the cyanpencil:colorize-arguments-2 branch 2 times, most recently from ca97c64 to c33d6f4 Jul 18, 2018
@@ -1629,6 +1631,26 @@ static bool issymbol(char c) {
}
}

static bool check_arg_name (char *p, RList *vars) {

This comment has been minimized.

@sivaramaaa

sivaramaaa Jul 18, 2018
Contributor

Can u explain me why do we need this function? (i couldn't understand that's why asking)

This comment has been minimized.

@cyanpencil

cyanpencil Jul 18, 2018
Author Contributor

So this function gets called from the r_colorize_opcode() loop when an arg of disasmed instruction is encountered. It returns true if it is equal to the name of one of the local vars.
I've added it to not clutter too much the r_colorize_opcode loop, but it is not necessary at all

r_list_join (list, reg_vars);
r_list_join (list, bpv_vars);
r_list_join (list, spv_vars);
r_list_free (reg_vars);

This comment has been minimized.

@sivaramaaa

sivaramaaa Jul 18, 2018
Contributor

good catch 👍

p[z] = '\0';
RAnalVar *var;
RListIter *iter;
r_list_foreach (vars, iter, var) {

This comment has been minimized.

@sivaramaaa

sivaramaaa Jul 18, 2018
Contributor

I think instead of iterating through list of vars , us this function r_anal_var_get_byname , it's faster since it uses sdb query to retrieve the var , also by doing this way we can avoid this hack and drop RList *vars from RPrint

This comment has been minimized.

@cyanpencil

cyanpencil Jul 18, 2018
Author Contributor

Hey this is a great idea, it solves a lot of problems!
Now I'm going to sleep, tomorrow I'll modify it for sure.
Thanks :)

@cyanpencil cyanpencil force-pushed the cyanpencil:colorize-arguments-2 branch from b6a6a46 to 54f91ed Jul 19, 2018
RList *reg_vars = r_anal_var_list (anal, fcn, R_ANAL_VAR_KIND_REG);
RList *bpv_vars = r_anal_var_list (anal, fcn, R_ANAL_VAR_KIND_BPV);
RList *spv_vars = r_anal_var_list (anal, fcn, R_ANAL_VAR_KIND_SPV);
r_list_join (list, reg_vars);

This comment has been minimized.

@radare

radare Jul 20, 2018
Collaborator

you can remove one of those lists if you just do RList *list = reg_vars; in the first line

This comment has been minimized.

@cyanpencil

cyanpencil Jul 20, 2018
Author Contributor

RList *list is not an empty pointer, it is another list declared some lines before, that needs to be merged to reg_vars, bpv_vars, spv_vars...

Copy link
Collaborator

@radare radare left a comment

lgtm

@radare
Copy link
Collaborator

@radare radare commented Jul 20, 2018

please fix the comment and rebase. should be green

@cyanpencil cyanpencil force-pushed the cyanpencil:colorize-arguments-2 branch from 71833cc to 6e04a55 Jul 20, 2018
@radare
Copy link
Collaborator

@radare radare commented Jul 20, 2018

ok! then ok to merge. but i would like to squash it, it is fine?

@radare radare merged commit 13a2cb3 into radareorg:master Jul 20, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ret2libc added a commit to ret2libc/radare2 that referenced this pull request Jul 23, 2018
…eorg#10777)"

This PARTIALLY reverts commit 13a2cb3.
All the updates to cons/d/* files were not reverted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants