Use signal handler to invoke call_terms() before exit when possible

[psyke83]

It's not advisable to rely on atexit() to invoke SDL_Quit(), as it causes a segmentation fault on exit for the SDL2 KMSDRM driver due to a conflict with the Mesa drivers* which upstream Mesa may not elect to fix.

The issue can be resolved by replacing the atexit() call with a signal handler that will interrupt the D_DoomMain() and D_DoomLoop() functions, then invoke call_terms() within the main thread before exiting.

We can leave the atexit() hook intact to handle edge cases such as abnormal process exit, or exit methods which do not produce signals such as Alt+F4 or window close via GUI (neither of which are possible in a KMS context, so should not affect KMSDRM sessions).

Fixes a segmentation fault/uninterruptible application hang on exit for all KMS targets, including Raspberry Pi 4B, 3B and Intel i965.

• See: https://bugzilla.libsdl.org/show_bug.cgi?id=4530 and https://lists.freedesktop.org/archives/mesa-users/2019-March/001519.html

---

This appears to be working well and proper cleanup is done for Ctrl+C, kill and exiting via the menu option... but I may have overlooked another case where the call_terms() also needs to be invoked.

I'm targeting the legacy build first due to my usage targeting Raspberry Pi devices that are limited to GL2.1, but if everything works fine here, I can submit to the upstream gzdoom repo later if needed/wanted, since many devices are capable of using KMS contexts.

Also pinging @vanfanel for comment as I noticed they suffered from the issue on the zdoom forums (https://forum.zdoom.org/viewtopic.php?f=7&t=60020)

[psyke83]

It was brought to my attention that this may break Windows builds due to a lack of SIGINT handling. I will close until I have more time to investigate.

[psyke83]

I changed the patch to hopefully avoid any conflicts with Windows, and also fix some edge cases that I noticed in which a signal handler won't be called before exit (exit via Alt+F4 or window close via X11).

[drfrag666]

Looks good so far but i need a second opinion. Could the volatile int be a problem for performance on Linux? I will use an ifdef-ed int for Win. Also i don't see how this could affect Windows as the src/posix/sdl/i_main.cpp file AFAIK is only compiled for Linux hence the linux define is not needed.

[psyke83]

@drfrag666

For the purposes of testing this PR I did try to compile lzdoom on Windows 10 via Visual Studio 2019, but I encountered issues and gave up, so don't take my words as authoritative, but...

Although the /posix/ directory would suggest that Windows should not be involved in any way, the files I modified including i_main.cpp are part of PLAT_SDL_SOURCES here: https://github.com/drfrag666/gzdoom/blob/g3.3mgw/src/CMakeLists.txt#L539-L549 ... which gets added to Windows builds via: https://github.com/drfrag666/gzdoom/blob/g3.3mgw/src/CMakeLists.txt#L570

I recorded a custom demo of me running around (just on E1M1), and ran the timedemo with vblank_mode=0, and it gets ~180fps with a 3fps variance on different runs, with or without the patch that utilizes the volatile int.

I'm happy to make any changes you'd like, but if I'm correct about Windows really utilizing the SDL files in the /posix/ subdirectory, then care should be taken to make sure that the extern definition of game_running is also ifdef'd differently for Windows, and the existing Linux isolation for the signal_handler should probably stay IMO.

[psyke83]

Whoops, I didn't look further down the cmake file: https://github.com/drfrag666/gzdoom/blob/g3.3mgw/src/CMakeLists.txt#L816-L817

So the files aren't compiled on Windows - apologies for the confusion. I updated the PR to set the volatile property for non-Windows builds, and removed the linux isolation in i_main.cpp - though for the latter, I'm not sure if that will break MacOS builds? I'm afraid I can't do much to check that myself.

[drfrag666]

No, MacOS uses the cocoa folder. It passed the Travis check. But now that you mention it i think it's better to put the ifdef linux in d_main instead. @alexey-lysiuk says the PR looks good.

[psyke83]

It looks as though putting #ifdef __linux__ (instead of #ifndef WIN32) in d_main.cpp will break legacy MacOS builds that have -DOSX_COCOA_BACKEND=0 explicitly set by the user during compile; see: https://github.com/drfrag666/gzdoom/blob/g3.3mgw/src/CMakeLists.txt#L580

I'm sure that very few people would need that, but it's probably not a good idea to knowingly break the build, especially since your branch is targeting legacy devices.

So, the options are:

1. Leave the PR as-is. This will use volatile int for MacOS and Linux, and no builds should break.
2. Change d_main.cpp & i_main.cpp to use #ifdef __linux__ for the variable and signal calls. This will use volatile int just for Linux, and the MacOS and Windows builds should have no risk of performance degradation.

Happy to change it to whatever you wish.

[drfrag666]

Thanks. I was not sure since they are the main loops that's why i asked. I wasn't aware of that option and i'm not sure if it still works. Anyway please just change WIN32 to _WIN32 just in case. :)

[psyke83]

@drfrag666

PR has been updated to use #ifndef _WIN32 - not sure how I missed the underscore for existing lines in that file, but a grep of the source does show a lot of instances without, and I don't see WIN32 being defined via cmake or elsewhere. Maybe it's added by the compiler automatically; I never compile stuff on Windows, so I'm a bit clueless.

Thanks.

[drfrag666]

Both should be okay but it's for MinGW, just in case. But as Alexey says it really doesn't matter.