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

Color stacktraces printed by the crash handler when in a TTY #33505

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

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Nov 10, 2019

Follow-up to #44118.

This makes crash backtraces easier to read, which in turn improves the developer experience.

Preview

Windows 10 (MSVC)

Windows 10 (MSVC)

Linux

Linux

PS: For testing purposes, you can call CRASH_NOW(); anywhere to make Godot crash.

@aaronfranke
Copy link
Member

@Calinou Is this still desired? If so, it needs to be rebased on the latest master branch.

@akien-mga
Copy link
Member

Needs rebasing. Should be fine to merge then if it works.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Jul 26, 2022
@Calinou Calinou force-pushed the crash-handler-color-stacktrace branch from 9e2e275 to cc4c4e2 Compare July 26, 2022 15:39
@Calinou
Copy link
Member Author

Calinou commented Jul 26, 2022

Rebased and tested again on Linux, it works as expected.

Please test this on macOS before merging. The Windows portion of this PR won't work correctly until #44118 is merged (which still needs testers on Windows 7/8.1).

@Calinou Calinou force-pushed the crash-handler-color-stacktrace branch from cc4c4e2 to 95d37cf Compare July 26, 2022 17:03
@Anutrix
Copy link
Contributor

Anutrix commented Sep 8, 2022

Since, #44118 is merged now, maybe we can get this merged after a rebase.

@Calinou
Copy link
Member Author

Calinou commented Sep 9, 2022

Rebased and tested again, it works as expected on Linux and Windows (MSVC). See OP for updated screenshots.

@bruvzg If you have time, could you test and review the macOS side of this just in case? Thanks in advance 🙂

@Calinou Calinou force-pushed the crash-handler-color-stacktrace branch from 95d37cf to a27ed52 Compare September 9, 2022 17:11
@Calinou Calinou marked this pull request as ready for review September 9, 2022 17:12
@Calinou Calinou requested review from a team as code owners September 9, 2022 17:12
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.

Seems to be working fine, with a few minor changes.

Screenshot 2022-09-09 at 20 09 15

Note: at least on macOS, CRASH_NOW will generate SIGTRAP, so it will not be processed by Godot crash handler unless signal(SIGTRAP, handle_crash); is added.

platform/macos/crash_handler_macos.mm Outdated Show resolved Hide resolved
platform/macos/crash_handler_macos.mm Outdated Show resolved Hide resolved
@Calinou
Copy link
Member Author

Calinou commented Sep 9, 2022

Note: at least on macOS, CRASH_NOW will generate SIGTRAP, so it will not be processed by Godot crash handler unless signal(SIGTRAP, handle_crash); is added.

Should this be changed in another PR, either in the crash handler or the CRASH_NOW() macro?

@Calinou Calinou force-pushed the crash-handler-color-stacktrace branch from a27ed52 to 8b20d4a Compare September 9, 2022 18:14
@bruvzg
Copy link
Member

bruvzg commented Sep 9, 2022

Probably a crash handler should be changed to handle SIGTRAP. CRASH_NOW is calling __builtin_trap, so I'm not sure if it can be changed, it's likely platform dependent how it's work.

@Riteo
Copy link
Contributor

Riteo commented Mar 16, 2023

Just noticed this, nice work!

Is it really needed to explicitly add TTY checks and ANSI codes everywhere instead of having some sort of generic, optionally colored print system though? That might sound a bit excessive (and can definitely become such) but I'm pretty sure that there might be a clean and simple way of going along with it (hell, perhaps even a bunch of macros?). If it's done in more places that might be material for another PR but I can't stop wondering about this.

@Calinou
Copy link
Member Author

Calinou commented Mar 16, 2023

Is it really needed to explicitly add TTY checks and ANSI codes everywhere instead of having some sort of generic, optionally colored print system though? That might sound a bit excessive (and can definitely become such) but I'm pretty sure that there might be a clean and simple way of going along with it (hell, perhaps even a bunch of macros?). If it's done in more places that might be material for another PR but I can't stop wondering about this.

print_line(), print_error(), print_line_rich() and all other print functions could have a check that strips ANSI escape codes from the printed string if the terminal isn't a TTY. This may have a performance impact when not running from a TTY and printing lots of text though.

@Calinou Calinou force-pushed the crash-handler-color-stacktrace branch from 8b20d4a to 95cbc6f Compare March 19, 2023 20:17
@Calinou
Copy link
Member Author

Calinou commented Mar 19, 2023

Rebased and tested again, it works as expected.

image

@fire
Copy link
Member

fire commented Nov 4, 2024

@Calinou Would you like to rebase this? I'm interested.

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.

7 participants