Stack Overflow on Win64

[pratikone]

This code SDL_SetWindowIcon(m_window, icon); in Screen::Screen() line 65 causes stack overflow in SDL library
Exception thrown at 0x00007FFCEFA0AF88 (SDL2.dll) in VVVVVV.exe: 0xC00000FD: Stack overflow (parameters: 0x0000000000000001, 0x000000D4F4613000).

Unhandled exception at 0x00007FFCEFA0AF88 (SDL2.dll) in VVVVVV.exe: 0xC00000FD: Stack overflow (parameters: 0x0000000000000001, 0x000000D4F4613000).

Commenting out the icon code makes the error go away and game launches perfectly fine. albeit, without icon on the window

Specs : Win 10 64-bit, running x64 Debug version of the game in Visual Studio with 64-bit SDL libraries provided by vcpkg

[0x08088405]

Could be related to how main apparently uses over 16kB of stack...
C6262: Function uses '17264' bytes of stack: exceeds /analyze:stacksize '16384'. Consider moving some data to heap.

[flibitijibibo]

I suppose this only happens with Win64? I don’t see this with Win32, Linux, or macOS.

[flibitijibibo]

You may have to add something like this to the Win32 portion of the CMake exe line:

set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /STACK:???")

Not sure what a good value would be.

[JayFoxRox]

The game does appear to use a surprising amount of stack. It's something that should be looked at.

Currently, the main() function keeps most game and asset related objects on the stack (which I assume to be pretty large in size), namely this block in main():

VVVVVV/desktop_version/src/main.cpp

Lines 94 to 103 in 8826f57

	UtilityClass help;
	// Load Ini
	Graphics graphics;
	musicclass music;
	Game game;

I assume this was done because these classes depend on some libraries already being initialized.

Some other game objects are in the data section (outside of any function):

VVVVVV/desktop_version/src/main.cpp

Lines 32 to 35 in 8826f57

	scriptclass script;
	edentities edentity[3000];
	editorclass ed;

[TijmenUU]

The game does appear to use a surprising amount of stack. It's something that should be looked at.

Currently, the main() function keeps most game and asset related objects on the stack (which I assume to be pretty large in size), namely this block in main():

VVVVVV/desktop_version/src/main.cpp

Lines 94 to 103 in 8826f57

	UtilityClass help;
	// Load Ini
	Graphics graphics;
	musicclass music;
	Game game;

I assume this was done because these classes depend on some libraries already being initialized.

Some other game objects are in the data section (outside of any function):

VVVVVV/desktop_version/src/main.cpp

Lines 32 to 35 in 8826f57

	scriptclass script;
	edentities edentity[3000];
	editorclass ed;

It's probably the array allocations you speak of later as there are no big stack allocations in the Graphics or musicclass as far as I can see. The large size objects are stored into std::vectors. Likewise for the strings std::string is used.

Perhaps the incredible amount of member fields used in these classes eventually add up too...

It could probably help getting rid of these array allocations and simply create an std::vector<T> of the desired size or just malloc it instead if you really want your raw pointers.

[flibitijibibo]

I'd be up for one of two paths:

• Get all the core classes allocated with new instead of just having them on the stack. This is way easier said than done, as the game uses those classes absolutely everywhere, and they're passed by reference so you can't just fix the .->-> in one function and call it a day. This is by far the most work you can do here, and doing it for individual members of the classes would be even worse.
• Increase the stack size on Windows at link time. This is what I'm leaning towards, since every other platform I'm aware of has a MUCH bigger default stack size (except for one secret platform that can also fix this trivially), so doing a whole bunch of potentially breaking work for Win64 and only Win64 just seems like a bad idea.

[TijmenUU]

I'd be up for one of two paths:

* Get all the core classes allocated with `new` instead of just having them on the stack. This is _way_ easier said than done, as the game uses those classes absolutely _everywhere_, and they're passed by reference so you can't just fix the `.`->`->` in one function and call it a day. This is by far the most work you can do here, and doing it for individual members of the classes would be even worse.

* Increase the stack size on Windows at link time. This is what I'm leaning towards, since every other platform I'm aware of has a MUCH bigger default stack size (except for one secret platform that can also fix this trivially), so doing a whole bunch of potentially breaking work for Win64 and _only_ Win64 just seems like a bad idea.

I'd say the latter is definitely the better option then, given that vvvvvv isn't going to get major additions to it in the future?

[flibitijibibo]

Right, it's basically just critical bugfixes and very minor accessibility features from here on out. (Maybe making controller support a little better too?)

[parkerlreed]

@flibitijibibo Yeah. Submitted an issue for that. #82

Including the built in SDL2 gamepad databaase and updating the button defs in Input.cpp may be all that's needed for updated support. I've been trying my hand locally but no luck as of yet.

[flibitijibibo]

Just pushed a patch that moves the memory around quite a bit, does this still happen?

[dekrain]

• Get all the core classes allocated with new instead of just having them on the stack. This is way easier said than done, as the game uses those classes absolutely everywhere, and they're passed by reference so you can't just fix the .->-> in one function and call it a day. This is by far the most work you can do here, and doing it for individual members of the classes would be even worse.

You can use std::unique_ptr instead of raw pointers and, since every function requires a reference, just pass the reference func(*obj_ptr) or even make a handle reference T& obj_ref = *obj_ptr (obj_ptr being std::unique_ptr<T> or T*)

[InfoTeddy]

Flibit's comments are a bit outdated now. #129 made it so that the core classes are now allocated outside of any function, which should mean they're no longer on the stack. Furthermore, #188 got rid of all the reference passing entirely.

[InfoTeddy]

All that's being done now is waiting for the original author (or anyone who has had this problem happen to them before) to report back and say "Yes, this is fixed now" or "No, it's still a problem".

[0x08088405]

You can use std::unique_ptr instead of raw pointers

The target for this repo is pre C++11 so you can't, no

[InfoTeddy]

Honestly, at this point the issue should be more-or-less fixed. I'd check for myself, but getting a compiling environment set up on Windows is more annoying than it is on Linux, so I don't have the patience for it right now. (Also, I'd need to compile an initial commit build, check that it stack overflows, and if it doesn't figure out why, and then compile a latest commit build with the same settings. I predict I'll have trouble with the "get the initial commit to stack overflow" part in the first place.)

I'd be very surprised if this was still a problem.

[flibitijibibo]

Pretty much all the large variables are off the stack now - if someone on Win64 finds an overflow they can reopen this, otherwise I think this is covered.