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

Ability to print and log script backtraces #91006

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reduz
Copy link
Member

@reduz reduz commented Apr 22, 2024

  • Changes how stack information is stored in GDScript to a reverse linked list.
  • This makes it fast enough to leave it enabled all time time on debug.
  • Added a new script function script_backtrace() to get a string with the current script backtrace.
  • Added the script backtrace to the logger. Errors now include the full currently running script backtraces.
  • Added script backtrace printing to console logs (Windows, MacOS, Linux) using blue color.
  • Additionally, added a custom Logger class to allow users intercept the internal messages/warnings/error (supersedes Add user-facing log interception callback. #87576).

How it looks:

using script_backtrace()

image

Results in the following output:

image

Logger

Backtraces are now added to the loggers. Here is how it looks for ANSI/Windows console loggers:

image

@realcoloride
Copy link

Extremely wanted feature that could make it way easier to debug. Love this

if (stack.size()) {
trace += "stack_language: " + sl->get_name();
for (int i = 0; i < stack.size(); i++) {
trace += "\n" + itos(i) + ": " + stack[i].func + " (" + stack[i].file + " : " + itos(stack[i].line) + ")";
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing as how the existing error/warning printing formats file-with-line as %s:%i (as seen in your own screenshot), rather than %s : %i, the spaces in " : " should probably be removed.

Suggested change
trace += "\n" + itos(i) + ": " + stack[i].func + " (" + stack[i].file + " : " + itos(stack[i].line) + ")";
trace += "\n" + itos(i) + ": " + stack[i].func + " (" + stack[i].file + ":" + itos(stack[i].line) + ")";

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense

@bruvzg
Copy link
Member

bruvzg commented Apr 22, 2024

Not sure how reliable it will be, but it might be worth calling it from the crash handler as well (tested on macOS only and seems to be working):

diff --git a/platform/macos/crash_handler_macos.mm b/platform/macos/crash_handler_macos.mm
index c370422bfa..91f76dc3b5 100644
--- a/platform/macos/crash_handler_macos.mm
+++ b/platform/macos/crash_handler_macos.mm
@@ -31,6 +31,7 @@
 #include "crash_handler_macos.h"
 
 #include "core/config/project_settings.h"
+#include "core/object/script_language.h"
 #include "core/os/os.h"
 #include "core/string/print_string.h"
 #include "core/version.h"
@@ -171,6 +172,9 @@ static void handle_crash(int sig) {
 	}
 	print_error("-- END OF BACKTRACE --");
 	print_error("================================================================");
+	print_error(ScriptServer::get_current_script_backtrace());
+	print_error("-- END OF SCRIPT BACKTRACE --");
+	print_error("================================================================");
 
 	// Abort to pass the error to the OS
 	abort();

@RedMser
Copy link
Contributor

RedMser commented Apr 22, 2024

How does the new function relate to the existing @GDScript.get_stack() and print_stack()? I assume there are technical differences, but these are not really made clear enough to the average user (when should I use this new function vs the existing ones?).

@farfalk
Copy link

farfalk commented Apr 22, 2024

In addition to what RedMser asked, how does this relate to #87576 ?

@@ -983,6 +983,8 @@ void UnixTerminalLogger::log_error(const char *p_function, const char *p_file, i
const char *magenta_bold = tty ? "\E[1;35m" : "";
const char *cyan = tty ? "\E[0;96m" : "";
const char *cyan_bold = tty ? "\E[1;36m" : "";
const char *blue = tty ? "\E[0;94m" : "";
//const char *blue_bold = tty ? "\E[1;34m" : "";
Copy link
Member

Choose a reason for hiding this comment

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

This can probably be removed, it's trivial to readd later if we need it:

Suggested change
//const char *blue_bold = tty ? "\E[1;34m" : "";

@reduz
Copy link
Member Author

reduz commented Apr 22, 2024

@RedMser

This new one works always, even if not running the debugger and it also is generic for all languages. print_stack() was created mostly for testing and should be deprecated and removed since its pretty useless.

@farfalk

Both PRs should work great together

@permelin
Copy link
Contributor

Did a quick test script with a function that is recursively called 500 times inside a for loop. To my surprise, this branch is 5% faster than master.

(Pushing the call stack beyond a depth of 506 or 507 will SIGSEGV, but that happens in master too.)

@dalexeev
Copy link
Member

Can script_backtrace() return data in a structured form (Array[Dictionary]), like get_stack()? It also looks like this PR will resolve godotengine/godot-proposals#105.

@reduz
Copy link
Member Author

reduz commented Apr 30, 2024

@dalexeev I honestly don't see a lot of use for this, since the main use case is to print it, but I think adding another function script_stack() may be good for this, that could also include more stack information if you want.

@reduz reduz force-pushed the live-backtrace branch 4 times, most recently from bb26553 to bcfa1bd Compare April 30, 2024 12:23
@reduz reduz requested a review from a team as a code owner April 30, 2024 12:23
static void _bind_methods();

public:
virtual void log_error(const char *p_function, const char *p_file, int p_line, const char *p_code, const char *p_rationale, bool p_editor_notify = false, ErrorType p_type = ERROR_TYPE_ERROR, const char *p_script_backtrace = nullptr);
Copy link
Contributor

@mihe mihe Apr 30, 2024

Choose a reason for hiding this comment

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

Not sure how big of a deal this is, but if someone uses auto-complete (or just copies the signature verbatim) to implement this virtual method in GDScript, they will trigger the "Shadowed Global Identifier" warning due to the script_backtrace parameter shadowing the global function by the same name.

Maybe renaming the script_backtrace function to get_script_backtrace is the better option?

String str;
str.parse_utf8(buf, len);
for (uint32_t i = 0; i < loggers.size(); i++) {
loggers[i]->log_message(str, p_err);
Copy link
Contributor

@mihe mihe Apr 30, 2024

Choose a reason for hiding this comment

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

Seeing as how _log_message will end up getting called on every print, you can fairly easily get yourself into a stack overflow by trying to print anything from _log_message.

Might be good to mention in the documentation at least.

EDIT: I would assume the same applies when trying to push_error from _log_error.

* Changes how stack information is stored in GDScript to a reverse linked list.
* This makes it fast enough to leave it enabled all time time on debug.
* Added a new script function script_backtrace() to get a string with the current script backtrace.
* Added the script backtrace to the logger.
* Added script backtrace to console logs (Windows, MacOS, Linux) using blue color.
* Added a custom Logger class to help log errors from script.
Comment on lines +582 to +583
GDScriptLanguage::CallLevel call_level;
GDScriptLanguage::get_singleton()->enter_function(&call_level, p_instance, this, stack, &ip, &line);
Copy link
Contributor

@mihe mihe May 6, 2024

Choose a reason for hiding this comment

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

This whole thing with keeping a pointer to this call_level here seems to cause some trouble with coroutines (functions that await), as they're allowed to exit this method without calling GDScriptLanguage::exit_function first (causing this pointer to go stale) since that gets called as part of GDScriptFunctionState::resume instead.

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, it works as expected.

extends Node2D

var iteration := 0

func _ready() -> void:
	recurse()


func recurse() -> void:
	iteration += 1
	if iteration < 5:
		recurse()
	else:
		print(script_backtrace())
		for i in 20:
			# Needed so the print shows up reliably in the Output panel.
			await get_tree().process_frame
		get_tree().quit()

Results in:

stack_language: GDScript
0: recurse (res://node_2d.gd : 14)
1: recurse (res://node_2d.gd : 12)
2: recurse (res://node_2d.gd : 12)
3: recurse (res://node_2d.gd : 12)
4: recurse (res://node_2d.gd : 12)
5: _ready (res://node_2d.gd : 6)

Some remarks:

  • Should the method be renamed to get_script_backtrace(), since it's a getter and has no side effects in itself?
  • Printed stack traces seem to go to stdout, but I'd generally expect them to go to stderr like in most other languages.
  • The stacktrace could have a note of the form Traceback (most recent call last) on the first line, like in Python.
    • To avoid making the stack trace longer, we can format the first line as follows:
    • GDScript traceback (most recent call last):
  • Colored printing in errors seems to be missing a reset color code at the end. Here, I use push_error() then manually print some text afterwards (which shouldn't appear colored as a result):

image

  • In error traces, I'd indent each line with 4 spaces so the entire error message is indented (except its first line).

@Mickeon
Copy link
Contributor

Mickeon commented Jul 5, 2024

A bit out of the loop, does #87576 essentially supersede this PR?

@reduz
Copy link
Member Author

reduz commented Jul 10, 2024

@Mickeon No, the other way around.

@RadiantUwU
Copy link
Contributor

Is the SIGSEGV gonna be fixed so i could try it out with #94364 ?

@kleonc
Copy link
Member

kleonc commented Aug 30, 2024

Can script_backtrace() return data in a structured form (Array[Dictionary]), like get_stack()? It also looks like this PR will resolve godotengine/godot-proposals#105.

@dalexeev I honestly don't see a lot of use for this, since the main use case is to print it, but I think adding another function script_stack() may be good for this, that could also include more stack information if you want.

Maybe the main use case is indeed to print it the default way, but for sure it needs to allow customization. One might want to print it but not with the default formatting (users do use custom logging for nice/clear output up to their liking). E.g. one could make the outputted stack trace entries clickable within the Godot editor, using BBCode (so they would take you to the relevant code).

With backtrace provided as a string one would need to do some reverse engineer parsing to get the actual data from it, which is not user friendly and not reliable.

I would argue that what's fundamental here is providing the actual stack data (e.g. as Array[Dictionary]). An easy way to obtain the whole backtrace string (with an arbitrary default formatting) I would consider as a nice-to-have bonus on top of that.

So I'm suggesting that the base should be:

  • Logger._log_error() instead of taking a preformatted string script_backtrace argument, it should take script_stack argument being the actual stack data.
  • Global script_stack() function returning the actual data, not a string. Could be named differently of course.

Then I wouldn't really mind what would be the additional stuff on top of that:

  • Some kind of script_stack_to_string(Array[Dictionary]) utility function for default formatting?
  • Additional script_backtrace() function? (same as script_stack_to_string(script_stack()))
  • Etc.

This kinda remainds me of Node.print_tree{_pretty}. I'm guessing these methods were initially added as "the main use case". Later users requested/added Node.get_tree_string{_pretty} as it makes sense to output same thing to some UI or file.
Note that these methods still do not handle cases when you want custom formatting, or to print custom data for each Node etc. But that's not a big problem, because Node tree data is available and thus everyone can manually traverse it outputting whatever/however they like.

In case of a script stack trace this would be a problem, because the actual stack trace data would be not obtainable in the first place.

@felipejfc
Copy link

Either this or #87576 need to be merged with high priority. Not being able to log unhandled exceptions to a remote system is a blocker to launch any serious mobile game with Godot.
For the SIGSEV that's happening here, could you fix it or give directions @reduz ?

@mihe
Copy link
Contributor

mihe commented Sep 2, 2024

I was asked by @reduz to try to take this across the finish line a while back, and was underway with diagnosing the SIGSEV at one point, but other priorities have kept getting in the way.

If I remember correctly the SIGSEV in the unit tests are largely the same issue I mentioned above with regards to stale pointers as a result of await, but I didn't have time to actually experiment with any fixes for it.

@reduz also expressed an interest in seeing some variation of #93648 merged into this PR, since you would then get not only the script backtrace as part of log_error, but also the native backtrace.

How that native backtrace holds up across the various platforms remains to be seen, but seeing as how symbolication will likely be impossible on mobile platforms I suspect the better option might be to have the backtraces be returned in some kind of structured format, rather than just a string, similar to what has been suggested in this thread by @kleonc already. That way you could at least get the stack frame pointers as part of the structured format and send those off to some service somewhere to be symbolicated, if you wanted to, or just pass the structured format into some function to be symbolicated locally (assuming you actually have the symbols and the means to do so) and then finally into some other function to be string-formatted.

Anyway, I'll try to get to this in a (hopefully) near future, if no one else beats me to it.

@juse4pro
Copy link

juse4pro commented Sep 5, 2024

Glad you are taking care of this topic @mihe 😃
This issue is a huge show stopper for our online game since Godot 4 came out. Unhandled C# exceptions that don't crash the game are terrible to debug. So this with PR we could gracefully get our logs / Sentry report and be happy.

Compared to Godot 3 a missing "feature". 😄

Is there any estimation if this PR could be merged into 4.4?

@felipejfc
Copy link

Possibly into 4.3 given it's a very small change that's easy to backport and is super important to any production project

@AThousandShips
Copy link
Member

This is an enhancement so it's not going to go into 4.3 unless it's considered really exceptional, we don't cherry-pick enhancements

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.