Fixed an issue where int3 breakpoints installed by the debugger were …

…endlessly looping again after being triggered once

lib86cpu/core/translate.cpp

@@ -1937,6 +1937,15 @@ cpu_exec_trampoline(cpu_t *cpu, const uint32_t ret_eip)
 	cpu_main_loop<true, false>(cpu, [cpu, ret_eip]() { return cpu->cpu_ctx.regs.eip != ret_eip; });
 }
 
+void
+dbg_exec_original_instr(cpu_t *cpu)
+{
+	cpu->cpu_flags |= CPU_DISAS_ONE;
+	// run the main loop only once, since we only execute the original instr that was replaced by int3
+	int i = 0;
+	cpu_main_loop<false, false>(cpu, [&i]() { return i++ == 0; });
+}
+
 template translated_code_t *cpu_raise_exception<0, true>(cpu_ctx_t *cpu_ctx);
 template translated_code_t *cpu_raise_exception<1, true>(cpu_ctx_t *cpu_ctx);
 template translated_code_t *cpu_raise_exception<2, true>(cpu_ctx_t *cpu_ctx);

lib86cpu/dbg/debugger.cpp

@@ -515,6 +515,9 @@ dbg_sw_breakpoint_handler(cpu_ctx_t *cpu_ctx)
 				iret_real_helper(cpu_ctx, (cpu_ctx->hflags & HFLG_CS32) ? SIZE32 : SIZE16, cpu_ctx->regs.eip);
 			}
 			cpu_ctx->regs.eip = ret_eip - 1;
+			dbg_remove_sw_breakpoints(cpu_ctx->cpu, pc);
+			dbg_exec_original_instr(cpu_ctx->cpu);
+			dbg_apply_sw_breakpoints(cpu_ctx->cpu, pc);
 		}
 		catch (host_exp_t type) {
 			// we can't handle an exception here, so abort
@@ -573,8 +576,8 @@ dbg_insert_sw_breakpoint(cpu_t *cpu, addr_t addr)
 	return inserted;
 }
 
-void
-dbg_apply_sw_breakpoints(cpu_t *cpu)
+template<typename T>
+static void dbg_update_sw_breakpoints(cpu_t *cpu, T &&lambda)
 {
 	// set cpl to zero and clear wp of cr0, so that we can write to read-only pages
 	uint8_t old_cpl = cpu->cpu_ctx.hflags & HFLG_CPL;
@@ -588,14 +591,7 @@ dbg_apply_sw_breakpoints(cpu_t *cpu)
 	wp_data.swap(cpu->wp_data);
 	wp_io.swap(cpu->wp_io);
 
-	for (const auto &elem : break_list) {
-		addr_t addr = elem.first;
-
-		// the mem accesses below cannot raise page faults since break_list can only contain valid pages because of the checks done in insert_sw_breakpoint
-		uint8_t original_byte = mem_read_helper<uint8_t>(&cpu->cpu_ctx, addr, cpu->cpu_ctx.regs.eip, 0);
-		mem_write_helper<uint8_t>(&cpu->cpu_ctx, addr, 0xCC, cpu->cpu_ctx.regs.eip, 0);
-		break_list.insert_or_assign(addr, original_byte);
-	}
+	lambda(cpu);
 
 	(cpu->cpu_ctx.hflags &= ~HFLG_CPL) |= old_cpl;
 	(cpu->cpu_ctx.regs.cr0 &= ~CR0_WP_MASK) |= old_wp;
@@ -604,33 +600,59 @@ dbg_apply_sw_breakpoints(cpu_t *cpu)
 }
 
 void
-dbg_remove_sw_breakpoints(cpu_t *cpu)
+dbg_apply_sw_breakpoints(cpu_t *cpu)
 {
-	// set cpl to zero and clear wp of cr0, so that we can write to read-only pages
-	uint8_t old_cpl = cpu->cpu_ctx.hflags & HFLG_CPL;
-	cpu->cpu_ctx.hflags &= ~HFLG_CPL;
-	uint32_t old_wp = cpu->cpu_ctx.regs.cr0 & CR0_WP_MASK;
-	cpu->cpu_ctx.regs.cr0 &= ~CR0_WP_MASK;
+	dbg_update_sw_breakpoints(cpu, [](cpu_t *cpu) {
+		for (const auto &elem : break_list) {
+			addr_t addr = elem.first;
+
+			// the mem accesses below cannot raise page faults since break_list can only contain valid pages because of the checks done in dbg_insert_sw_breakpoint
+			uint8_t original_byte = mem_read_helper<uint8_t>(&cpu->cpu_ctx, addr, cpu->cpu_ctx.regs.eip, 0);
+			mem_write_helper<uint8_t>(&cpu->cpu_ctx, addr, 0xCC, cpu->cpu_ctx.regs.eip, 0);
+			break_list.insert_or_assign(addr, original_byte);
+		}
+		});
+}
 
