kosarev / z80

Fast and flexible Z80/i8080 emulator with C++ and Python APIs
MIT License
63 stars 10 forks source link

Breakpoint incorrectly triggers on CALL. #39

Open mooskagh opened 2 years ago

mooskagh commented 2 years ago

Possibly that's bug on my side, I'll look into it from my side further.

So, I'm trying to implement breakpoints, in a way similar to one in z80.h.

What I do:

void Machine::run(uint64_t max_ops) {
  for (uint64_t op = 0; op < max_ops; ++op) {
    events_ = 0;
    on_step();
    if (events_ & kEventInterrupt) {
      ++interrupts;
      on_handle_active_int();
    }
    if (event_mask_ & events_) break;
  }
}
void Machine::on_set_pc(z80::fast_u16 addr) {
  if (breakpoints_[addr]) events_ |= kEventBreakpoint;
  base::on_set_pc(addr);
}

Now I have the following code:

...
40147 | CALL 41055
40150 | CALL 40433
40153 | CALL 41602
...
...
41055 | PUSH HL  
41056 | LD A,121
...

When I do:

my_machine.breakpoints_[40150] = true;
my_machine.run(1000000);
std::cout << my_machine.get_pc();

I expect it to stop at 40150, but instead 41055 is output.

kosarev commented 2 years ago

Right, on_set_pc() does not look to be the best handler to catch breakpoints. And I have doubts about on_fetch_cycle() and even on_m1_fetch_cycle(). on_step() maybe?

But it won't stop at 40150 anyway, if you set a breakpoint at that address. The general approach is that for a breakpoint or watchpoint to be hit, the instruction has to be actually executed. It's the debugger's job to then figure out which breakpoint it is.

mooskagh commented 2 years ago

In the end I did it in run(), in other places it had issues (especially given that I need to breakpoint before the instruction, like other libs do):

void Machine::run(uint64_t max_ops) {
  for (uint64_t op = 0; op < max_ops; ++op) {
    events_ = 0;
    on_step();
    if (events_ & kEventInterrupt) {
      ++interrupts;
      on_handle_active_int();
    }
    if (breakpoints_[get_pc()]) events_ |= kEventBreakpoint;
    if (event_mask_ & events_) break;
  }
}
kosarev commented 2 years ago

Yep, this should do. We couldn't do the same for watchpoints and other kinds of breakpoints, however, so we still might want to give this all a proper thought...