Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crashes due to stack overflow when painting a large area in tile map #76548

Merged

Conversation

komugi1211s
Copy link
Contributor

Fixes #76473.

This PR takes alloca out of the loop, reuses previous allocations whenever possible to reduce the chance of using up too much stack space.

Cause
the crash described in linked issue is most likely (almost certain) caused by the stack overflow.

filling a large area of tiles at once will create a long list of operations for Undo/Redo to record, which gets processed by the for loop inside UndoRedo::_process_operation_list function, which uses alloca inside. memory obtained from alloca gets freed after you leave the function scope.
given long enough list and right condition (which can be achieved by using a CTRL+SHIFT fill on tile map), alloca can exhaust the stack and cause a stack overflow, resulting in crash.

Valgrind report
Screenshot from 2023-04-28 22-49-01

each call allocates around 40 bytes (8 bytes for pointer * 5 arguments), resulting in roughly 6.6 MB before the crash
Screenshot from 2023-04-28 22-53-48

@komugi1211s komugi1211s requested a review from a team as a code owner April 28, 2023 14:34
@Chaosus Chaosus added this to the 4.1 milestone Apr 28, 2023
@bitsawer
Copy link
Member

I wonder if it would be acceptable to just use LocalVector here with a preallocation call to .resize() (or .reserve()) instead of alloca()? Would result in simpler and more solid code at the expense of one memory allocation+deallocation.

@komugi1211s
Copy link
Contributor Author

@bitsawer sorry for the late reply -- I think that's a good idea. I kept alloca simply because that is what this code had, replacing with LocalVector makes much more sense.
The performance difference was indistinguishable from one another on my machine as well.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (rebased against master 33957ae) on Linux, it works.

Please squash the commits together so we can merge this. See PR workflow for instructions 🙂

… once crashes

the engine due to segmentation fault.
@komugi1211s komugi1211s force-pushed the tilemap-ctrl-shift-edit-crash-fix branch from 6e4bc6b to 09fa220 Compare June 18, 2023 13:32
@akien-mga akien-mga merged commit fc42065 into godotengine:master Jun 18, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash while CTRL+SHIFT tilemap on viewport
5 participants