beehive-lab / mambo

A low-overhead dynamic binary instrumentation and modification tool for ARM (both AArch32 and AArch64 support) and RISC-V (RV64GC).
Apache License 2.0
318 stars 69 forks source link

Various API helper functions ported to RISC-V, along with callback support and branch printing plugins #49

Closed jkressel closed 7 months ago

jkressel commented 2 years ago

I have included the increased size of BRANCH_FSPACE as well as the plugins, since this allows the plugins to run correctly

GuillermoCallaghan commented 2 years ago

Hi, as we discussed through email with you and @lgeek, lets start with these changes:

Also the name of the pull request does not reflect what it is actually doing.

  1. Increased size of BRANCH_FSPACE to deal with a case where a BNE needed 104 bytes > 96 bytes. Atomic increment u64 for riscv
    • This commit has to be separated into two. One with the increase of BRANCH_FSPACE and the other with Atomic increment.
  2. The two "update header" commits need to be squashed together, but the second "update headers" also includes handling jal_riscv, which needs to be in a separate commit. Also includes some code that was uncommented in plugins/branch_count.c which I think it should not have been commented out in the first place.
  3. Fixed unconditional branch print plugin this commit should be squashed with the one that introduced the plugin.
  4. This one seems ok Unconditional branch linking on risc v
  5. Something is still broken, the plugins mostly work except that when r…
    • Ok, if something is not working, don't push it. Also, this includes a lot of things, they need to be in their own commit. For example, once cnd_branch_print.c is done and working, its commit should be something like "Plugin to print conditional branches"
  6. These commits should not be there. There should be just one that implements the API.
  7. Elf loader now deals with some unhandled cases for riscv These cases are not riscv only, and may need to be handle correctly.
jkressel commented 2 years ago

@GuillermoCallaghan @lgeek I have fixed the commit situation, so that each feature is a commit

lgeek commented 2 years ago

Hi @jkressel. Thanks again for the patches. I think this pull request will be getting very unwieldy if we continue using it for code review, there are many commits, quite a bit of code, and IIRC any time you force push an update, the entire code review will be collapsed so it will be difficult to keep track of what's been done. This is why I was asking for several pull requests more narrow in scope in my initial email. We need to split it up into at least 3 separate pull requests:

Basically create two new branches based on the riscv branch in this repository and cherry-pick the commits I've mentioned. Then open a new pull request for each branch.

My recommendation would be to leave the plugins aside until we get everything else merged and focus on the other patches. Since @GuillermoCallaghan started to review the plugins in this pull request, we can leave it open for them.

jkressel commented 2 years ago

@lgeek @GuillermoCallaghan okay I'll create some new pull requests for those commits