FreeBoy - Division by zero

[zonkmachine]

Division by zero. Debugging with the fpe flag -DCMAKE_BUILD_TYPE=Debug -DWANT_DEBUG_FPE=True gives a division by zero error in FreeBoy.

This is on stable-1.2 and master as of 7808e0b. It's probably introduced with the original FreeBoy(papu) commit.

Logs

Click to expand

Thread 13 "Mixer::fifoWrit" received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7fffbd5bc700 (LWP 93992)]
0x00007fffbcd69f73 in FreeBoyInstrument::playNote (this=0x7ffff2bb0060, _n=0x7ffff2ae0e40, _working_buffer=0x7fffa2ca0860)
    at /home/zonkmachine/builds/lmms/plugins/FreeBoy/FreeBoy.cpp:374
374				f = 524288.0 / ( r * pow( 2.0, s + 1.0 ) );
(gdb) bt full
#0  0x00007fffbcd69f73 in FreeBoyInstrument::playNote(NotePlayHandle*, std::array*)
    (this=0x7ffff2bb0060, _n=0x7ffff2ae0e40, _working_buffer=0x7fffa2ca0860) at /home/zonkmachine/builds/lmms/plugins/FreeBoy/FreeBoy.cpp:374
        r = 0 '\000'
        s = 0 '\000'
        sopt = 0 '\000'
        ropt = 1 '\001'
        fopt = 262144
        f = 0
        tfp = 0
        samplerate = 44100
        frames = 255
        offset = 1

lmms/plugins/FreeBoy/FreeBoy.cpp

Lines 370 to 377 in 7808e0b

	float fopt = 524288.0 / ( ropt * pow( 2.0, sopt + 1.0 ) );
	float f;
	for ( char s=0; s<16; s++ )
	for ( char r=0; r<8; r++ ) {
	f = 524288.0 / ( r * pow( 2.0, s + 1.0 ) );
	if( fabs( freq-fopt ) > fabs( freq-f ) ) {
	fopt = f;
	ropt = r;

We set 'r' to 0 in the second for loop and this gives f = 524288.0 / 0on line 374.

Something like this for line 374 could work:
f = 524288.0 / std::max( r * pow( 2.0, s + 1.0 ), 0.000001 );

It plays back similar on this demo project.
freeboy.mmp.zip

[zonkmachine]

It's possible to turn the floating point exceptions off temporarily.

#include <fenv.h>

...

fedisableexcept( FE_DIVBYZERO );
f = 524288.0 / ( r * pow( 2.0, s + 1.0 ) );
feenableexcept( FE_DIVBYZERO );

I'm not sure about this case though but I'm sure there are a lot of places where things like division with zero doesn't matter. For instance when you already test for infinites and discard them.

[PhysSong]

r = 0 doesn't make sense at all, so you can safely change the range of r to exclude 0.

[DomClark]

Here's where the values are used in the game-music-emu code: https://bitbucket.org/mpyne/game-music-emu/src/013d4676c689dc49f363f99dcfb8b88f22278236/gme/Gb_Oscs.cpp#lines-194

static unsigned char const table [8] = { 8, 16, 32, 48, 64, 80, 96, 112 };
int period = table [regs [3] & 7] << (regs [3] >> 4);

regs [3] & 7 is r and regs [3] >> 4 is s.
The table is just multiples of 16, apart from the first entry which is 8, not 0. Thus when r = 0, the value 0.5 should be used in the calculation instead.

[PhysSong]

@DomClark Nice catch! I think we can even more optimize calcuating f as well, without using many floating-point operations.

[PhysSong]

Also, (r, s) = (0, n) is equivalent to (r, s) = (1, n - 1), so we don't have to check for r = 0 unless s = 0.

[aidan-mueller]

@PhysSong Just noticed you self-assigned this. In case you haven't seen it yet, PR #6053 fixes this issue.