[g3.7pre-752-ge70138a26] Crash on exit with QCDE

Bugs that have been investigated and resolved somehow.

Moderator: GZDoom Developers

Forum rules
Please don't bump threads here if you have a problem - it will often be forgotten about if you do. Instead, make a new thread here.
dpJudas
 
 
Posts: 3036
Joined: Sat May 28, 2016 1:01 pm

Re: [g3.7pre-752-ge70138a26] Crash on exit with QCDE

Post by dpJudas »

Graf Zahl wrote:Have you ever looked at the mess of code that's involved with this?
I never looked at the disassembly, no. Generally speaking the normal cost of a function static variable should be an unsynchronized comparison. If the variable says it isn't initialized it locks a mutex, checks again to prevent a race condition, initializes the object if it wasn't and then flags it as initialized, then unlocks. For all following calls the unsynchronized comparison will result in it not jumping to the init code. Total cost after the first call: 1-2 clock cycles. Could even be free in follow up calls if the C++ compiler is willing to patch the initial function.

I'm not 100% sure, but I believe C# static constructors are initialized the same way. First time any static functions or variables are accessed it calls the constructor. Probably implemented in a similar way.

In some situations the static variables in a function can actually be faster - mostly because they aren't initialized unless the function is actually called. Global static variables always are initialized on startup.

In any case, I think the real problem with global variables are the destructors. Because they aren't allowed to call anything else global there's very little stuff they can actually do. This problem exists no matter how you declare them, inside a function or not. The C++ runtime implementations in particular are having problems with this. It used to be extremely easy to crash them, just create a static std::thread or something else that usually talks to some subsystem in its destructor.
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
Posts: 49056
Joined: Sat Jul 19, 2003 10:19 am
Location: Germany

Re: [g3.7pre-752-ge70138a26] Crash on exit with QCDE

Post by Graf Zahl »

C++ really made a mess of this, that's for sure.
But from what I have seen, static function-local variables are 10 times worse than real global ones because now all bets are off regarding destruction order. It works fine as long as you don't use atexit, unfortunately that has historically been the preferred method to exit Doom, so we are now stuck right in the middle of termination handling and cannot in anyway risk that anything gets added to the list of exit routines after our own.

ZDoom even went so far to hook up its own exit dispatcher and then use atexit to register that.
And when it comes to resource allocation, most code is old enough to predate the RAII idiom, so again, there's far too much explicit cleanup.
I try to change this wherever I can but it often looks like a hopeless fight because there's simply too much of this mess and too many global variables are accessed from too many places. So far I operated under the assumption that mere deallocation from static destructors is fine. And some stuff like mutexes needs to be global anyway to work so what can we do?
_mental_
 
 
Posts: 3812
Joined: Sun Aug 07, 2011 4:32 am

Re: [g3.7pre-752-ge70138a26] Crash on exit with QCDE

Post by _mental_ »

Sorry, I was wrong. Moving argsCache out of JitCompiler::CreateFuncSignature() function fixes the crash for me.
dpJudas
 
 
Posts: 3036
Joined: Sat May 28, 2016 1:01 pm

Re: [g3.7pre-752-ge70138a26] Crash on exit with QCDE

Post by dpJudas »

Reading the documentation for std::atexit I can see that it officially states the functions are called mixed with static destructors. It also says if a static constructor is called after atexit then its destructor must run before the atexit handler is called.

Basically what we are seeing is what should always happen according to the standard. Good news is that the guarantee also applies the other way: all global static constructors called before our first std::atexit call must have their destructors called after the handler was called. In short: your initial assumption is actually written directly into the standard and thus moving the static to global will always work.
dpJudas
 
 
Posts: 3036
Joined: Sat May 28, 2016 1:01 pm

Re: [g3.7pre-752-ge70138a26] Crash on exit with QCDE

Post by dpJudas »

I pushed _mental_'s fix for this.
User avatar
Chris
Posts: 2940
Joined: Thu Jul 17, 2003 12:07 am
Graphics Processor: ATI/AMD with Vulkan/Metal Support

Re: [g3.7pre-752-ge70138a26] Crash on exit with QCDE

Post by Chris »

Graf Zahl wrote:But from what I have seen, static function-local variables are 10 times worse than real global ones because now all bets are off regarding destruction order.
Even globals are severely problematic. Recently I've been changing OpenAL Soft to use C++11 internally, and I've been having weird issues with GZDoom crashing on exit that I wasn't before. First a double-free, then a crash due to a global vector having empty entries that it was never given.

Thanks to this thread, I've tracked it down. OpenAL Soft's global object destructors are being run before GZDoom's atexit handlers, and GZDoom shuts down the audio device in an atexit handler. So the library has already run its global destructors when GZDoom tries to call into the library, with all kinds of things being broken. I'm not sure how to deal with this since once the destructors start firing, all bets are off on what's still valid outside of a given object's destructor. Attempts to fix it have been spotty at best, and no doubt well within undefined behavior land. The main cause seems to be because OpenAL is loaded at runtime, and is not considered a dependency of gzdoom (normally a library is destructed only after all its dependencies, so an executable or library that depends on libc can still call into it during destruction, for example).
User avatar
Graf Zahl
Lead GZDoom+Raze Developer
Lead GZDoom+Raze Developer
Posts: 49056
Joined: Sat Jul 19, 2003 10:19 am
Location: Germany

Re: [g3.7pre-752-ge70138a26] Crash on exit with QCDE

Post by Graf Zahl »

Yeah, atexit and external libraries are another story of their own...

To be honest, I think it was beyond stupid to put the entire engine shutdown into an atexit handler. Of course this was done by id and TeamTNT on plain C code.
I think it's time to explicitly process GZDoom's internal exit list BEFORE calling exit and not from inside the CRT.

The main problem of course is that thanks to using atexit it very liberally called exit from too many places instead of have a coordinated exit strategy. I actually never wrote a program myself that depended on such shitty semantics, but back in the 90 it must have been cool...
Post Reply

Return to “Closed Bugs [GZDoom]”