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

Add: use breakpad to create crash.dmp on MacOS / Linux too #11202

Merged
merged 1 commit into from Aug 20, 2023

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Aug 16, 2023

Motivation / Problem

Normally only the Windows platform could create a crash.dmp, making analysing crash-reports from MacOS / Linux rather tricky.

Description

Breakpad is a commonly used library from Google to create a minidump (what we call crash.dmp). It contains all the important bits and pieces about a crash, like stacktraces, threads, etc etc. Windows has a native function for that (via dbghelp.dll); this PR brings this to the other distros too.

You will read on the internet that you should use crashpad, instead of breakpad, but they say that for different reasons. As can be seen in https://github.com/google/breakpad, breakpad is perfectly maintained. The difference between crashpad and breakpad is in how it handles crashes. But as we do that ourselves, that is meaningless to us; we just want the minidump.

(crashpad starts a second process to monitor the actual application, allowing it to transmit a crash report from that other process. This is much safer, as the second process is perfectly healthy. But this gives tons of drawback, as having to ship a second executable, which has to be signed, and is seen in the process list, etc etc. Just read up the problems Chrome and Firefox had with that .. it is not worth it for OpenTTD. For transmitting crash reports I plan on adding code that on next startup says: "there was a crash, want to send it?". Which seems to be a much more robust way. As such, breakpad is a better fit for OpenTTD).

Windows is also using breakpad, as the minidump created by breakpad contains a bit more information than the dbghelp variant does.

Tested on MacOS 13.5.1, crash with ALT+0 in a debug build:
10 openttd!HandleKeypress(unsigned int, char32_t) + 0x1d6

Tested on Ubunut 22.04, crash with ALT+0 in a debug build:
6 openttd!HandleKeypress(unsigned int, char32_t) [window.cpp : 2675 + 0x9]

Both cases show a clear place it crashes. The Linux variant is slightly more useful, but that might be related to how MacOS is build. Not something in scope of this PR :)

Limitations

I made breakpad optional, but ENCOURAGED. This mainly as breakpad is not available in any distro, so for most developers it would be a bit annoying to install (vcpkg has it, so there is that). If you don't have breakpad, the crash.dmp will simply not be made.
I did look into vendoring the library, but that was way too much work for what we get in return.

One could argue we could leave the Windows dbghelp flow in, in case breakpad is not available. But as that means it is a release not made by us (as we do include it), the usefulness of such crash.dmp declines rapidly. Without the original pdb or breakpad symbols it is not useful. So those that actually care about having readable crash.dmps, need to keep those debug files anyway. And for them it is a small effort to install breakpad to. All-in-all, it is less technical debt for us, with no real user-impact.
Additionally, this means we no longer depend on an somewhat up-to-date version of dbghelp.dll, as we embed breakpad in the binary (instead of hooking into a dll that is within the users control).

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@PeterN
Copy link
Member

PeterN commented Aug 16, 2023

Yeah, either vendor it or make it optional, please...

@TrueBrain TrueBrain force-pushed the breakpad branch 6 times, most recently from 42a926c to 4947ae1 Compare August 17, 2023 22:50
src/crashlog.cpp Outdated Show resolved Hide resolved
@glx22
Copy link
Contributor

glx22 commented Aug 19, 2023

I tested the new windows minidump and opening it in MSVC still works fine.

Normally only the Windows platform could create a crash.dmp, making
analysing crash-reports from MacOS / Linux rather tricky.
@TrueBrain TrueBrain merged commit f120d2b into OpenTTD:master Aug 20, 2023
20 checks passed
@TrueBrain TrueBrain deleted the breakpad branch August 20, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants