#2221 Watchpoints get a wrong PC during interrupts

[rice123]

Activate the monitor and enter:

a 1000 sei
lda #$00
sta $d012
lda #$01
sta $d01a
lda #$00
sta $0314
lda #$30
sta $0315
cli
inc $2000
jmp $1016

a 3000 inc $d019
jmp $ea31

r pc = 1000
watch 2000 if (pc >= $3000)
x

The watchpoint is triggered even though the program counter is below $3000:

#1 (Stop on  load 2000)  256/$100,  14/$0e
.C:1016  EE 00 20    INC $2000      - A:30 X:00 Y:0A SP:f0 ..-..I..    5873630
(C:$ff48) x
#1 (Stop on store 2000)  256/$100,  14/$0e
.C:1016  EE 00 20    INC $2000      - A:30 X:00 Y:0A SP:f0 ..-..I..    5873630
(C:$ff48) watch
WATCH: 1  C:$2000  (Stop on load store)
    Condition: ( PC >= $3000 )

This is annoying when you want to watch memory accesses from a specific range of code.

[rice123]

I just noticed it also affects subroutine jumps:

a 1000 jmp $6000
rts

a 6000 jsr $1003
jmp $1000

r pc = 1000
watch 2000 9fff if (pc >= $1000) && (pc <= $1003)
x

The monitor thinks that loading the address high byte from $6002 happens when PC == $1003:

#1 (Stop on  load 6002)   12/$00c,  11/$0b
.C:6000  20 03 10    JSR $1003      - A:00 X:00 Y:0A SP:f1 ..-...Z.    5013047

[gpz]

the second example at least doesn't seem unexpected to me. Note that any registers used in the condition are "live" - the condition triggers when the expression becomes true. So when an instruction changes/updates PC (like JSR) that will cause this condition to become true mid-instruction (indeed after fetching the 3rd byte)

the first case is a bit odd, it looks like a similar thing, but i wouldn't expect it to be this stable (is it really?)

perhaps (i dont know if this is realistic) some special cases like this can be checked for, and then when the watchpoint hits, it also disasassembles the instruction after the PC changed

[rice123]

is it correct to assume that in this code from mon_breakpoint_check_checkpoint:

    instpc = new_addr(mem, (monitor_cpu->mon_register_get_val)(mem, e_PC));
    loadstorepc = new_addr(mem, lastpc);

instpc is $1003 and loadstorepc is $6000 for the second example? Cant check myself without debugging symbols for WinDbg.

If this assumption is correct then maybe it can be special cased with something like this:

diff --git a/mon_breakpoint.c b/mon_breakpoint.c
index a8d2bd4..840b8d6 100644
--- a/mon_breakpoint.c
+++ b/mon_breakpoint.c
@@ -534,6 +534,11 @@ bool mon_breakpoint_check_checkpoint(MEMSPACE mem, unsigned int addr, unsigned i

     ptr = search_checkpoint_list(list, addr);


+    // Temporarily revert PC for `mon_evaluate_conditional()` calls below
+    if (is_loadstore) {
+      monitor_cpu->mon_register_set_val(mem, e_PC, (uint16_t)(addr_location(loadstorepc)));
+    }
+
     while (ptr) {
         cp = ptr->checkpt;
         ptr = ptr->next;
@@ -608,6 +613,11 @@ bool mon_breakpoint_check_checkpoint(MEMSPACE mem, unsigned int addr, unsigned i
         }
     }


+    // Restore the current PC
+    if (is_loadstore) {
+      monitor_cpu->mon_register_set_val(mem, e_PC, (uint16_t)(addr_location(instpc)));
+    }
+
     return must_stop;
 }

[gpz]

I have digged a bit.... first of all, it looks like the code in mon_breakpoint_check_checkpoint() actually works correctly. ultimatively, it is getting the wrong value for the PC via mon_register_get_val().

I have a similar (a bit cleaner) patch locally for the is_loadstore case...

• for this first case, it does no more break
• for the second example it breaks like this:

#1 (Stop on  load 6002)    0/$000,  17/$11
.C:1003  60          RTS            - A:00 X:00 Y:0A SP:f3 ..-B..Z.         15

which makes complete sense to me (dummy fetch from return address -1)

does that make sense? any other special case we should check? :)

[gpz]

comitted in r46128, please test

[rice123]

The first case (interrupts) works fine, thanks.

which makes complete sense to me (dummy fetch from return address -1)
does that make sense?

I don't think that discarded dummy fetches should trigger watchpoints (that's only relevant for a handful of registers where some action is taken on access, or is there some other special case?), but even ignoring that, I don't see the dummy fetch from return address - 1 you're referring to. The dummy fetch in RTS is done before pulling the return address from the stack:

 RTS

    #  address R/W description
   --- ------- --- -----------------------------------------------
    1    PC     R  fetch opcode, increment PC
    2    PC     R  read next instruction byte (and throw it away)
    3  $0100,S  R  increment S
    4  $0100,S  R  pull PCL from stack, increment S
    5  $0100,S  R  pull PCH from stack
    6    PC     R  increment PC

