Clean fallback from memory management errors

[jarkkojs]

Problem

Currently we need to use _aligned_alloc() together with the new-operator in order to allocate 16-byte aligned memory for use with the SSE2 instructions. For Linux and macOS this function is substituted to malloc(), which is always gives 16-byte aligned on these platforms.

It'd be better to use the new-operator for all allocations because this will cause std::bad_alloc to be thrown when allocations fail, which means that we always get a clean exit instead of
crashing with a null pointer dereference.

Solution

C++17 provides language level support for allocations for over-aligned data [1].

Example program:

#include <iostream>
#include <iomanip>

class alignas(256) Foo {
    unsigned long data;
};

int main()
{
    Foo *foo = new Foo();
    std::cout << std::internal << std::hex << std::setw(18) << std::setfill('0')
              << foo << std::endl;

    return 0;
}

If I compile with -faligned-new:

$ g++ -faligned-new -std=c++14 -o test test.cpp 

$ ./test 
0x0000557b8ff08f00

Compiler support:

• GCC: since 7.0 [2]. Distributions currently ship with 8.x.
• Clang: since 4 0 [3]. Isn't this quite old already?
• Visual Studio: since 15.5 [4] (with /Zc:alignedNew). Not sure how recent this is.

[1] https://www.bfilipek.com/2017/06/cpp17-details-clarifications.html#dynamic-memory-allocation-for-over-aligned-data
[2] https://www.gnu.org/software/gcc/projects/cxx-status.html
[3] https://clang.llvm.org/cxx_status.html
[4] https://docs.microsoft.com/en-us/cpp/build/reference/zc-alignednew?view=vs-2017

[baconpaul]

Is there any reasonable clean fallback we can take if this happens? So many os host conbos. Is it a legit concern?

[jarkkojs]

@baconpaul: I think it is important just to recognize the problem at this point. Not something that there is a "quick fix". Changing malloc() calls to use new-operator in the code base is a good start because at least the the behavior is consistent. Would not try rush with this.

[baconpaul]

Ooh malloc and new are different on purpose.

I guess my view is “if you are out of memory you will crash”. That’s not ok in the kernel but in Logic Pro i don’t think there’s anything smart a plug-in can do!

[jarkkojs]

Can you point me to a call site where the use of malloc() is mandatory? Not sure I did get the purpose.

[jarkkojs]

When you deal with memory errors, it will make you to do more well informed choices by other angels too when you write new code (like dealing with other types of errors). And it gives a good baseline to scale this project down towards smaller devices. And it is a bug class and I just don't see how it could be OK just let a bug class let be in the code base.

Easy to way start would be to get rid of mallocs. And maybe GUI could have a top level hander to show a dialog when std::bad_alloc is catched?

[baconpaul]

Not from my phone! But I haven’t read all the dsp code and there’s lots of stuff in there with memory and alignment which seems purposeful so I think a swap would require a very careful understanding of the code which I don’t have.

[baconpaul]

Also I never quite understood what to put in the block

If( ! Malloc )
// what to do here?

or similarly what to do in a std bad alloc handler which would improve the users experience reliably

But I’m not that bothered by it! Just trying to make sure we are super careful making changes

[jarkkojs]

@baconpaul, at least consistent behavior would be nice at this point i.e. have an exception thrown. Then the plugin always exists cleanly, never crashes. We would close this issue with that. I can make more localized proposals but that would be good step to take with its own set commits. Even using the default handler for bad_alloc is already a vast improvement even without any type of visual cue because you never actually crash.

There is nothing special in the DSP code that is connected to the use of malloc() . Check #171.

[baconpaul]

Ok cool! Appreciate the thoughtfulness. And happy new year.

[jarkkojs]

@baconpaul, happy new year to you too :)

[jarkkojs]

I will fix this and #171 with the same PR.

[jarkkojs]

When the fallback path is an exception and not a random crash, it will also improve the debugging experience. You can run gdb and do something like b 'std::bad_alloc::bad_alloc()'. Improbable but you will know when it happens, and you know what happened.

[jarkkojs]

@baconpaul, yeah, and I like this surprising amount of diversity among people who work on this :) Have to from time to time reconsider how I view some problems. Kind of good form of distraction.

[jarkkojs]

Almost untested WiP code https://github.com/jsakkine/surge/tree/malloc. This will be also pull branch for #171.

[baconpaul]

So it seems

_aligned_malloc( -, 16 )

Does something different than new. Namely it aligns on 16 byte boundaries which is required for sse.

I’m not an expert here but did I miss where we decided it was a noop change? On windows it seems it will change behavior but I could have missed the convo where we deciddd it didn’t

