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.

Sign up for GitHub

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

CallstackInstr: switch to SBE/EBE callbacks #1445

Merged
merged 1 commit into from
Feb 19, 2024
Merged

CallstackInstr: switch to SBE/EBE callbacks #1445

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 18 additions & 36 deletions panda/plugins/callstack_instr/callstack_instr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,8 @@ extern "C" {
#include "panda/plog.h"
#include "callstack_instr_int_fns.h"

bool translate_callback(CPUState* cpu, target_ulong pc);
int exec_callback(CPUState* cpu, target_ulong pc);
void before_block_exec(CPUState* cpu, TranslationBlock *tb);
void after_block_exec(CPUState* cpu, TranslationBlock *tb, uint8_t exitCode);
void start_block_exec(CPUState* cpu, TranslationBlock *tb);
void end_block_exec(CPUState* cpu, TranslationBlock *tb);
void after_block_translate(CPUState* cpu, TranslationBlock *tb);

bool init_plugin(void *);
Expand Down Expand Up @@ -322,7 +320,7 @@ void after_block_translate(CPUState *cpu, TranslationBlock *tb) {
return;
}

void before_block_exec(CPUState *cpu, TranslationBlock *tb) {
void start_block_exec(CPUState *cpu, TranslationBlock *tb) {
// if the block a call returns to was interrupted before it completed, this
// function will be called twice - only want to remove the return value from
// the stack once
Expand Down Expand Up @@ -366,7 +364,7 @@ void before_block_exec(CPUState *cpu, TranslationBlock *tb) {
}
}

void after_block_exec(CPUState* cpu, TranslationBlock *tb, uint8_t exitCode) {
void end_block_exec(CPUState* cpu, TranslationBlock *tb) {
target_ulong pc = 0x0;
target_ulong cs_base = 0x0;
uint32_t flags = 0x0;
Expand All @@ -377,35 +375,19 @@ void after_block_exec(CPUState* cpu, TranslationBlock *tb, uint8_t exitCode) {

// sometimes an attempt to run a block is interrupted, but this callback is
// still made - only update the callstack if the block has run to completion
if (exitCode <= TB_EXIT_IDX1) {
if (tb_type == INSTR_CALL) {
stack_entry se = {tb->pc + tb->size, tb_type};
callstacks[curStackid].push_back(se);

// Also track the function that gets called
// This retrieves the pc in an architecture-neutral way
cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
function_stacks[curStackid].push_back(pc);

PPP_RUN_CB(on_call, cpu, pc);
} else if (tb_type == INSTR_RET) {
//printf("Just executed a RET in TB " TARGET_FMT_lx "\n", tb->pc);
//if (next) printf("Next TB: " TARGET_FMT_lx "\n", next->pc);
}
}
// in case this block is one that a call returns to, need to node that its
// execution was interrupted, so don't try to remove it from the callstack
// when try (as already removed before this attempt)
else {
// verbose output is helpful in regression testing
if (tb_type == INSTR_CALL) {
verbose_log("callstack_instr not adding Stopped caller to stack",
tb, curStackid, true);
}
if (tb_type == INSTR_CALL) {
stack_entry se = {tb->pc + tb->size, tb_type};
callstacks[curStackid].push_back(se);

// Also track the function that gets called
// This retrieves the pc in an architecture-neutral way
cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
// erase nicely does nothing if key DNE
stoppedInfo.erase(curStackid);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I'll move this discussion over to the new PR)

stoppedInfo[curStackid] = pc;
function_stacks[curStackid].push_back(pc);

PPP_RUN_CB(on_call, cpu, pc);
} else if (tb_type == INSTR_RET) {
//printf("Just executed a RET in TB " TARGET_FMT_lx "\n", tb->pc);
//if (next) printf("Next TB: " TARGET_FMT_lx "\n", next->pc);
}
}

Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still AFTER_BLOCK_EXEC? Is that what you intended? Same with BEFORE_BLOCK_EXEC below. @LauraLMann spotted this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be missing something super obvious though

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I am missing something super obvious! Thanks, I'l fix

pcb.before_block_exec = before_block_exec;
pcb.start_block_exec = start_block_exec;
panda_register_callback(self, PANDA_CB_BEFORE_BLOCK_EXEC, pcb);

bool setup_ok = true;
Expand Down
Loading