|
From: Julian S. <ju...@op...> - 2023-06-18 13:57:52
|
On Sun, 18 Jun 2023 09:54:14 +0100 James Hogan via Flightgear-devel <fli...@li...> wrote: > On Saturday, 17 June 2023 23:05:30 BST Julian Smith wrote: > > Apologies if i've misunderstood, but this patch seems a little > > questionable - it seems to be hiding a problem rather than trying to > > gather information that could be used to fix it. > > No worries, I'm really not a big fan of it either. BTW apologies for not noticing the thread about this already here on flightgear-devel. > > > Could you give some more details of the failure mode we are trying > > to cope with here? > > It seems that on certain Mac platforms, the lock is no longer usable > after it has been destroyed resulting in exceptions when used by > destructing property nodes, presumably like the one Fahim Dalvi > quoted yesterday: > > libc++abi: terminating with uncaught exception of type > std::__1::system_error: mutex lock failed: Invalid argument > > So yeh its a destructor ordering thing, and it'd be better if the > properties were cleaned up first. Given that its not a problem on > Linux or I think Windows I figured its best to just make it work in > that case. Thanks for this. I've managed to provoke the problem on Linux by creating a global SGPropertyNode_ptr with `SGPropertyNode_ptr g_test1(new SGPropertyNode);` in simgear/simgear/props/props.cxx itself, before (i.e. above in the source code) the `NecroLock nodeOriginsLock;` line. Globals are destroyed in the reverse order in which they were constructed, so this guarantees that g_test1's destructor will be called /after/ nodeOriginsLock's destructor, so the former's call to SGSourceLocation() will try to use a destructed NecroLock. This reproducer only works reliably because everything is happening in the same compilation unit. Order of initialisation between compilation units is undefined, so i suspect it's just luck that things are currently working on Linux and Windows. > > > [If the problem is that properties are being manipulated after the > > static global `std::shared_mutex nodeOriginsLock` is being > > destructed by the C++ runtime, and for some reason we cannot avoid > > this happening, then maybe we could put nodeOriginsLock on the heap > > and never delete it, e.g. with `static std::shared_mutex& > > nodeOriginsLock = *new std::shared_mutex`?] > > Sure, that does seem better. I'll give that a try and report back > here with a patch for a kind Mac user to test. A possibly more elegant solution might be to make all SGPropertyNode constructors call a new dummy function in NecroLock(), which will ensure that the global nodeOriginsLock is created before all SGPropertyNode's. So nodeOriginsLock will be correspondingly destroyed /after/ all SGPropertyNode's. Arguably sticking nodeOriginsLock on the heap is a simpler solution though...? - Jules -- http://op59.net |