-
-
Notifications
You must be signed in to change notification settings - Fork 23.7k
GDScript call stack as reverse linked list with fixed coroutines #106790
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
Conversation
|
Cool stuff! I spent days trying to find some fix for that issue with the linked list approach in #91006, with various invocations of I can't say I understand all the ins-and-outs of this alternative approach with the coroutine signal handover, so I'll leave that for the GDScript people to review, but seeing as how all the unit tests pass and whatnot, this looks promising. You'll probably want to change the documentation of the godot/doc/classes/ProjectSettings.xml Lines 652 to 656 in 45fc515
EDIT: I just realized you removed the |
Oh, I probably removed that check by accident, as I was thinking of removing the option altogether, but it might be too controversial a change. I'm not sure how open the team is to enabling script traces for all builds by default. UPDATE: |
db41873 to
53caa04
Compare
|
Just realized, since |
53caa04 to
9327cbf
Compare
|
I found an issue with this implementation. Since I removed UPDATE: Done. |
9327cbf to
db9b9b6
Compare
vnen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really familiar with the call stack code and the coroutine code is quite complex so it's always hard to review. But the code does look okay to me.
We probably need to add proper test for coroutines to guarantee they work as expected, but this is a topic for another day.
modules/gdscript/gdscript.h
Outdated
|
|
||
| if (unlikely(_call_stack.stack_pos >= _debug_max_call_stack)) { | ||
| if (unlikely(_call_stack_size >= _debug_max_call_stack)) { | ||
| // stack overflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to avoid comments like this. The error message is already saying "Stack overflow", so this is very redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense – I removed the comment.
db9b9b6 to
0d2a6c3
Compare
* GDScript call stack as reverse linked list with issues fixed (originally proposed in 91006). * Fix coroutine issues with call stack by resuming async call chain inside `GDScriptFunction::call()`. * This fixes corrupted line numbers for coroutines in the debugger and backtrace (106489). Co-authored-by: Juan Linietsky <[email protected]>
0d2a6c3 to
a095c5e
Compare
|
Thanks! |
Loggerand ability to print and log script backtraces #91006).GDScriptFunction::call().Note: The issue with the original implementation was that it was crashing in coroutine tests -- it's resolved here.
Merging this will help Sentry SDK to display accurate stack traces to users.
Coroutine issue explanation
Why it happens
call():linevariable is local, andenter_function()is passed a reference to this variable.enter_function()initializesCallLevelwith a pointer to thelinevariable (which is scoped tocall()).linevariable is continuously updated during GDScript function execution using OPCODE_LINE operation.call()is exited, andlineis popped off the stack, but theCallLevelinstance still lives on due to specifics of the resumed function execution (exit_function()isn't called yet).CallLevelframes overstayed the lifetime of theirlinevariables.Conclusion: Because
CallLevelin GDScript call stack outlivescall()'s scope for resumed coroutines,CallLevel*andint *linepointers can sometimes point to heaven-knows-what.Note: This PR ensures that function-state execution remains within the
call()scope, and doesn't cause issues withCallLevelbeing freed before it's removed from the GDScript call stack.Explaining on example
Consider this code:
Output:
Notice that the backtrace reports crazy line numbers, and how
call(f1)already exited, but the associated CallLevel frame lingers slightly longer in the end (exit_function(f1)). This only happens for resumed functions.