z80 icon indicating copy to clipboard operation
z80 copied to clipboard

Breakpoint incorrectly triggers on CALL.

Open mooskagh opened this issue 2 years ago • 3 comments

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.

mooskagh avatar Oct 17 '21 08:10 mooskagh

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.

kosarev avatar Oct 24 '21 19:10 kosarev

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;
  }
}

mooskagh avatar Dec 15 '21 12:12 mooskagh

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

kosarev avatar Dec 26 '21 14:12 kosarev