search

stardot / beebem-windows

BBC micro emulator for Windows
http://www.mkw.me.uk/beebem/
Other
90 stars 36 forks source link

Fixed address wraparound in IndYAddrModeHandler_Data #9

Closed chrisn closed 7 years ago

chrisn commented 7 years ago

See http://stardot.org.uk/forums/viewtopic.php?f=4&t=12351

mungre commented 7 years ago

I think there's another bug in the original code!

The high byte of the address comes from WholeRam[ZPAddr+1]. If ZPAddr is 0xFF then the high byte will come from 0x100, which doesn't seem like the kind of thing the 6502 would do. I don't have a real one to hand to try it out but jsbeeb wraps this.

chrisn commented 7 years ago

Yes, that sounds right. I think we should find a test suite so we can see that this is fixed correctly.

mungre commented 7 years ago

It should be fairly straightforward to make Klaus Dormann's test program run on the beeb. I'll have a look at it.

fordp2002 commented 7 years ago

Rather than masking it it may be better to just declare the local variable as unsigned 16 bit?

ZornsLemma commented 7 years ago

This fix looks good to me, thanks for doing the work!

@mungre I'm fairly sure hoglet has posted a port of Klaus Dormann's test suite to stardot at some point (as a disc image, probably), although a quick search on there didn't find it. I can post on there asking him about it if you want (or of course you can post yourself) - I haven't posted yet as I don't want to interrupt if you're having fun looking at it yourself. :-)

mungre commented 7 years ago

@ZornsLemma Cheers, I was having fun and it's done now. I did have a look for previous efforts, but not terribly hard.

chrisn commented 7 years ago

@fordp2002 You're right, thanks for the suggestion!

I've updated the code and it passes the tests now. But I can see that other functions might have the same problem, e.g., IndYAddrModeHandler_Address, and possibly others?

mungre commented 7 years ago

You could change IndYAddrModeHandler_Data to call IndYAddrModeHandler_Address in both 6502core.cpp and tube.cpp. That will reduce the four copies of the calculation down to two! Searching for "IndY" doesn't turn up any other instances.

chrisn commented 7 years ago

For now, I've made the same change to both functions. I see that there's a pattern in the 6502 emulation code with separate _Address and _Data functions, so lots of duplication. Maybe something to be simplified in a separate refactoring?