mambo

mambo copied to clipboard

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

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.
   • WIP! atomic incrament and RETURN branch decoding still need fixing
     • increment has a typo in that commit message. Also I think there is nothing related to atomic increment in this commit.
     • More API helpers and internals wired up for riscv
     • WIP! started wiring up api helper functions on riscv
     • Started porting api helper functions over to riscv
     • Branch decoder support will now check for return branches on riscv
     • Add beginning of RISCV Branch Decoder - Return instruction decoding i…
7. Elf loader now deals with some unhandled cases for riscv These cases are not riscv only, and may need to be handle correctly.

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

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:

• API patches for RISCV: a22387d, 6d7c295, 63a7557, efd3e2f
• Scanner fixes for RISCV: b8e2e9f, ba4fa47
• Plugins: 3fefa17, 1233bf2

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.

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