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

Another attempt to do EsilHooks ##esil #21939

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

condret
Copy link
Member

@condret condret commented Jun 19, 2023

Maybe this time

Still a bit WIP

  • Mark this if you consider it ready to merge
  • I've added tests (optional)
  • I wrote some lines in the book (optional)

Description

@condret condret force-pushed the esil_hooks2 branch 6 times, most recently from 6aae0a8 to 87c6114 Compare June 20, 2023 11:59
Copy link
Collaborator

@trufae trufae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the way it works, but i think this introduces so many methods when we can make it much simpler by using a single function that takes operation as argument.

It's also very similar to how REvent works. maybe better to re-evaluate to improve this api that we dont use yet because we can have multiple event handlers around and that will be important for ranal too.

What do you think?

@@ -4011,10 +4018,16 @@ R_API bool r_esil_setup(REsil *esil, RAnal *anal, int romem, int stats, int nonu
esil->cb.reg_write = internal_esil_reg_write_no_null;
esil->cb.mem_read = internal_esil_mem_read_no_null;
esil->cb.mem_write = internal_esil_mem_write_no_null;
r_esil_set_reg_write_imp (esil, (REsilImpHookRegWriteCB)internal_esil_reg_write_no_null, esil);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally the cast shuoldnt be needed, otherwise we can pass functions with different signature and get runtime crashes hard to debug

R_FREE (esil->hooks->mem_read_implementation);
}

R_API bool r_esil_set_mem_write_imp(REsil *esil, REsilImpHookMemWriteCB imp, void *user) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about having only a set method and tell which event as argument, index those type of events in an fixed size array like esil->hooks[R_ESIL_EVENT_RR]..

R_API bool r_esil_set_mem_write_imp(REsil *esil, REsilImpHookMemWriteCB imp, void *user) {
r_return_val_if_fail (esil && esil->hooks, false);
if (!esil->hooks->mem_write_implementation) {
esil->hooks->mem_write_implementation = R_NEW (REsilHook);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uhm, only 1 handler per event? we cant have multiple listeners? that seems like not what we want right?

return true;
}

R_API void r_esil_del_reg_write_imp(REsil *esil) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would prefer to have a smaller API and less functions, we want to do the ADD, DEL, CLEAR operations on RR,RW,MM,MW array of callbacks. this array of pointers can be an RVec, so they are executed in the right order and hold a void*user pointer. im thinking about something like this:

esil.hook(ADD, MR, cb, user);

ADD = enum for the supported operations: ADD, DEL, CLEAR, ..
MR = memory read, just another enum with other stuff
cb = the user provided callback for the given esil operation hook
user = user data pointer associated with the given callback

The cb is used as a uniqueid for each event, this is, you can esil.hook(DEL, cb, NULL);

At the end this is very similar to what REvent, what are the limitations you can think about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants