CallstackInstr: switch to SBE/EBE callbacks #1445
Conversation
AndrewFasano
force-pushed
the
callstack_instr_sbe
branch
from
f1b34c4
to
52e9126
Compare
The old logic would miss many calls if tb_chaining was enabled (which is is by default). Since this plugin did not disable tb_chaining, many calls would be missed if a user didn't disable chaining or load another plugin that disabled chaining. This commit updates the plugin to use start_block_exec and end_block_exec which work even with tb_chaining enabled.
AndrewFasano
force-pushed
the
callstack_instr_sbe
branch
from
52e9126
to
a8f238a
Compare
| @@ -611,9 +593,9 @@ bool init_plugin(void *self) { | |||
|
|
|||
| pcb.after_block_translate = after_block_translate; | |||
| panda_register_callback(self, PANDA_CB_AFTER_BLOCK_TRANSLATE, pcb); | |||
| pcb.after_block_exec = after_block_exec; | |||
| pcb.end_block_exec = end_block_exec; | |||
| panda_register_callback(self, PANDA_CB_AFTER_BLOCK_EXEC, pcb); | |||
There was a problem hiding this comment.
This is still AFTER_BLOCK_EXEC? Is that what you intended? Same with BEFORE_BLOCK_EXEC below. @LauraLMann spotted this.
There was a problem hiding this comment.
I don't think so - looks like before_block_exec -> start_block_exec and after_block_exec -> start_block_exec. They're conceptually similar, but the implementation is different (most relevant here is that SBE and EBE trigger for each block even with TB chaining enabled).
There was a problem hiding this comment.
I might be missing something super obvious though
There was a problem hiding this comment.
The second arg to panda_register_callback didn't change. pcb is just a big union - the field names in that structure can be interchanged - changing just the field name in pcb is essentially a no-op. Changing the field name implied that you wanted to change which callback was being invoked, but you are still registering the same callback.
There was a problem hiding this comment.
Ah yes, I am missing something super obvious! Thanks, I'l fix
| cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags); | ||
| // erase nicely does nothing if key DNE | ||
| stoppedInfo.erase(curStackid); |
There was a problem hiding this comment.
I think I have a case where the removal of this logic is causing a regression. Basically running the same recording can result in callstack_instr generating different output. I think that's what this logic was preventing from happening. Working on verifying this is indeed the case.
There was a problem hiding this comment.
If callstack_instr is firing more often after this change (including the correct callback types in #1447), I'd expect that - with the new callbacks it should be able to detect more calls.
If it's firing less, then that's an issue. I was thinking the end-block-exec callback would only trigger at the very end of a TB, and not if the block is jumped out of prematurely. But if the end-block-exec callbacks fires even if the block is interrupted, that would cause issues (particularly because that callback doesn't provide an "exitCode" arg to check for this behavior against). Hey @lacraig2 - do you know which of these behaviors we should expect from EBE?
There was a problem hiding this comment.
(I'll move this discussion over to the new PR)
The old callstack instr logic would miss many calls if tb_chaining was enabled (which is is by default). Since this plugin did not disable tb_chaining, many calls would be missed if a user didn't disable chaining or load another plugin that disabled chaining.
This commit updates the plugin to use start_block_exec and end_block_exec which work even with tb_chaining enabled.
Thanks to @lacraig2 for suggesting this fix.
Hopefully fixes #1443.
One slight concern about this change is how the EBE callback works with interrupts - I know ABE is triggered when a block is interrupted and callbacks are infored of this through the
exitCodearg which callstack_instr was checking. If EBE is just not called when this happens, this updated code will be correct. Otherwise, if it's still called on these weird "end" blocks, I'm not sure these changes will be correct.