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

websid/src/cia.c

Lines 841 to 881 in dd6e107

	void ciaReset(uint8_t is_rsid, uint8_t is_ntsc) {
	ciaClock = &ciaClockRSID; // default
	_is_rsid = is_rsid;
	initMem(0xdc00, 0x7f); // S_W_A_F_2_tune_1.sid
	initMem(0xdc01, 0xff); // Master_Blaster_intro.sid/instantfunk.sid actually check this
	// CIA 1 defaults (by default C64 is configured with CIA1 timer / not raster irq)
	initMem(0xdc0d, 0x81); // interrupt control (interrupt through timer A)
	initMem(0xdc0e, 0x01); // control timer A: start - must already be started (e.g. Phobia, GianaSisters, etc expect it)
	initMem(0xdc0f, 0x08); // control timer B (start/stop) means auto-restart
	// see ROM $FDDD: initalise TAL1/TAH1 for 1/60 of a second (always!)
	const uint32_t c = is_ntsc ? 0x4295 : 0x4025;
	initMem(0xdc04, c & 0xff);
	initMem(0xdc05, c >> 8);
	if (_is_rsid) {
	initMem(0xdc06, 0xff);
	initMem(0xdc07, 0xff);
	// CIA 2 defaults
	initMem(0xdd04, 0xff); // see Great_Giana_Sisters.sid
	initMem(0xdd05, 0xff);
	initMem(0xdd06, 0xff);
	initMem(0xdd07, 0xff);
	// initMem(0xdd0d, 0x00); // redundant
	initMem(0xdd0e, 0x08); // control timer 2A (start/stop) default: ONE SHOT
	initMem(0xdd0f, 0x08); // control timer 2B (start/stop)
	}
	// todo: rather than using the above memory settings to then init
	// the timers below, regular timer access API should be used, avoiding
	// duplicate logic and the risk of init-logic getting out of sync
	initTimerData(ADDR_CIA1, &(_cia[0]));
	initTimerData(ADDR_CIA2, &(_cia[1]));
	_tod_in_millies = 0;
	}

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

websid/src/cia.c

Lines 808 to 819 in dd6e107

	static void initMem(uint16_t addr, uint8_t value) {
	if (!_is_rsid) { // needed by MasterComposer crap
	memWriteRAM(addr, value);
	}
	// FIXME since timers are later bootstrapped with settings
	// from IO memory area the previous update of timer state via
	// ciaWriteMem here does not make any sense.. (leave cleanup for next retesting)
	ciaWriteMem(addr, value); // also write RO defaults, e.g. dc01
	memWriteIO(addr, value);
	}

ciaWriteMem calls setInterruptMask with uninitialized timer

websid/src/cia.c

Lines 789 to 794 in dd6e107

	case 0xdd0d:
	if (!_is_rsid) {
	value &= 0x7f; // don't allow PSID crap to trigger NMIs
	}
	setInterruptMask(&(_cia[CIA2]), value);
	break;

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

websid/src/cia.c

Lines 332 to 339 in dd6e107

	static void setInterruptMask(struct Timer* t, uint8_t mask) {
	// i.e. $Dx0D, handle updates of the CIA interrupt control mask
	const uint16_t addr = t->memory_address + 0x0d;
	if (mask & ICR_SRC_SET) {
	uint8_t new_mask = memReadIO(addr) | (mask & 0x1f); // set mask bits
	UPDATE_INT_MASK(t, addr, new_mask);

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

websid/src/memory.c

Lines 155 to 157 in dd6e107

	void memWriteIO(uint16_t addr, uint8_t value) {
	_io_area[addr - 0xd000] = value;
	}

Second issue

This issue relates to setInterruptMask too. Look at initMem:

websid/src/cia.c

Lines 808 to 819 in dd6e107

	static void initMem(uint16_t addr, uint8_t value) {
	if (!_is_rsid) { // needed by MasterComposer crap
	memWriteRAM(addr, value);
	}
	// FIXME since timers are later bootstrapped with settings
	// from IO memory area the previous update of timer state via
	// ciaWriteMem here does not make any sense.. (leave cleanup for next retesting)
	ciaWriteMem(addr, value); // also write RO defaults, e.g. dc01
	memWriteIO(addr, value);
	}

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