From d2d3d14c411ea43a46cbfd72915883e619e4e158 Mon Sep 17 00:00:00 2001 From: Ryan Fleury Date: Mon, 5 Feb 2024 10:47:57 -0800 Subject: [PATCH] demon/win32: roll back on all hit traps, even if explicit. the previous implementation would silently skip threads past explicit traps that they hit, as a way of implicitly storing the fact that trap exceptions had been reported, and the user could continue past them. this resulted in incorrect instruction pointer display in those circumstances. this change adjusts this, so that after a trap exception of any kind, the instruction pointer is ALWAYS rolled back. to ensure that the trap is not repeatedly hit, if the associated exception has already been reported, to allow the user to e.g. step over traps (this is the behavior of Visual Studio), additional state is stored per-thread-entity, which allows a subsequent demon_os_run to adjust RIPs past their previously reported traps before running again. --- src/ctrl/ctrl_core.c | 4 +- src/demon/demon_accel.c | 6 +-- src/demon/demon_common.h | 4 +- src/demon/demon_inc.c | 1 + src/demon/demon_os.c | 28 ++++++++++ src/demon/demon_os.h | 6 +++ src/demon/win32/demon_os_win32.c | 90 +++++++++++++++++++++++--------- src/demon/win32/demon_os_win32.h | 3 ++ src/mule/mule_main.cpp | 27 ++++++++++ 9 files changed, 135 insertions(+), 34 deletions(-) create mode 100644 src/demon/demon_os.c diff --git a/src/ctrl/ctrl_core.c b/src/ctrl/ctrl_core.c index e724ce10..0ea0862a 100644 --- a/src/ctrl/ctrl_core.c +++ b/src/ctrl/ctrl_core.c @@ -767,7 +767,9 @@ ctrl_process_write(CTRL_MachineID machine_id, CTRL_Handle process, Rng1U64 range ins_atomic_u64_inc_eval(&ctrl_state->memgen_idx); } - //- rjf: success -> forcibly, synchronously update cache, for small regions + //- rjf: success -> wait for cache updates, for small regions - prefer relatively seamless + // writes within calling frame's "view" of the memory, at the expense of a small amount of + // time. if(result) { Temp scratch = scratch_begin(0, 0); diff --git a/src/demon/demon_accel.c b/src/demon/demon_accel.c index 2db93c6a..c9b9ede4 100644 --- a/src/demon/demon_accel.c +++ b/src/demon/demon_accel.c @@ -144,11 +144,7 @@ demon_accel_read_regs(DEMON_Entity *thread){ // update reg cache if (accel->reg_cache_time != demon_time){ accel->reg_cache_time = demon_time; - B32 success = 0; - switch (thread->arch){ - case Architecture_x86:{success = demon_os_read_regs_x86(thread, &accel->regs.x86);}break; - case Architecture_x64:{success = demon_os_read_regs_x64(thread, &accel->regs.x64);}break; - } + B32 success = demon_os_read_regs(thread, &accel->regs); if (!success){ MemoryZeroStruct(&accel->regs); } diff --git a/src/demon/demon_common.h b/src/demon/demon_common.h index 4375f079..07ec4765 100644 --- a/src/demon/demon_common.h +++ b/src/demon/demon_common.h @@ -30,8 +30,8 @@ struct DEMON_Entity DEMON_Entity *first; DEMON_Entity *last; - U16 kind; - U16 arch; + DEMON_EntityKind kind; + Architecture arch; U32 gen; U64 id; U64 addr_range_dim; diff --git a/src/demon/demon_inc.c b/src/demon/demon_inc.c index 36c7a743..64a45e09 100644 --- a/src/demon/demon_inc.c +++ b/src/demon/demon_inc.c @@ -4,6 +4,7 @@ #include "demon_core.c" #include "demon_common.c" #include "demon_accel.c" +#include "demon_os.c" #if OS_WINDOWS # include "win32/demon_os_win32.c" diff --git a/src/demon/demon_os.c b/src/demon/demon_os.c new file mode 100644 index 00000000..76c55a9b --- /dev/null +++ b/src/demon/demon_os.c @@ -0,0 +1,28 @@ +//////////////////////////////// +//~ rjf: Helpers + +internal B32 +demon_os_read_regs(DEMON_Entity *thread, void *dst) +{ + B32 result = 0; + switch(thread->arch) + { + default:{}break; + case Architecture_x86:{result = demon_os_read_regs_x86(thread, (REGS_RegBlockX86 *)dst);}break; + case Architecture_x64:{result = demon_os_read_regs_x64(thread, (REGS_RegBlockX64 *)dst);}break; + } + return result; +} + +internal B32 +demon_os_write_regs(DEMON_Entity *thread, void *src) +{ + B32 result = 0; + switch(thread->arch) + { + default:{}break; + case Architecture_x86:{result = demon_os_write_regs_x86(thread, (REGS_RegBlockX86 *)src);}break; + case Architecture_x64:{result = demon_os_write_regs_x64(thread, (REGS_RegBlockX64 *)src);}break; + } + return result; +} diff --git a/src/demon/demon_os.h b/src/demon/demon_os.h index 1b0bc593..fd6aac59 100644 --- a/src/demon/demon_os.h +++ b/src/demon/demon_os.h @@ -35,6 +35,12 @@ struct DEMON_OS_RunCtrls U64 trap_count; }; +//////////////////////////////// +//~ rjf: Helpers + +internal B32 demon_os_read_regs(DEMON_Entity *thread, void *dst); +internal B32 demon_os_write_regs(DEMON_Entity *thread, void *src); + //////////////////////////////// //~ rjf: @demon_os_hooks Main Layer Initialization diff --git a/src/demon/win32/demon_os_win32.c b/src/demon/win32/demon_os_win32.c index 5f81258b..1366cb49 100644 --- a/src/demon/win32/demon_os_win32.c +++ b/src/demon/win32/demon_os_win32.c @@ -571,20 +571,24 @@ demon_os_run(Arena *arena, DEMON_OS_RunCtrls *ctrls){ } } - // prep threads that will be allowed to run - for (DEMON_W32_EntityNode *node = first_run_thread; - node != 0; - node = node->next){ + // prep suspension state of threads that will be allowed to run + for(DEMON_W32_EntityNode *node = first_run_thread; + node != 0; + node = node->next) + { DEMON_Entity *thread = node->entity; DEMON_W32_Ext *thread_ext = demon_w32_ext(thread); DWORD resume_result = ResumeThread(thread_ext->thread.handle); - if (resume_result == max_U32){ + if(resume_result == max_U32) + { // TODO(allen): Error. Unknown cause (do GetLastError, FromatMessage) } - else{ + else + { DWORD desired_counter = 0; DWORD current_counter = resume_result - 1; - if (current_counter != desired_counter){ + if(current_counter != desired_counter) + { // NOTE(rjf): Warning. The user has manually suspended this thread, // so even though from Demon's perspective it thinks this thread // should run, it will not, because the user has manually called @@ -593,6 +597,33 @@ demon_os_run(Arena *arena, DEMON_OS_RunCtrls *ctrls){ } } + // rjf: if run threads are marked as having reported an explicit trap + // on their last run, shift their RIPs past that trap instruction, so + // that they may continue + for(DEMON_W32_EntityNode *node = first_run_thread; + node != 0; + node = node->next) + { + DEMON_Entity *thread = node->entity; + DEMON_W32_Ext *thread_ext = demon_w32_ext(thread); + if(thread_ext->thread.last_run_reported_trap) + { + Temp temp = temp_begin(scratch.arena); + U64 regs_block_size = regs_block_size_from_architecture(thread->arch); + void *regs_block = push_array(temp.arena, U8, regs_block_size); + B32 good = demon_os_read_regs(thread, regs_block); + U64 pre_rip = regs_rip_from_arch_block(thread->arch, regs_block); + if(good && pre_rip == thread_ext->thread.last_run_reported_trap_pre_rip) + { + regs_arch_block_write_rip(thread->arch, regs_block, thread_ext->thread.last_run_reported_trap_post_rip); + demon_os_write_regs(thread, regs_block); + } + temp_end(temp); + thread_ext->thread.last_run_reported_trap = 0; + thread_ext->thread.last_run_reported_trap_post_rip = 0; + } + } + // send last saved continue signal B32 good_state = 1; if (demon_w32_resume_needed != 0){ @@ -988,27 +1019,22 @@ demon_os_run(Arena *arena, DEMON_OS_RunCtrls *ctrls){ } // rjf: determine whether to roll back instruction pointer - B32 rollback = (is_trap && !hit_explicit_trap); + B32 rollback = (is_trap); // rjf: roll back - if (rollback){ - // TODO(allen): possibly buggy - switch (thread->arch){ - case Architecture_x86: - { - REGS_RegBlockX86 regs = {0}; - demon_os_read_regs_x86(thread, ®s); - regs.eip.u32 = instruction_pointer; - demon_os_write_regs_x86(thread, ®s); - }break; - case Architecture_x64: - { - REGS_RegBlockX64 regs = {0}; - demon_os_read_regs_x64(thread, ®s); - regs.rip.u64 = instruction_pointer; - demon_os_write_regs_x64(thread, ®s); - }break; + U64 post_trap_rip = 0; + if(rollback) + { + Temp temp = temp_begin(scratch.arena); + U64 regs_block_size = regs_block_size_from_architecture(thread->arch); + void *regs_block = push_array(scratch.arena, U8, regs_block_size); + if(demon_os_read_regs(thread, regs_block)) + { + post_trap_rip = regs_rip_from_arch_block(thread->arch, regs_block); + regs_arch_block_write_rip(thread->arch, regs_block, instruction_pointer); + demon_os_write_regs(thread, regs_block); } + temp_end(temp); } // allen: if this is not a user trap or explicit trap it's a previous trap @@ -1018,7 +1044,9 @@ demon_os_run(Arena *arena, DEMON_OS_RunCtrls *ctrls){ B32 skip_event = (hit_previous_trap); // emit event - if (!skip_event){ + if(!skip_event) + { + // rjf: fill top-level info DEMON_Event *e = demon_push_event(arena, &result, DEMON_EventKind_Exception); e->process = demon_ent_handle_from_ptr(process); e->thread = demon_ent_handle_from_ptr(thread); @@ -1026,6 +1054,16 @@ demon_os_run(Arena *arena, DEMON_OS_RunCtrls *ctrls){ e->flags = exception->ExceptionFlags; e->instruction_pointer = (U64)exception->ExceptionAddress; + // rjf: explicit trap -> mark this thread as having reported this trap + if(hit_explicit_trap) + { + DEMON_W32_Ext *thread_ext = demon_w32_ext(thread); + thread_ext->thread.last_run_reported_trap = 1; + thread_ext->thread.last_run_reported_trap_pre_rip = instruction_pointer; + thread_ext->thread.last_run_reported_trap_post_rip = post_trap_rip; + } + + // rjf: fill by exception code switch (exception->ExceptionCode){ case DEMON_W32_EXCEPTION_BREAKPOINT: { diff --git a/src/demon/win32/demon_os_win32.h b/src/demon/win32/demon_os_win32.h index 3fcea46a..7233e0d0 100644 --- a/src/demon/win32/demon_os_win32.h +++ b/src/demon/win32/demon_os_win32.h @@ -37,6 +37,9 @@ union DEMON_W32_Ext U64 thread_local_base; U64 last_name_hash; U64 name_gather_time_us; + B32 last_run_reported_trap; + U64 last_run_reported_trap_pre_rip; + U64 last_run_reported_trap_post_rip; } thread; struct{ HANDLE handle; diff --git a/src/mule/mule_main.cpp b/src/mule/mule_main.cpp index 78dd90a1..87f6c7ef 100644 --- a/src/mule/mule_main.cpp +++ b/src/mule/mule_main.cpp @@ -2266,6 +2266,31 @@ debug_string_tests(void) #endif } +//////////////////////////////// +//~ rjf: Interrupt Stepping Tests + +#include + +static void +interrupt_stepping_tests(void) +{ + for(int i = 0; i < 1000; i += 1) + { + if(i == 999) + { + __debugbreak(); + } + } + for(int i = 0; i < 1000; i += 1) + { + if(i == 999) + { + assert(0); + } + } + int x = 0; +} + //////////////////////////////// // NOTE(allen): Exception Stepping @@ -2561,6 +2586,8 @@ mule_main(int argc, char** argv){ debug_string_tests(); + interrupt_stepping_tests(); + exception_stepping_tests(); return(0);