I think that'd access $1004 in this case. Are you sure that the load watchpoint isn't triggered by the previous JSR instruction?

 JSR

    #  address R/W description
   --- ------- --- -------------------------------------------------
    1    PC     R  fetch opcode, increment PC
    2    PC     R  fetch low address byte, increment PC
    3  $0100,S  R  internal operation (predecrement S?)
    4  $0100,S  W  push PCH on stack, decrement S
    5  $0100,S  W  push PCL on stack, decrement S
    6    PC     R  copy low address byte to PCL, fetch high address
                   byte to PCH

$6002 is fetched in cycle 6 while PC is still in the state set in cycle 2. It seems that mon_breakpoint_check_checkpoint() is called after the new PC is set and the current patch isn't 100% sufficient to fix this case. Maybe the passed lastpc is already wrong?

[rice123]

oookay, so this part of RTS description (coming from the ancient 64doc.txt) turned out to be incomplete:

    6    PC     R  increment PC

RTS does actually read from PC before incrementing it in cycle 6 (i.e. the operation is the same as in cycle 1) and that's in fact what causes the watchpoint to trigger.

That being said, it still shouldn't trigger. The monitor maintains the dummy option which defaults to being disabled:

(C:$e5cf) dummy
Checkpoints will not trigger on dummy accesses.

The bug is in 6510dtvcore.c:

#define RTS()                 \
    do {                      \
        uint16_t tmp;         \
                              \
        CHECK_PROFILE_RTS();  \
        if (!SKIP_CYCLE) {    \
            STACK_PEEK();     \
            CLK_INC();        \
        }                     \
        tmp = PULL();         \
        CLK_INC();            \
        tmp |= (PULL() << 8); \
        CLK_INC();            \
        LOAD(tmp);            \
        CLK_INC();            \
        tmp++;                \
        JUMP(tmp);            \
    } while (0)

LOAD(tmp); should be replaced with LOAD_DUMMY(tmp);

[gpz]

oh wow, another good catch. actually cycle 2 and 3 are dummy cycles too, will check in detail asap

[gpz]

applied in r46201 (can we close this now?)

[rice123]

cycle 3 is handled correctly in STACK_PEEK():

#define STACK_PEEK()  memmap_mem_read_dummy(0x100 + reg_sp)

functionally the same thing as LOAD_DUMMY:

#define LOAD_DUMMY(addr) \
    memmap_mem_read_dummy(addr)

cycle 2 is where things get hairy... cycles 1 & 2 are handled generically in FETCH_OPCODE():

/* FETCH_OPCODE implementation(s) */
#if !defined WORDS_BIGENDIAN && defined ALLOW_UNALIGNED_ACCESS
#define FETCH_OPCODE(o)                                        \
    do {                                                       \
        if (((int)reg_pc) < bank_limit) {                      \
            check_ba();                                        \
            o = (*((uint32_t *)(bank_base + reg_pc)) & 0xffffff); \
            MEMMAP_UPDATE(reg_pc);                             \
            SET_LAST_OPCODE(p0);                               \
            CLK_INC();                                         \
            check_ba();                                        \
            CLK_INC();                                         \
            if (fetch_tab[o & 0xff]) {                         \
                check_ba();                                    \
                CLK_INC();                                     \
            }                                                  \
        } else {                                               \
            o = LOAD(reg_pc);                                  \
            SET_LAST_OPCODE(p0);                               \
            CLK_INC();                                         \
            o |= LOAD(reg_pc + 1) << 8;                        \
            CLK_INC();                                         \
            if (fetch_tab[o & 0xff]) {                         \
                o |= (LOAD(reg_pc + 2) << 16);                 \
                CLK_INC();                                     \
            }                                                  \
        }                                                      \
    } while (0)

There are two branches where one is optimized, the other is not, and they're functionally different. In the fast case (if (((int)reg_pc) < bank_limit)) the memory access is direct and bypasses the generic load handlers ((*((uint32_t *)(bank_base + reg_pc)) & 0xffffff);). As a result, cycles 1 & 2 can't possibly trigger any load watchpoints, even if dummy is on.

In the slow case (else), the loads in cycles 1 & 2 always trigger load watchpoints, even if dummy is off. This difference between the fast and slow case is observable, for example in these two test cases:

a 1000 lda #$00

watch load 1000 1001
r pc = 1000
step
step

a 9ffd lda #$00

watch load 9ffd 9ffe
r pc = 9ffd
step
step

the load watchpoints will trigger for the second case but not for the first one :)

The question is not only what's correct, but also what's useful for users. Let's assume this code:
$1000 LDA #$00
When a user sets a load watchpoint on $1001, do they really want the watchpoint to trigger when the code at $1000 is executed? From my experience with debugging stuff, what's really useful is catching accesses made externally, i.e. it's meaningful to know that there's $9500 LDA $1001, but a watchpoint triggered when $1000 is executed would be noise even if valid.

I'd personally change all three LOADs in FETCH_OPCODE() to LOAD_DUMMY for a better watchpoint experience, but I can see how that could be perceived as incorrect :)