Geonkick-Synthesizer / geonkick

Geonkick is a free software synthesizer capable of generating a wide range of percussive sounds, including kicks, snares, claps, hi-hats, shakers, and also unique effect sounds.
https://geonkick.org
GNU General Public License v3.0
105 stars 7 forks source link

Fix UBSan errors #59

Closed treapster closed 7 months ago

treapster commented 7 months ago

Since we now found three bugs in geonkick with the help of debug flags and llvm toolchain, i decided to build geonkick with clang and -fsanitize=undefined to see what else may come up. Here are the errors:

/home/me/geonkick/src/geonkick_api.cpp:714:40: runtime error: load of value 3183113248, which is not a valid value for type 'enum gkick_envelope_apply_type'
/home/me/geonkick/src/compressor_group_box.cpp:159:34: runtime error: -inf is outside the range of representable values of type 'int'
/home/me/geonkick/src/kit_model.cpp:167:19: runtime error: -inf is outside the range of representable values of type 'int'
/usr/include/rapidjson/internal/stack.h:117:13: runtime error: applying non-zero offset 16 to null pointer
/usr/include/rapidjson/internal/stack.h:117:13: runtime error: applying non-zero offset 1 to null pointer

Ignoring rapidjson issue(it is known and fixed but not released yet), there are 3 errors:

treapster commented 7 months ago

Some additional info: when trying to catch and log the third problem, i used the following:

        // in KitModel::percussionLeveler
        double logVal = 20 * log10(realVal);
        if (isnan(logVal) || isinf(logVal)) {
            /* log realVal */
        }

And i never got the log output despite UBSan printing error about NaN/inf! In the doc, i found the following:

GCC and Clang support a -ffinite-math option (additionally implied by -ffast-math), which allows the respective compiler to assume the nonexistence of special IEEE-754 floating point values such as NaN, infinity, or negative zero. In other words, std::isnan is assumed to always return false under this option.

-ffast-math is indeed set in the CmakeLists, so the only way to catch it is to set conditional breakpoint if realVal < 0.0. I was also surprised that even in debug mode all the same optimizations are applied as in release, the difference is only in logging. I also found a couple of places where isnan is used currently - with current build config it simply does not work. With -ffast-math we can not* detect nan, but can only prevent it by e.g making sure the value is not negative and doing log(x + 0.000001) instead of log(x).

* if we really want, we can use bit_cast to integer and compare with integer representation of nan, and it should work even with -ffast-math, but there are no library funcitons for it.

treapster commented 7 months ago

We can use -funsafe-math-optimizations -fno-math-errno -fno-trapping-math instead of -ffast-math to get almost all the same optimizations but still be able to detect nans. I tried and it works with both clang and gcc. More info.

In the meantime i figured out that leveler is apparently the level indicator which goes up and down, in which case it makes sense to update it dynamically, and the fix is to just use the absolute sample value. I'll include it in the PR, as well as ffast-math fix, so you only have to check whether i got it right:)

iurienistor commented 7 months ago

Since we now found three bugs in geonkick with the help of debug flags and llvm toolchain, i decided to build geonkick with clang and -fsanitize=undefined to see what else may come up. Here are the errors:

/home/me/geonkick/src/geonkick_api.cpp:714:40: runtime error: load of value 3183113248, which is not a valid value for type 'enum gkick_envelope_apply_type'
/home/me/geonkick/src/compressor_group_box.cpp:159:34: runtime error: -inf is outside the range of representable values of type 'int'
/home/me/geonkick/src/kit_model.cpp:167:19: runtime error: -inf is outside the range of representable values of type 'int'
/usr/include/rapidjson/internal/stack.h:117:13: runtime error: applying non-zero offset 16 to null pointer
/usr/include/rapidjson/internal/stack.h:117:13: runtime error: applying non-zero offset 1 to null pointer

Ignoring rapidjson issue(it is known and fixed but not released yet), there are 3 errors:

* [synth_kick_env_get_apply_type](https://github.com/Geonkick-Synthesizer/geonkick/blob/1d20f0069c4bbd7213609a355a727af3172d9b20/src/dsp/src/synthesizer.c#L1086) was sometimes called with envelope type different from filter cutoff envelope, which resulted in uninitialized apply_type [here](https://github.com/Geonkick-Synthesizer/geonkick/blob/1d20f0069c4bbd7213609a355a727af3172d9b20/src/geonkick_api.cpp#L710) and everywhere it was used. Fixed in this PR.

* an int argument [passed](https://github.com/Geonkick-Synthesizer/geonkick/blob/1d20f0069c4bbd7213609a355a727af3172d9b20/src/compressor_group_box.cpp#L159) to attackSlider->onSetValue was derived from log10(0), which is -inf. Fixed in this PR.

* and now the hard one which i could not easily fix: the leveler value used [here](https://github.com/Geonkick-Synthesizer/geonkick/blob/1d20f0069c4bbd7213609a355a727af3172d9b20/src/kit_model.cpp#L165) can sometimes be not just zero, but significantly below zero(-0.16,- 0.2 and similar values were observed in gdb), resulting in NaN when passed to log10. I found out that it comes from [here](https://github.com/Geonkick-Synthesizer/geonkick/blob/1d20f0069c4bbd7213609a355a727af3172d9b20/src/dsp/src/mixer.c#L114), and as i understand the leveler is set from current sample value multiplied by limiter, which does not make sense to me, because apparently it is supposed to be non-negative(it's passed to log10!), and i don't exactly know what leveler means in that context(in my understanding the leveler is something used per channel, not per sample). So, i'm leaving it up to you to figure out what's happening and what to do with that:)

The absolut value of the sample passed to the leveler is ok.

treapster commented 7 months ago

Force-pushed to fix "leverer" error in commit message, now should be good to go.