-	// disable debug exp since we only want to remove a breakpoint
-	std::vector<wp_info<addr_t>> wp_data;
-	std::vector<wp_info<port_t>> wp_io;
-	wp_data.swap(cpu->wp_data);
-	wp_io.swap(cpu->wp_io);
+void
+dbg_apply_sw_breakpoints(cpu_t *cpu, addr_t addr)
+{
+	dbg_update_sw_breakpoints(cpu, [addr](cpu_t *cpu) {
+		// the mem accesses below cannot raise page faults since break_list can only contain valid pages because of the checks done in dbg_insert_sw_breakpoint
+		uint8_t original_byte = mem_read_helper<uint8_t>(&cpu->cpu_ctx, addr, cpu->cpu_ctx.regs.eip, 0);
+		mem_write_helper<uint8_t>(&cpu->cpu_ctx, addr, 0xCC, cpu->cpu_ctx.regs.eip, 0);
+		break_list.insert_or_assign(addr, original_byte);
+		});
+}
 
-	for (const auto &elem : break_list) {
-		const auto &[addr, original_byte] = elem;
+void
+dbg_remove_sw_breakpoints(cpu_t *cpu)
+{
+	dbg_update_sw_breakpoints(cpu, [](cpu_t *cpu) {
+		for (const auto &elem : break_list) {
+			const auto &[addr, original_byte] = elem;
 
-		try {
-			mem_write_helper<uint8_t>(&cpu->cpu_ctx, addr, original_byte, cpu->cpu_ctx.regs.eip, 0);
-		}
-		catch (host_exp_t type) {
-			// this can only happen when the page is invalid
+			try {
+				mem_write_helper<uint8_t>(&cpu->cpu_ctx, addr, original_byte, cpu->cpu_ctx.regs.eip, 0);
+			}
+			catch (host_exp_t type) {
+				LIB86CPU_ABORT_msg("Unhandled page fault while removing a sw breakpoint");
+			}
 		}
-	}
+		});
+}
 
-	(cpu->cpu_ctx.hflags &= ~HFLG_CPL) |= old_cpl;
-	(cpu->cpu_ctx.regs.cr0 &= ~CR0_WP_MASK) |= old_wp;
-	wp_data.swap(cpu->wp_data);
-	wp_io.swap(cpu->wp_io);
+void
+dbg_remove_sw_breakpoints(cpu_t *cpu, addr_t addr)
+{
+	dbg_update_sw_breakpoints(cpu, [addr](cpu_t *cpu) {
+		if (auto it = break_list.find(addr); it != break_list.end()) {
+			try {
+				mem_write_helper<uint8_t>(&cpu->cpu_ctx, addr, it->second, cpu->cpu_ctx.regs.eip, 0);
+			}
+			catch (host_exp_t type) {
+				LIB86CPU_ABORT_msg("Unhandled page fault while removing a sw breakpoint");
+			}
+		}
+		});
 }

lib86cpu/dbg/debugger.h

@@ -16,13 +16,16 @@ void read_setting_files(cpu_t *cpu);
 void write_setting_files(cpu_t *cpu);
 bool dbg_insert_sw_breakpoint(cpu_t *cpu, addr_t addr);
 void dbg_apply_sw_breakpoints(cpu_t *cpu);
+void dbg_apply_sw_breakpoints(cpu_t *cpu, addr_t addr);
 void dbg_remove_sw_breakpoints(cpu_t *cpu);
+void dbg_remove_sw_breakpoints(cpu_t *cpu, addr_t addr);
 void JIT_API dbg_update_exp_hook(cpu_ctx_t *cpu_ctx);
 void dbg_add_exp_hook(cpu_ctx_t *cpu_ctx);
 std::vector<std::pair<addr_t, std::string>> dbg_disas_code_block(cpu_t *cpu, addr_t pc, unsigned instr_num);
 void dbg_exp_handler(cpu_ctx_t *cpu_ctx);
 void dbg_ram_read(cpu_t *cpu, uint8_t *buff);
 void dbg_ram_write(uint8_t *data, size_t off, uint8_t val);
+void dbg_exec_original_instr(cpu_t *cpu);
 
 inline cpu_t *g_cpu;
 inline bool mem_editor_update = true;