Surge crashes when trying to use the "Blue Whale Organ" patch #6702
Comments
|
Oh wow well that means we have a clamp backwards but it also means you have a debug build for the asset to fail no? |
|
Oh and if you run in a debugger do you get a line number? Thanks! |
|
There are two sides to this. #0 __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0)
at pthread_kill.c:44
#1 0x00007ffff77a66b3 in __pthread_kill_internal (signo=6, threadid=<optimized out>) at pthread_kill.c:78
#2 0x00007ffff7756958 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3 0x00007ffff774053d in __GI_abort () at abort.c:79
#4 0x00007ffff7ae1112 in std::__glibcxx_assert_fail (
file=file@entry=0x1197f88 "/usr/include/c++/12.2.0/bits/stl_algo.h", line=line@entry=3623,
function=function@entry=0x119c988 "constexpr const _Tp& std::clamp(const _Tp&, const _Tp&, const _Tp&) [with _Tp = int]", condition=condition@entry=0x1197f1a "!(__hi < __lo)")
at /usr/src/debug/gcc/libstdc++-v3/src/c++11/debug.cc:60
#5 0x0000000000dca8f0 in std::clamp<int> (__hi=<optimized out>, __lo=<optimized out>,
__val=<optimized out>) at /usr/include/c++/12.2.0/bits/stl_algo.h:3623
#6 limit_range<int> (high=<optimized out>, low=<optimized out>, x=<optimized out>)
at /usr/src/debug/surge-xt/src/common/dsp/vembertech/basic_dsp.h:7
#7 WavetableOscillator::init (this=0x7ffff65179c0, pitch=<optimized out>, is_display=false,
nonzero_init_drift=<optimized out>)
at /usr/src/debug/surge-xt/src/common/dsp/oscillators/WavetableOscillator.cpp:80
#8 0x0000000000c2a036 in SurgeVoice::switch_toggled (this=this@entry=0x7ffff6504120)
at /usr/src/debug/surge-xt/src/common/dsp/SurgeVoice.cpp:484
#9 0x0000000000c2d37c in SurgeVoice::SurgeVoice (this=this@entry=0x7ffff6504120,
storage=storage@entry=0x7ffff643d640, oscene=<optimized out>, params=params@entry=0x7ffff63cfa68,
key=key@entry=65, velocity=velocity@entry=60, channel=<optimized out>, scene_id=<optimized out>,
detune=detune@entry=0, keyState=<optimized out>, mainChannelState=<optimized out>,And what makes it crash in that patch is the wavetable in osc3. On the other hand I can't reproduce the crash with a self-compiled surge and gcc 12.2.0 |
|
Oh right so a table with one frame will be inverted in that clamp. We can fix that with a min max of course ratter than a clamp but still curios why the assert crashes |
|
Thanks for pinging me, I’ll have a look at buildinfo. It may not crash for you due to our distribution flags (LDFLAGS, CFLAGS etc) being quite strict. |
|
Yeah it certainly seems linux distributions exist to randomly change compile settings and still expect things to work :) is it possible to share those? Or upstream the information? “Your program crashes with an asset we activate when compiled using some secret flags” is a bit of a support pain. Anyway the line of code in question will invert clamp for size 1 wavetables. And the inversion is definite ub so we should fix it. |
In recent versions we allowed wavetables of length one but when doing so missed this clamp which resulted in hi < lo. that is UB (but defacto clamped to low) but that inversion triggers an assert on some configurations. So (1) add a debug assert to our limit range and (2) use a max of 0 on the upper bound. Closes surge-synthesizer#6702
|
Anyway the diff in #6703 adds a debug assert and fixes the problem I'm really still not sure why your production build is triggering a debug assert in clamp. That sounds like something is wrong in your build and it makes me worry you may have non-release flags on. But in this case it helped us catch a bug which we've fixed. Strongly suggest reviewing your build instruction set to make sure it is not activating debug asserts improperly. |
In recent versions we allowed wavetables of length one but when doing so missed this clamp which resulted in hi < lo. that is UB (but defacto clamped to low) but that inversion triggers an assert on some configurations. So (1) add a debug assert to our limit range and (2) use a max of 0 on the upper bound. Closes #6702
In recent versions we allowed wavetables of length one but when doing so missed this clamp which resulted in hi < lo. that is UB (but defacto clamped to low) but that inversion triggers an assert on some configurations. So (1) add a debug assert to our limit range and (2) use a max of 0 on the upper bound. Closes surge-synthesizer#6702
I got the same assert on the flatpak build because the default CXXFLAGS have This is the first line of defense against known UB with the C++ library. It has already caught a few other UB (in various projects) like accessing item 0 in an empty container. |
|
I understand why assertions are useful; but they are also expensive! I wonder Triggering them in release builds has performance impacts. I kinda assume that std clamp goes directly to inline assembly and I don’t see how it can if it has this assertion right? Or am I mis understanding the feature |
|
I was just giving here how I got hit by this bug, and why said define is set. It's not a broken config. Answering this:
Albeit I am not the OP, so I can't really speak for Arch. |
|
Gotcha well it’s a choice I didn’t expect for sure! Thanks for helping understand where it comes from but we also were able to fix it which is even better! |
|
Huh a quick google search also leaves the question of the performance impact here uncertain. Seems some checks can be optimized out but others not but they are only super lightweight (their words not mine) ones according to some of the other threads I found where people try to see if it is ok to have this on in production code the thing that was clear is that security sensitive code benefits from these checks - especially the bounds checks - and so a few other threads weee of the form “this isn’tsensitive” and the response “everything is sensitive” how interesting. Thanks for chiming in |
|
I came to this issue from looking at the code and (I would have submitted a patch otherwise). |
|
Ahh gotcha didn’t this get swept into 1.1.2 or some such? I guess not! Cool glad we found it |
|
Just to wrap this up the glibc assert adds an extra compare and jump to every clamp taking it from 3 instructions without a branch to 5 with a branch and adding the branch points. |
Bug Description:
When I choose "Blue Whale Organ" patch Surge just crash.
Surge XT Version
Surge XT 1.1.2 - Arch Build (Surge XT 1.1.makepkg.d3568a9ae)
Reproduction Steps:
Steps to reproduce the behavior:
Expected Behavior:
Should have played the sound
Computer Information:
Additional Information:
If I run Surge in a terminal, I get this error.
I use Pipewire.
There is no such crash in Windows.
Yes, the crash occurs when choosing some more patches, but I don’t remember which ones.
The text was updated successfully, but these errors were encountered: