SPC700 `dbnz` and `cbne` instructions have an improper error condition that triggers on valid code

[exodustx0]

Reproducer:

asar 1.90
arch spc700
norom

org $0123 ; has to be $007F or higher
Foo: assert 0 != Foo ; has to have a label on rhs

- dbnz Y, -

This prints Errors were detected while assembling the patch. Assembling aborted. Your ROM has not been modified. and nothing more. The error is thrown from arch-spc700.cpp#L346, but only on pass 1, not the pass 2 which causes the actual error (relative_branch_out_of_bounds) to be printed (which is an incorrect error).

If you remove the assert (or have rhs not be a label), the only reason this error is not thrown on pass 1 is because foundlabel is false. However, in the assert code, the getnumdouble() call on Foo sets foundlabel = true and it thereafter is never reset, it persists until the dbnz code.

I am unable to tell where the resetting should've taken place. For that matter, I generally have trouble wrapping my head around Asar's assembler state, so I'm not excluding the possibility that I'm missing some context and it's some other piece of the puzzle where the bug resides.

[exodustx0]

This also happens with the macro that writes the other dbnz instruction as well as both cbne instructions, at arch-spc700.cpp#L349.

[exodustx0]

Oh, I think I have a better idea. The macro that writes all simple branch instructions checks for pass == 2 rather than just pass in the error condition (arch-spc700.cpp#L211), maybe that should've been used for dbnz and cbne too.

I'm gonna assume that this is the issue, going by the pass explanation provided in the pass loop in interface-cli.cpp.

[exodustx0]

Forgot to point this out earlier, but for the record, I'll note that at the very least there is an easy workaround: yoda-ing assert statements so that there is no label in rhs.

[Alcaro]

Another workaround is inserting some random assert with no labels, like assert 2+2 <= 5, before the offending dbnz/cbne.

Your choice which one is less ugly...

[exodustx0]

I would personally always favour yodaism over load-bearing magic (especially since the latter adds additional work, whereas the former just reorders), but yeah, while I think this is a somewhat serious bug in how sneaky it is, at least the workaround is trivial.

One annoying factor, however, is how the workaround doesn't involve itself directly with the place where the actual bug lives: you touch up the assert to fix the dbnz/cbne.

[exodustx0]

I'm having a lot of trouble using dbnz and cbne because of this. I can't find where else foundlabel == true is being leaked, but it is somewhere and I can't find an assert statement with a label as rhs. I know there's other places where it is leaked (such as in the org and base handlers), but I'm mentioning this to point out that the workaround isn't as trivial as I thought. I'm gonna be using a macro to manually db any dbnz and cbne instructions to ensure I won't hit this issue while working on my code until this issue is fixed.

[trillllian]

foundlabel isn't the problem here, it's always set with whatever the last math parse returned. real problem is that arch-spc700 only ever does math in pass 2 (i think i did this "recently" as a way to fix some other phantom errors), whereas this check is done on pass 1 too, so it gets garbage data there. the wr and w1r macros are the only thing that check if(pass) instead of if(pass==2), i think swapping that around should make it act nicer???

[exodustx0]

I'd expect as much! Local testing confirms, at the very least. But the logic is sound as far as I can tell.

[trillllian]

backported the fix to 1.9 aswell (and fixed another mistake related to pass which i noticed next to it)