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

Fix regression in master by adding Ct command #10791

Merged
merged 4 commits into from Jul 24, 2018

Conversation

@cyanpencil
Copy link
Contributor

@cyanpencil cyanpencil commented Jul 21, 2018

So this adds the Ct command, that behaves in the exact same way of the CC command, only that inserts special type analysis comments (that are now colored differently).

This way, now users can add (or delete) type analysis comments themselves.

This is done to fix the regression in master, together with pr: https://github.com/radare/radare2-regressions/pull/1404

(Also included a small fix in r_print.h that generated a lot of compiler warning, special thanks to @sivaramaaa that reported it!)

@kazarmy
Copy link
Contributor

@kazarmy kazarmy commented Jul 21, 2018

I think it's better for type analysis comments to be a subtype of R_META_TYPE_COMMENT (I'm all for different colors for different kinds of comments). This does mean r_meta API changes/additions though to increase support for subtypes...

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 22, 2018

This issue is related: #5716

@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 23, 2018

@sivaramaaa what is your opinion on this one?

@XVilka XVilka requested a review from sivaramaaa Jul 23, 2018
Copy link
Contributor

@ret2libc ret2libc left a comment

Why didn't you add Ct as done for Cs, Cr, Cd, etc.? They are handled in cmd_meta_hsdmf and not in cmd_meta_comment. What's the difference?

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 23, 2018

@ret2libc I tried to do that first, but I found out that using the same cmd_meta_comment () was giving much more functionality without any drastical changes to r_meta API.

In particular, it made possible to the user to add type analysis comments himself, with Ct comment , and, more importantly, it let the user delete unwanted type analysis comments, with Ct-, with just a few changes to the code (basically let cmd_meta_comment select which comment type to operate on with cmt_type).

But imho I think the most elegant solution is what @kazarmy suggest, that is dealing with different comments through subtypes: we could design a general system for handling many types of different comments at once, thus solving #5716 too at the same time. (but I also think that this should be done in a later pr, not in this one)

However as I said on tg I'm pretty unfamiliar with the r_meta part of the code, and so if you think that it should go into cmd_meta_hsdmf () for consistency reasons, I'll change it.

@ret2libc
Copy link
Contributor

@ret2libc ret2libc commented Jul 23, 2018

I see. I'm not very familiar with it neither, but from a quick look it seems it's already possible for users to do Cd-, Cd 4 @ XXX or Cm 4 mystring @ XXX. Though the code is a bit messy (as usual :D ) there's already support for subtypes in cmd_meta_hsdmf. Moreover, by using it you should already have everything you need.

So, my 2 cents is that we keep using cmd_meta_hsdmf just for consistency (even if that code doesn't look too good IMHO). @radare ?

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 23, 2018

Updated to follow @ret2libc 's tips.
Please, before merging, I would like to have confirmation by @kazarmy

@cyanpencil cyanpencil requested a review from kazarmy Jul 23, 2018
r_meta_del (a, R_META_TYPE_VARTYPE, addr, size);
}
if (type == R_META_TYPE_COMMENT || type == R_META_TYPE_VARTYPE) {
snprintf (key, sizeof (key)-1, "meta.%c.0x%"PFMT64x, type, addr);

This comment has been minimized.

@ret2libc

ret2libc Jul 23, 2018
Contributor

is this needed only for VARTYPE or also for other types as well?

This comment has been minimized.

@cyanpencil

cyanpencil Jul 23, 2018
Author Contributor

This is needed only for metas set up through r_meta_set_string (which currently are only COMMENT and VARTYPE). All other metas, which are set up through r_meta_add, do not need this.
This was already done for COMMENT, I only extended for VARTYPE too. I think it was originally done because r_meta_set_string is faster than r_meta_add, but I'm not 100% sure.

This comment has been minimized.

@ret2libc

ret2libc Jul 23, 2018
Contributor

Where do you use r_meta_set_string for VARTYPE?

This comment has been minimized.

@cyanpencil

cyanpencil Jul 23, 2018
Author Contributor

Inside libr/core/anal_tp.c, in function type_match()

This comment has been minimized.

@cyanpencil

cyanpencil Jul 23, 2018
Author Contributor

I could modify and use r_meta_add there too, but originally (before my last pr) it was already r_meta_set_string with COMMENT, I only switched it to VARTYPE

This comment has been minimized.

@ret2libc

ret2libc Jul 23, 2018
Contributor

It's ok. Just wanted to understand ;)

@cyanpencil cyanpencil force-pushed the cyanpencil:fix-vartype-comments branch from 58d42c5 to c3c0fbd Jul 23, 2018
@kazarmy
Copy link
Contributor

@kazarmy kazarmy commented Jul 24, 2018

Travis is red ... you have to rebase both this branch and its associated r2r branch ... rebase the r2r branch first

@@ -953,6 +954,7 @@ static int cmd_meta(void *data, const char *input) {
case 'd': /* "Cd" data */
case 'm': /* Cm magic */
case 'f': /* Cf formatted */
case 't': /* Ct type analysis commnets */
cmd_meta_hsdmf (core, input);

This comment has been minimized.

@kazarmy

kazarmy Jul 24, 2018
Contributor

might as well rename the function

This comment has been minimized.

@ret2libc

ret2libc Jul 24, 2018
Contributor

yes but please rename it to something which is not based on the commands otherwise next time we add a new C type, you need to rename the function again. Something like cmd_meta_types, cmd_meta_others or something else.

@kazarmy
Copy link
Contributor

@kazarmy kazarmy commented Jul 24, 2018

Please, before merging, I would like to have confirmation by @kazarmy

If @radare says yes on this, I'm not going to argue.

@cyanpencil cyanpencil force-pushed the cyanpencil:fix-vartype-comments branch from c3c0fbd to 90886fc Jul 24, 2018
@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented Jul 24, 2018

Ok, renamed the function and rebased, should be green now. Let's wait what @radare says.

@XVilka
Copy link
Contributor

@XVilka XVilka commented Jul 24, 2018

@radare is in vacation, lets not take his precious time. I think it looks good already. If there will be a strong disagreement with this - feel free to send new PR.

@XVilka XVilka merged commit f260195 into radareorg:master Jul 24, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants