Interrupt-induced race condition in math32 lib for sdcc

[vmorilla]

Hi,

This corresponds to a pull-request I have just made with the solution (https://github.com/z88dk/z88dk/pull/2714), but wanted to share it with you in case it can help someone in the future. This bug was a nasty one and has been hunting me for days. In short, this is a race condition with interrupts in the sdcc math32 library. Since sdcc passed 8 bits values as just 1 octect in the stack, it is common to see the following pattern:

Code: Select all

	pop	bc	;return value
	dec	sp
	pop	af	;1 octect value goes to a
        push bc
        ...

However, in cm32_sdcc___uchar2fs_callee.asm we had this:

Code: Select all

	pop	bc	;return
	pop	hl	;value goes to L
	dec	sp
	push	bc
	ld	h,0
        ...

Apparently harmful and faster since the 1 byte value is stored directly in L... However, if there is an interruption right after the second pop, the content of the stack will be overwritten. So, it is very important that the stack is decremented before the pop, so that the content of the stack is never left vulnerable to interruptions:

Code: Select all

	pop	bc	;return
	dec	sp
	pop	hl	;value goes to H now, but stack is not exposed to undesired writes from an interruption
	push	bc
        ld  l,h
	ld  h,0

I wonder if there are similar bugs in the code... I see how easy is to introduce bugs like this. I also wonder why on earth sdcc has introduced this complexity. Does it really make a difference to save 1 byte in the stack?

[vmorilla]

BTW, I came accross this a part of the development of my new video-game for the Spectrum Next: Next Point. You can download it here: https://vmorilla.github.io/next-point/. I have to say I am quite happy with the results and I am receiving very good feedback, so I wanted to take the chance to thank all z88dk contributors, and DOM specially, for making this possible.

[feilipu]

Glad you wrote this up here. It is a very subtle bug. Congratulations on identifying and capturing it.


I’m (we’re) going through the whole development kit now to find other cases where this could happen. Hopefully it will improve reliability for RTOS operations in particular.

[Timmy]

Thank you for discovering, that sounds like a very nasty bug.

I had to re-read several times until this sinks in, it's very unintuitive.

I personally never do that kind of thing myself, but now I will also try to avoid it in the future. Thanks!

[vmorilla]

It came with a lot of headaches, indeed. But it was so satisfying finding it and getting things running stable!

[stefano]

>Does it really make a difference to save 1 byte in the stack?

I suppose it's a matter of design choices. The flexible and accurate parameter passing is an important feature they decided to have the comparison with sccz80 was initially a bit embarrassing, the char types we had requires some correction, especially the signed type and the conversion/comparison functions.

To me sdcc seemed excessively slow and I admit I asked myself "does it really make a difference?" but I'm the first fan of code optimization. .

well spotted, though and love the graphics !

[feilipu]

I wonder if there are similar bugs in the code...

Yes. There were quite a few examples in the RC2014 target diskio code bindings for sdcc. My bad (again). Anyway I’m pretty sure that they’re all managed now. Surprisingly the latent issues were not actually causing problems. But I’m much happier with the revised solutions, written with 4 years more experience.

Does it really make a difference to save 1 byte in the stack?

It is worth mentioning that sdcc is a general purpose small device compiler, and the 16-bit stack access vs 8-bit cpu is an 8080 (and equivalent) quirk. So it makes sense to do stack access as needed for the variable size, and let the code generation for the various platforms deal with what that generates. As Stefano says, a reasonable choice.


I’m looking forward to reading your project repository, and loading it into my ZX Next to play too.

[vmorilla]

I’m looking forward to reading your project repository, and loading it into my ZX Next to play too.

Thank you!