Bug\UB in CIA timers initialization ?

[mitallast]

Hi! As first, thanks for great c64 implementation!

I found multiple possible bugs in CIA initialization of timers and ICR/IMR registers:

First issue

in this function initMem call happens before than initTimerData call

https://github.com/wothke/websid/blob/dd6e1078c50aa0879b1531305755c3da5eb700ea/src/cia.c#L841-L881

let's see to initMem, there's a call to ciaWriteMem and then memWriteIO

https://github.com/wothke/websid/blob/dd6e1078c50aa0879b1531305755c3da5eb700ea/src/cia.c#L808-L819

ciaWriteMem calls setInterruptMask with uninitialized timer

https://github.com/wothke/websid/blob/dd6e1078c50aa0879b1531305755c3da5eb700ea/src/cia.c#L789-L794

as default, timer's memory_address is equal to 0x0000, and setInterruptMask receives invalid memory address with non-predicted behaviour:

https://github.com/wothke/websid/blob/dd6e1078c50aa0879b1531305755c3da5eb700ea/src/cia.c#L332-L339

as result, const uint16_t addr will be result of expression like 0x0000 + 0x0d equal to -0x0d. Obliviously, this is is not a valid address for memWriteIO

https://github.com/wothke/websid/blob/dd6e1078c50aa0879b1531305755c3da5eb700ea/src/memory.c#L155-L157

Second issue

This issue relates to setInterruptMask too. Look at initMem:

https://github.com/wothke/websid/blob/dd6e1078c50aa0879b1531305755c3da5eb700ea/src/cia.c#L808-L819

ciaWriteMem compute correct new IMR value, but memWriteIO overrides it with write command to it. First write to IMR register works correct, but next reads & writes don't, because they read illegal value overriden by memWriteIO.

As example, you can load singularity.sid file with debugger, and view next sequence:

FileLoader::initTune()
  Core::startupTune()
    resetDefaults()
       ciaReset()
         initMem()
           ciaWriteMem(addr, value) // addr = 0xdc0d, value = 0x81
             setInterruptMask(value)
               old_mask = memReadIO(addr) // 0x00
               new_mask = old_mask | (value & 0x1f) // 0x01
               memWriteIO(addr, new_mask) //  0xdc0d now contains 0x01
           memWriteIO(addr, value) // addr = 0xdc0d, value = 0x81

Expected value at io memory by addr 0xdc0d is 0x01, but actual 0x81!

Next write/read will view 0x81, so error propagates to all next calls.

[wothke]

Now THIS is what I call be nice bug report! Well done! Thanks!

I had the cleanup of the timer initialization on my todo list (see "todo" and "fixme" comments) and your analysis just moved it to the top of the list. I'll look into it when I next touch the code.

PS: is this the song you are mentioning above: https://deepsid.chordian.net/?file=/MUSICIANS/S/Scarzix/Singularity_2SID.sid (if so, is there an audible effect of the bugs in this song?)

[mitallast]

LMAO, I saw a lot of your comments about the quality of PSID hacks & quality!

Yep, this particular song. Unfortunately, I don't know how it should be sounding on real hardware c64. I'm working on sid player written in rust lang, based of resid-rs crate - as part of my game, just for fun. It's just the first song that somehow worked on from scratch! My version sounds like this: https://soundcloud.com/mitallast/singularity-2sid