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

Make push_error and push_warning print name of calling function #93648

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

TV4Fun
Copy link
Contributor

@TV4Fun TV4Fun commented Jun 27, 2024

Currently, push_error and push_warning give themselves as the calling
function, which is not useful for debugging. I have altered these to use the
C stacktrace to get the name of the function that called them. This saves
having to turn these functions into macros, which would require recompiling
every single piece of code that uses them. I have also set it to, when debug
symbols are available, use them with atos (available on macOS) to find the
exact source file and line being called from. It should be possible to the same
thing on Linux using addr2line, but I don't have a Linux box handy to test
that. I am not sure how you would implement this in Windows.

I have tested this on macOS Sonoma 14.5 (23F79). It should work on any *nix
system. I am not sure about Windows.

Fixes #76770

@TV4Fun TV4Fun requested a review from a team as a code owner June 27, 2024 05:17
@Mickeon Mickeon added this to the 4.x milestone Jun 27, 2024
@Mickeon
Copy link
Contributor

Mickeon commented Jun 27, 2024

Could you provide screenshots on how this would look now?

Also check out the static check errors.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

This needs to be done in a platform independent fashion, you need to fix the style issues so the CI can run to verify it on all platforms, but this would need to run on all platforms with reasonable support IMO, or be put just in the specific platform

core/templates/list.h Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

AThousandShips commented Jun 27, 2024

Also I'm not sure this change makes sense, this method is called from scripting, the GDScript method won't be shown, so please add some screenshots to see what this outputs

We don't want it to print the c++ method that called into scripting IMO

@TV4Fun TV4Fun requested review from a team as code owners June 28, 2024 04:51
@TV4Fun
Copy link
Contributor Author

TV4Fun commented Jun 28, 2024

Okay, I have fixed the formatting and code style issues, moved the platform dependent code into relevant subclasses of OS, and fixed it so that it now properly reports the stack trace when called from scripts. This is only implemented for macOS for now as that is the system I have available. The Linux implementation should be similar except that retrieving debug symbols should be done with addr2line instead of atos. I am not sure exactly how to implement this on Windows, so perhaps someone with more knowledge of Windows debugging could contribute that part.

To clarify, there are two cases this fix covers:

  1. push_error/push_warning are called from GDScript code This appears to have been partially implemented in the debugger for runtime code, but still fails for editor code. I have added code to retrieve the script callstack modeled after what RemoteDebugger::_err_handler does. In order for this to work requires that DEBUG_ENABLED be defined (true by default for editor and template_debug targets), and that Godot is run with the --debug flag.
  2. push_error/push_warning are called from C++ code This can be from within the Godot engine or from GDExtension/GDNative code. This requires retrieving the C++ callstack. Getting source filenames and line numbers requires that the assembly generating the message was compiled with debug symbols. If debug symbols are not available, it can still usually get the name of the function that produced the message, which is much more useful than what is currently availble.

Current behavior

Runtime GDScript

As I said, this is partially implemented already, but still includes an unhelpful reference to variant_utility.cpp that is now fixed. Here is how it looked with a warning generated from a GDScript file before in debugger:

Screenshot 2024-06-27 at 10 14 14 PM

Editor GDScript

GDScript run in the editor did not properly show where it was called from before, either in the editor logger or to stdout. Here is the view from the editor:

Screenshot 2024-06-27 at 10 13 01 PM

and here is the view from stdout (with Rider):

Screenshot 2024-06-27 at 10 13 08 PM

C++

Here is a warning generated from godot-git-plugin in the editor logger:

Screenshot 2024-06-27 at 10 13 35 PM

and the view from stdout:

Screenshot 2024-06-27 at 10 13 43 PM

Proposed improvements

Runtime GDScript

Screenshot 2024-06-27 at 11 16 41 PM Screenshot 2024-06-27 at 10 32 10 PM

Editor GDScript

View in editor logger:
Screenshot 2024-06-27 at 10 16 50 PM

View in stdout:

Screenshot 2024-06-27 at 10 17 06 PM

C++

View in editor logger:

Screenshot 2024-06-27 at 10 17 39 PM

View in stdout:

Screenshot 2024-06-27 at 10 17 58 PM

There are I'm sure still ways to make the results look a little better. All of the various logger implementations seem to have different ideas of what the two different string arguments to errfunc are for, which makes it hard to create a unified implementation that gives the relevant information without giving extra irrelevant information. I will happily accept any contributions anyone else wants to make to improve the output formatting.

@Calinou
Copy link
Member

Calinou commented Jun 28, 2024

Would #91006 be of any use here?

@TV4Fun
Copy link
Contributor Author

TV4Fun commented Jul 2, 2024

Would #91006 be of any use here?

Yes, that looks like it would do very much the same thing as what I do for GDScript stacktraces. Though theirs is a much more sweeping change, and mine is much more targeted. Mine requires being run in debug mode to get an accurate GDScript backtraces, while theirs completely rewrites GDScript's stack system to be fast enough to run all the time. There are pros and cons to both approaches.

One thing my PR does that theirs doesn't is print the C++ stacktrace, which is still needed if push_warning or push_error are called from C++ code.

@AThousandShips AThousandShips dismissed their stale review July 2, 2024 09:12

Will take a new look

@TV4Fun TV4Fun requested review from a team as code owners July 2, 2024 20:17
@AThousandShips AThousandShips removed request for a team July 3, 2024 07:43
core/templates/list.h Outdated Show resolved Hide resolved
core/os/os.h Outdated Show resolved Hide resolved
core/error/error_macros.h Outdated Show resolved Hide resolved
platform/macos/os_macos.h Outdated Show resolved Hide resolved
core/variant/variant_utility.cpp Outdated Show resolved Hide resolved

#if !defined(__ANDROID__) && !defined(__EMSCRIPTEN__)
virtual Vector<StackInfo> get_cpp_stack_info() const override;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

These should be in the respective OS implementations I'd say, i.e. Mac and Linux, or overridden as stubs in Android and Web, this makes it hard to track

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AThousandShips I think it makes the most sense to keep them in OS_Unix because execinfo.h is theoretically a part of any standards-compliant Unix system. The problem is that Android and Emscripten aren't fully POSIX compliant and don't provide the execinfo.h header. I was surprised to even see that they were children of OS_Unix because neither are really Unix systems. I can remove the #ifdefs from the header here, but I will need to keep them in os_unix.cpp (which already has a ton of these because of the differences in different Unix implementations) because the implementation there depends on being able to import execinfo.h, which is not present in either

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, but this makes it harder to find as it'd be expected to be in those platforms instead of drivers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's weird to me that OS_MacOS in platform inherits from OS_Unix in drivers, which inherits from OS in core, and each provides different portions of the platform class's implementation, but here we are.

@RandomShaper
Copy link
Member

Precisely these days I'm trying to hunt down the root trigger of certain operations and the ability to obtain a proper C++ stack trace would come super handy.

Regarding the implementation, I'd just like to point out that the various handle_crash() implementations already contain code for stack trace collection. I'd say this PR would need to refactor those pieces out into individual functions instead of adding its own.

Incorporate suggestions from @AThousandShips and necessary code changes to make
those work.
@TV4Fun
Copy link
Contributor Author

TV4Fun commented Jul 4, 2024

Precisely these days I'm trying to hunt down the root trigger of certain operations and the ability to obtain a proper C++ stack trace would come super handy.

Regarding the implementation, I'd just like to point out that the various handle_crash() implementations already contain code for stack trace collection. I'd say this PR would need to refactor those pieces out into individual functions instead of adding its own.

@RandomShaper Yes, just looking at the MacOS implementation, there is definitely a lot of overlap in functionality that could be moved into a separate library to save some duplication. If you would like to contribute a refactor to do that, you are more than welcome to. Personally, I am happy with this version and need to move on to other projects.

#ifdef DEBUG_ENABLED
String OS_MacOS::get_debug_descriptor(const StackInfo &p_info) {
String pipe;
Error error = execute("atos", { "-o", p_info.file, "-l", String::num_uint64(reinterpret_cast<uint64_t>(p_info.load_address), 16), String::num_uint64(reinterpret_cast<uint64_t>(p_info.symbol_address), 16) }, &pipe);
Copy link
Member

Choose a reason for hiding this comment

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

atos call probably should have -arch arm64 or -arch x86_64 arg added based on the running executable architecture, or in some conditions (like running under Rosetta) it might try reading wrong architecture from the fat binary.

@RandomShaper
Copy link
Member

@RandomShaper Yes, just looking at the MacOS implementation, there is definitely a lot of overlap in functionality that could be moved into a separate library to save some duplication. If you would like to contribute a refactor to do that, you are more than welcome to. Personally, I am happy with this version and need to move on to other projects.

Would it be fine with you that someone (possibly me when I can spend time on this) takes over your PR? That would mean that other contributor would take your changes, apply the refactor and commit them to a new PR superseding this one, crediting both authors (by having separate commits if it makes sense for the repo or using a co-author annotation).

@TV4Fun
Copy link
Contributor Author

TV4Fun commented Jul 4, 2024

@RandomShaper Yes, just looking at the MacOS implementation, there is definitely a lot of overlap in functionality that could be moved into a separate library to save some duplication. If you would like to contribute a refactor to do that, you are more than welcome to. Personally, I am happy with this version and need to move on to other projects.

Would it be fine with you that someone (possibly me when I can spend time on this) takes over your PR? That would mean that other contributor would take your changes, apply the refactor and commit them to a new PR superseding this one, crediting both authors (by having separate commits if it makes sense for the repo or using a co-author annotation).

Yes, that would be fine. Just give me credit in some way.

@reduz
Copy link
Member

reduz commented Jul 10, 2024

This is something I would really like to add to #91006 to make it more complete (my PR fixes a number of issues in GDScript and also makes it work properly outside the debugger, as well as making the log system support it properly too).

The problem is that It's kind of only usable for desktop, afaik on mobile and other platforms there is no implementation to get the C stack.

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.

push_error/warning should print GDScript line, not C++
7 participants