[jarkkojs]

Commented the other issue. Point is that using the new-operator already gives always clean exit whereas malloc() without error check causes random crash if it fails. With an overloaded new operator we could possibly even spawn an error dialog inside the new operator.

[jarkkojs]

@baconpaul, @kurasu: updated the description. Closing the other issue. Doesn't that look like a plan for this issue?

[jarkkojs]

@baconpaul: I think that you can take design required tag away. It does not solve everything related to mm but I think it is enough for the scope of one issue. After implementing that we have more clean platform to make more detailed choices on memory management.

[jarkkojs]

Visual Studio 15.5 is fairly old: https://docs.microsoft.com/en-us/visualstudio/releasenotes/vs2017-relnotes-v15.5. I think it should be safe to just take this feature into use with the compilation flags described above.

[baconpaul]

Removed that tag. When I put it on, design was required! Now it seems the design is in place; namely tag everything with alignas() and test. (The reason to test, of course, isn't to make sure it is aligned; it is to make sure we don't miss any classes getting the alignas() flag).

[jarkkojs]

Every possible Linux distribution uses at least GCC 7.3 because it is the oldest GCC version carrying retpoline support, which is a feature providing Spectre mitigation. If we ever need to do larger alignments in future than 16-bytes, we can then add -faligned-new for Linux and macOS targets without major concerns.

[jarkkojs]

Did a sanity check that this actually gets inherited to sub-classes:

#include <iostream>
#include <iomanip>

class alignas(256) Foo {
    unsigned long data;
};

class Bar : Foo {
};

int main()
{
    Foo *foo = new Foo();
    std::cout << std::internal << std::hex << std::setw(18) << std::setfill('0')
              << foo << std::endl;

    Bar *bar = new Bar();
    std::cout << std::internal << std::hex << std::setw(18) << std::setfill('0')
              << bar << std::endl;

    return 0;
}

When I run this:

$ ./test 
0x00007ff678c02c00
0x00007ff678c02e00

Looks good.

[jarkkojs]

Great, the approach works. Now it is only just matter of using the same pattern to the rest of the allocations.

[jarkkojs]

Extending a scope of this issue a bit. With user interaction functions we can implement an error message when running out of memory (catch std::bad_alloc and show the dialog).

[jarkkojs]

The remaining uses of aligned_malloc():

src/common/SurgeSynthesizer.cpp:      voices_array[0][i] = (SurgeVoice*)_aligned_malloc(sizeof(SurgeVoice), 16);
src/common/SurgeSynthesizer.cpp:      voices_array[1][i] = (SurgeVoice*)_aligned_malloc(sizeof(SurgeVoice), 16);
src/common/SurgeSynthesizer.cpp:       (QuadFilterChainState*)_aligned_malloc((MAX_VOICES >> 2) * sizeof(QuadFilterChainState), 16);
src/common/SurgeSynthesizer.cpp:       (QuadFilterChainState*)_aligned_malloc((MAX_VOICES >> 2) * sizeof(QuadFilterChainState), 16);
src/common/dsp/effect/PhaserEffect.cpp:      biquad[i] = (BiquadFilter*)_aligned_malloc(sizeof(BiquadFilter), 16);

For SurgeVoice, I have a pending PR #333. It works on every other DAW I've tried except both Windows and macOS versions of Bitwig Studio. @baconpaul has provided a screenshot showing the error.

@kurasu, we have absolutely zero idea what could be the cause and effect on this one. Since you know Bitwig internals better than us, does anything come to mind that could somehow connect to this?

[jarkkojs]

We can close this after doing this also in "init" of VST3 and AU:

   try {
      std::unique_ptr<SurgeSynthesizer> synthTmp(new SurgeSynthesizer(this));
      synthTmp->setSamplerate(this->getSampleRate());

      std::unique_ptr<SurgeGUIEditor> editorTmp(new SurgeGUIEditor(this, synthTmp.get()));

      _instance = synthTmp.release();
      editor = editorTmp.release();
   } catch (std::bad_alloc err) {
      Surge::UserInteractions::promptError(err.what(), "Out of memory");
      state = DEAD;
      return false;
   } catch (Surge::Error err) {
      Surge::UserInteractions::promptError(err);
      state = DEAD;
      return false;
   }

[jarkkojs]

I could work on equivalent infrastructure for VST3 that I've done for VST2 after I've finished with the Linux resource manager.

[jarkkojs]

I think it is good enough for the time being now. We can do smaller scale issues when we actually want to start doing something for some useful reasons.