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

Allow crash dialog for MinGW build #3715

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@qwertychouskie
Copy link
Contributor

qwertychouskie commented Jan 17, 2019

No description provided.

qwertychouskie added some commits Jan 17, 2019

@qwertychouskie

This comment has been minimized.

Copy link
Contributor Author

qwertychouskie commented Jan 25, 2019

capture

This was on a RelWithDebInfo cross-compiled MinGW 64-bit test build. cv2pdb -C supertuxkart.exe was used to make the PDB.

@qwertychouskie

This comment has been minimized.

Copy link
Contributor Author

qwertychouskie commented Jan 25, 2019

The only sorta-problem I had was that it took about a minute after STK freezing for the crash dialog to pop up.

EDIT: For reference, to cause a crash I added this to the top of LinkHelper::openURL:

// On-demand crash, use multiple ways
int* p = nullptr; *p = 10;
*((unsigned int*)0) = 0xDEAD;
int x=0; 1/x;
@qwertychouskie

This comment has been minimized.

Copy link
Contributor Author

qwertychouskie commented Feb 1, 2019

Any thoughts on this? The main STK builds need to be switched to MinGW builds because of #3344, and it would be nice to still have crash dialogs.

@qwertychouskie

This comment has been minimized.

Copy link
Contributor Author

qwertychouskie commented Feb 13, 2019

Here's a test build: https://jacobspctuneup.tk/STK/stk-code-mingw-crash-handling.zip

In this build LinkHelper::openURL is rigged to crash, the donate button in the credits works for triggering it.

@deveee You have a MinGW setup, right? Any thoughts on this PR?

@deveee

This comment has been minimized.

Copy link
Member

deveee commented Feb 14, 2019

I wonder if handler for memory allocation error should be totally disabled. Because in this case we should free some memory and not alloc more to display window with crash report. Especially that if there are some memory leaks or not enough ram, then it will probably crash in random place anyway. And I'm not sure if VS function is smart enough, so that it will throw exception.

There is std::set_new_handler() that works with mingw, but they explicitly say that you must free some resources or abort app.

And if everything else works, then I will merge this evening.

@deveee

This comment has been minimized.

Copy link
Member

deveee commented Feb 14, 2019

Tbh. I'm not convinced... Because mingw is typically used for self-compilation. And you need additional external tools like cv2pdb to create pdb file. So that most people will just get wrong crash reports.

@qwertychouskie

This comment has been minimized.

Copy link
Contributor Author

qwertychouskie commented Feb 14, 2019

The reason for this is #3344 (comment). Basically for the official Windows builds we either have to switch to MinGW builds or direct users to MS's website to download MSVCRT (which would be very bad UX). Or maybe it's possible to hex edit the MSVC-compiled binary to point to the unversioned MSVCRT dll?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment