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

[3.x] Crash handler: Use print_error to include backtrace in logs #60780

Merged
merged 1 commit into from
May 5, 2022

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented May 5, 2022

I'm a bit surprised that we never thought about changing this until now, as it's a recurring hurdle for users that even with logging enabled, they won't get the actual output of our crash handler. This should now be fixed.

This doesn't necessarily mean that the output will be super useful though, as unless you provide an unstripped debug build to end users, the backtrace will be mangled or just ???. I also think it's not working well on Windows when using MSVC (or when using MinGW, I don't remember). Anyway, this was a low hanging fruit to improve things a bit.

master PR: #60782


Only tested on Linux so far. Help welcome to test macOS and Windows, and notably to see what's the output for both Windows MSVC and MinGW.

Test project (triggers a crash using OS.crash(), also handles NOTIFICATION_CRASH from script). It's configured to write the log directly in the project folder / next to the exported binary.
CrashTest.zip

@bruvzg
Copy link
Member

bruvzg commented May 5, 2022

print_error will work only if OS logger is initialized and functional, I guess crash can happen when it's not. But it is probably fine for most cases.

as unless you provide an unstripped debug build to end users, the backtrace will be mangled or just ???.

Might be better to build with separate symbols (Scons flag should be there) and have it as an optional download, instead of providing unstripped binaries.

@akien-mga akien-mga force-pushed the 3.x-crash-handler-logs branch from 4299fb5 to 6db4a9e Compare May 5, 2022 10:04
@akien-mga
Copy link
Member Author

akien-mga commented May 5, 2022

print_error will work only if OS logger is initialized and functional, I guess crash can happen when it's not. But it is probably fine for most cases.

Yeah the crash handlers already check that OS::get_singleton() is valid - but that doesn't necessarily mean that the logger itself is. If we find that it's a problem, this could likely be reworked further so that print methods have a graceful handling of invalid logger state (e.g. to fall back on a simple fprintf to stdout/stderr).

as unless you provide an unstripped debug build to end users, the backtrace will be mangled or just ???.

Might be better to build with separate symbols (Scons flag should be there) and have it as an optional download, instead of providing unstripped binaries.

Yeah this requires some more testing and documentation, I'm not sure the workflow is very well oiled for this. That's something we should aim to improve IMO, currently users are a bit powerless to get information on remote crashes from their players.

@bruvzg BTW if you want to poke at improving the Windows crash handler, reading it I assume it only works with MSVC as it depends heavily on Win32 APIs. Might be nice to have another one used for MinGW that would likely work similarly to the Linux/macOS one (I'm speculating, I haven't actually tested this on Windows recently).

@akien-mga akien-mga force-pushed the 3.x-crash-handler-logs branch from 6db4a9e to 3b8818e Compare May 5, 2022 10:13
@bruvzg
Copy link
Member

bruvzg commented May 5, 2022

BTW if you want to poke at improving the Windows crash handler, reading it I assume it only works with MSVC as it depends heavily on Win32 APIs

It will probably work, but debug symbols format might be incompatible, I'll check it.

@bruvzg
Copy link
Member

bruvzg commented May 5, 2022

Seems like crash handler can be enabled for MinGW build with a minimal changes, but debug symbols are incompatible, so it's will either require including some sort of custom DWARF information parser (https://github.com/ssbssa/dwarfstack, but lindwarf is LGPL). Or executable should be preprocessed with a tool like https://github.com/rainers/cv2pdb to convert symbols.

@bruvzg
Copy link
Member

bruvzg commented May 5, 2022

Also, Breakpad/Crashpad doesn't seem to support MinGW either, at least out of the box. But sentry fork seems to have it add - https://github.com/getsentry/crashpad/blob/getsentry/README.getsentry.md

@akien-mga
Copy link
Member Author

Tested the Windows/MSVC build from the PR's CI on Linux with Wine, it seems to work well too, the backtrace ends up in the godot.log.

As I've noticed in several bug reports, the Windows backtraces often seem to be weirdly hijacked by embree's TaskScheduler, that's also something we should look into (maybe it performs better with actual debug symbols). But that's outside the scope of this PR.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Tested on Windows/MSVC and macOS, stack trace gets to the file.

@akien-mga akien-mga merged commit c648c8e into godotengine:3.x May 5, 2022
@akien-mga akien-mga deleted the 3.x-crash-handler-logs branch May 5, 2022 12:47
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.

2 participants