Yes, this has been a problem with debug builds in the past with other functions... interestingly, VS2013 doesn't have that. Ideally this should be fixed by redoing the way the text window works.

@jmkinzer, thank you, I have the same issue but not actually try to understand why it happens.

Got the same issue.
Going to try that fix.

This allowed me to compile in VS2017-- will this be added to master?

Fixed in master.

@whitequark
Let me resurrect this ancient one. I think I know why the stack overflow was (is) happening on Windows. Using the code analysis tool in MSVC 2019 I get the following warnings:

.\src\solvespace.h(813): warning C6262: Function uses '5614392' bytes of stack:  exceeds /analyze:stacksize '16384'.. This allocation was for a compiler-generated temporary for 'class SolveSpace::TextWindow' at line 813.  Consider moving some data to heap.
.\src\solvespace.h(814): warning C6262: Function uses '21016632' bytes of stack:  exceeds /analyze:stacksize '16384'.. This allocation was for a compiler-generated temporary for 'class SolveSpace::System' at line 814.  Consider moving some data to heap.

The lines in question are these:
https://github.com/solvespace/solvespace/blob/master/src/solvespace.h#L813

So the analyser thinks that this constructor:

    SolveSpaceUI()
        : pTW(new TextWindow({})), TW(*pTW),
          pSys(new System({})), sys(*pSys) {}

somehow temporarily puts a copy of both TextWindow and System on the stack or about 26MB - not nice. I'll try to debug this on assebler level to confirm that this is indeed the case.

I know that this is/was the case, no need to confirm anything. Fixing this properly requires rewriting the text window completely to use a less primitive rendering approach. This is complicated: it really ought to use Freetype and sprite atlases to handle CJK with good resolution as well as to allow changing text window size. So this isn't done yet.

I'm not really interested in replacing the current (poor) solution that commits 26 MB of stack with another hack.

Hmmm... I think the problem is that new TextWindow({}) in addition to allocating memory on the heap for the instance also creates a zero initialized TextWindow object on the stack (because of {}) and passes it to the constructor. new System({}) does the same. If we remove the {} the stack usage drops dramatically. See my pull request #533.

Solvespace runs. The only remaining question is whether omitting the {} is actually bad. I suspect it might be, because (if I understand the C++ initialization semantics correctly) the two instances end up uninitialized ("random" garbage from the heap). If we can find a way to assign to the TW and sys references zero initialized, heap allocated TextWindow and System objects without the current initialization syntax, we would be fine.
For example pull pTW and pSys out of the SolveSpaceUI class, "memset" them to 0 and then change the SolveSpaceUI constructor to something like this:

SolveSpaceUI() : TW(*pTW), sys(*pSys) {}

But it is not "pretty" from "architecture" point of view.

But it is not "pretty" from "architecture" point of view.

There is nothing "not pretty" about memsetting them. It is simply wrong (invokes UB), which is why it is not done in the first place. TextWindow contains, right now, non-POD objects like std::shared_ptr, which may not be memset. System does not but it should be possible to use them there.

(If you're observant, you might have noticed that TextWindow::ClearSuper() does exactly that. Which is a bug that needs fixing.)

Thanks for making me think harder (I'm a C guy by day :-) ). And sorry for the "noise" I'm making.

So omitting the {} will cause all POD members of the two instances to contain garbage from the heap, while all "proper objects" (e.g. std::*) will be OK because of their constructors. On the other hand we can not memset the instances, because then only the PODs will be properly set to 0 while the "proper classes"... will? ... be filled with zeroes and that may not be OK?

In this case what is the proper way to initialize these two instances without using {} and wasting a bunch of stack? Would it be to write proper constructors that set all the members to "sane" default values?

Would it be to write proper constructors that set all the members to "sane" default values?

Here are the exact rules 1 2 3 for our case. They cause no shortage of bugs, and are a good example of why C++ should not be used for new projects. In practice, the worst issues result from the fact that once you add a base class, in C++11, doing new T{} stops performing aggregate-initialization (which does initialize POD members) and starts performing default-initialization (which does not).

In practice adding default member initializers (class { int x = 0; }) would fix this, but it is ugly for other reasons. We should probably systematically add default member initializers to all classes that may eventually become non-POD (not everywhere, since zero-initializing an array of Vectors is something we IIRC profiled and rejected as too expensive), but not just as a workaround for this bug.

Thanks! I read them all, plus these:
https://en.cppreference.com/w/cpp/language/aggregate_initialization
https://en.cppreference.com/w/cpp/language/initializer_list
and - after looking at the definitions of TextWindow and System - came to the conclusion that we should be OK. But since it is a lot of information to parse I decided to debug it as well. As far as I can see everything is properly initialized with #533 in place. And 2519 frames are worth a few hundred words ;-) https://youtu.be/oZcwAU8KcuU
So I think you should consider merging it.

came to the conclusion that we should be OK

Why? I don't see what guarantees that System is initialized at all.

From 1:

"2) if T is a class type with a default constructor that is neither user-provided nor deleted (that is, it may be a class with an implicitly-defined or defaulted default constructor), the object is zero-initialized and then it is default-initialized if it has a non-trivial default constructor; "

But all 5 links we discussed make it hard to be sure - that is why I debugged it, and at least VS2019 does call memset on it (as seen in the video), which is no proof, just another data poit :-)

@ruevs Hm, OK. Then you also should take out the /STACK linker parameter (IIRC it's in two places) in your PR.

Done. Found it only here:
https://github.com/solvespace/solvespace/blob/master/src/CMakeLists.txt#L343

@whitequark are you waiting for something else from me?

@ruevs Sorry, I missed this comment somehow. Which PR should I merge?

This one:
#533
:-)