static init ordering issues with MutexImpl

dtool/src/dtoolbase/deletedChain.T

@@ -21,7 +21,7 @@ TVOLATILE TYPENAME DeletedChain<Type>::ObjectNode * TVOLATILE DeletedChain<Type>
 
 #ifndef DELETED_CHAIN_USE_ATOMIC_EXCHANGE
 template<class Type>
-MutexImpl DeletedChain<Type>::_lock;
+MutexImpl *DeletedChain<Type>::_lock;
 #endif
 
 ////////////////////////////////////////////////////////////////////
@@ -38,18 +38,19 @@ allocate(size_t size) {
   TVOLATILE ObjectNode *obj;
 
 #ifndef DELETED_CHAIN_USE_ATOMIC_EXCHANGE
-  _lock.lock();
+  init_lock();
+  _lock->lock();
   if (_deleted_chain != (ObjectNode *)NULL) {
     obj = _deleted_chain;
     _deleted_chain = _deleted_chain->_next;
-    _lock.release();
+    _lock->release();
 #ifndef NDEBUG
     assert(obj->_flag == ((PN_int32)obj ^ deleted_chain_flag_hash));
     obj->_flag = 0;
 #endif  // NDEBUG
     return (Type *)obj;
   }
-  _lock.release();
+  _lock->release();
 
 #else  // DELETED_CHAIN_USE_ATOMIC_EXCHANGE
   obj = _deleted_chain;
@@ -102,12 +103,18 @@ deallocate(Type *ptr) {
 #endif  // NDEBUG
 
 #ifndef DELETED_CHAIN_USE_ATOMIC_EXCHANGE
-  _lock.lock();
+  // We have to test the lock again, even though we must have
+  // allocated this object at some point, because Windows might make
+  // multiple different DeletedChain instances for a particular Type,
+  // and there's no guarantee this one is the same one we used to
+  // allocate this object.
+  init_lock();
+  _lock->lock();
 
   obj->_next = _deleted_chain;
   _deleted_chain = obj;
 
-  _lock.release();
+  _lock->release();
 
 #else  // DELETED_CHAIN_USE_ATOMIC_EXCHANGE
 
@@ -123,3 +130,18 @@ deallocate(Type *ptr) {
 
 #endif  // DELETED_CHAIN_USE_ATOMIC_EXCHANGE
 }
+
+#ifndef DELETED_CHAIN_USE_ATOMIC_EXCHANGE
+////////////////////////////////////////////////////////////////////
+//     Function: DeletedChain::init_lock
+//       Access: Private
+//  Description: Ensures the lock pointer has been allocated.
+////////////////////////////////////////////////////////////////////
+template<class Type>
+INLINE void DeletedChain<Type>::
+init_lock() {
+  if (_lock == (MutexImpl *)NULL) {
+    _lock = new MutexImpl;
+  }
+}
+#endif  // DELETED_CHAIN_USE_ATOMIC_EXCHANGE

dtool/src/dtoolbase/deletedChain.h

@@ -87,7 +87,8 @@ private:
 #ifndef DELETED_CHAIN_USE_ATOMIC_EXCHANGE
   // If we don't have atomic compare-and-exchange, we need to use a
   // Mutex to protect the above linked list.
-  static MutexImpl _lock;
+  static INLINE void init_lock();
+  static MutexImpl *_lock;
 #endif
 };
 

dtool/src/interrogatedb/typeRegistry.I

@@ -31,3 +31,15 @@ freshen_derivations() {
     _derivations_fresh = true;
   }
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: TypeRegistry::init_lock
+//       Access: Private, Static
+//  Description: Ensures the lock pointer has been allocated.
+////////////////////////////////////////////////////////////////////
+INLINE void TypeRegistry::
+init_lock() {
+  if (_lock == (MutexImpl *)NULL) {
+    _lock = new MutexImpl;
+  }
+}

dtool/src/interrogatedb/typeRegistry.cxx

@@ -31,7 +31,7 @@
 // static init time, and we must use the arrow syntax to force
 // initialization of the interrogatedb_cat category.
 
-MutexImpl TypeRegistry::_lock;
+MutexImpl *TypeRegistry::_lock = NULL;
 TypeRegistry *TypeRegistry::_global_pointer = NULL;
 
 
@@ -48,7 +48,7 @@ TypeRegistry *TypeRegistry::_global_pointer = NULL;
 ////////////////////////////////////////////////////////////////////
 bool TypeRegistry::
 register_type(TypeHandle &type_handle, const string &name) {
-  _lock.lock();
+  _lock->lock();
 
   if (type_handle != TypeHandle::none()) {
     // Here's a type that was already registered.  Just make sure
@@ -56,7 +56,7 @@ register_type(TypeHandle &type_handle, const string &name) {
     TypeRegistryNode *rnode = look_up(type_handle, NULL);
     if (&type_handle == &rnode->_ref) {
       // No problem.
-      _lock.release();
+      _lock->release();
       nassertr(rnode->_name == name, false);
       return false;
     }
@@ -86,7 +86,7 @@ register_type(TypeHandle &type_handle, const string &name) {
     _derivations_fresh = false;
 
     type_handle = new_handle;
-    _lock.release();
+    _lock->release();
     return true;
   }
   TypeRegistryNode *rnode = (*ri).second;
@@ -102,7 +102,7 @@ register_type(TypeHandle &type_handle, const string &name) {
 
     if (type_handle == rnode->_handle) {
       // No problem.
-      _lock.release();
+      _lock->release();
       return false;
     }
     // But wait--the type_handle has changed!  We kept a reference to
@@ -113,7 +113,7 @@ register_type(TypeHandle &type_handle, const string &name) {
     interrogatedb_cat->error()
       << "Reregistering " << name << "\n";
     type_handle == rnode->_handle;
-    _lock.release();
+    _lock->release();
     return false;
   }
 
@@ -129,7 +129,7 @@ register_type(TypeHandle &type_handle, const string &name) {
 
     type_handle = rnode->_handle;
   }
-  _lock.release();
+  _lock->release();
   return false;
 }
 
@@ -144,7 +144,7 @@ register_type(TypeHandle &type_handle, const string &name) {
 ////////////////////////////////////////////////////////////////////
 TypeHandle TypeRegistry::
 register_dynamic_type(const string &name) {
-  _lock.lock();
+  _lock->lock();
 
   NameRegistry::iterator ri;
   ri = _name_registry.find(name);
@@ -172,14 +172,14 @@ register_dynamic_type(const string &name) {
     _name_registry[name] = rnode;
     _derivations_fresh = false;
 
-    _lock.release();
+    _lock->release();
     return *new_handle;
   }
 
   // Return the TypeHandle previously obtained.
   TypeRegistryNode *rnode = (*ri).second;
   TypeHandle handle = rnode->_handle;
-  _lock.release();
+  _lock->release();
   return handle;
 }
 
@@ -193,7 +193,7 @@ register_dynamic_type(const string &name) {
 ////////////////////////////////////////////////////////////////////
 void TypeRegistry::
 record_derivation(TypeHandle child, TypeHandle parent) {
-  _lock.lock();
+  _lock->lock();
 
   TypeRegistryNode *cnode = look_up(child, NULL);
   nassertv(cnode != (TypeRegistryNode *)NULL);
@@ -212,7 +212,7 @@ record_derivation(TypeHandle child, TypeHandle parent) {
     _derivations_fresh = false;
   }
 
-  _lock.release();
+  _lock->release();
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -226,7 +226,7 @@ record_derivation(TypeHandle child, TypeHandle parent) {
 ////////////////////////////////////////////////////////////////////
 void TypeRegistry::
 record_alternate_name(TypeHandle type, const string &name) {
-  _lock.lock();
+  _lock->lock();
 
   TypeRegistryNode *rnode = look_up(type, (TypedObject *)NULL);
   if (rnode != (TypeRegistryNode *)NULL) {
@@ -239,7 +239,7 @@ record_alternate_name(TypeHandle type, const string &name) {
     }
   }
 
-  _lock.release();
+  _lock->release();
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -251,7 +251,7 @@ record_alternate_name(TypeHandle type, const string &name) {
 ////////////////////////////////////////////////////////////////////
 TypeHandle TypeRegistry::
 find_type(const string &name) const {
-  _lock.lock();
+  _lock->lock();
 
   TypeHandle handle;
   NameRegistry::const_iterator ri;
@@ -259,7 +259,7 @@ find_type(const string &name) const {
   if (ri != _name_registry.end()) {
     handle = (*ri).second->_handle;
   }
-  _lock.release();
+  _lock->release();
 
   return handle;
 }
@@ -277,11 +277,11 @@ find_type(const string &name) const {
 ////////////////////////////////////////////////////////////////////
 string TypeRegistry::
 get_name(TypeHandle type, TypedObject *object) const {
-  _lock.lock();
+  _lock->lock();
   TypeRegistryNode *rnode = look_up(type, object);
   nassertr(rnode != (TypeRegistryNode *)NULL, "");
   string name = rnode->_name;
-  _lock.release();
+  _lock->release();
 
   return name;
 }
@@ -307,7 +307,7 @@ get_name(TypeHandle type, TypedObject *object) const {
 bool TypeRegistry::
 is_derived_from(TypeHandle child, TypeHandle base,
                 TypedObject *child_object) {
-  _lock.lock();
+  _lock->lock();
 
   const TypeRegistryNode *child_node = look_up(child, child_object);
   const TypeRegistryNode *base_node = look_up(base, (TypedObject *)NULL);
@@ -316,7 +316,7 @@ is_derived_from(TypeHandle child, TypeHandle base,
   freshen_derivations();
 
   bool result = TypeRegistryNode::is_derived_from(child_node, base_node);
-  _lock.release();
+  _lock->release();
   return result;
 }
 
@@ -329,10 +329,10 @@ is_derived_from(TypeHandle child, TypeHandle base,
 ////////////////////////////////////////////////////////////////////
 int TypeRegistry::
 get_num_root_classes() {
-  _lock.lock();
+  _lock->lock();
   freshen_derivations();
   int num_roots = _root_classes.size();
-  _lock.release();
+  _lock->release();
   return num_roots;
 }
 
@@ -344,13 +344,13 @@ get_num_root_classes() {
 ////////////////////////////////////////////////////////////////////
 TypeHandle TypeRegistry::
 get_root_class(int n) {
-  _lock.lock();
+  _lock->lock();
   freshen_derivations();
   TypeHandle handle;
   if (n >= 0 && n < get_num_root_classes()) {
     handle = _root_classes[n]->_handle;
   }
-  _lock.release();
+  _lock->release();
 
   return handle;
 }
@@ -373,11 +373,11 @@ get_root_class(int n) {
 ////////////////////////////////////////////////////////////////////
 int TypeRegistry::
 get_num_parent_classes(TypeHandle child, TypedObject *child_object) const {
-  _lock.lock();
+  _lock->lock();
   TypeRegistryNode *rnode = look_up(child, child_object);
   nassertr(rnode != (TypeRegistryNode *)NULL, 0);
   int num_parents = rnode->_parent_classes.size();
-  _lock.release();
+  _lock->release();
   return num_parents;
 }
 
@@ -390,14 +390,14 @@ get_num_parent_classes(TypeHandle child, TypedObject *child_object) const {
 ////////////////////////////////////////////////////////////////////
 TypeHandle TypeRegistry::
 get_parent_class(TypeHandle child, int index) const {
-  _lock.lock();
+  _lock->lock();
   TypeHandle handle;
   TypeRegistryNode *rnode = look_up(child, (TypedObject *)NULL);
   nassertr(rnode != (TypeRegistryNode *)NULL, TypeHandle::none());
   if (index >= 0 && index < (int)rnode->_parent_classes.size()) {
     handle = rnode->_parent_classes[index]->_handle;
   }
-  _lock.release();
+  _lock->release();
   return handle;
 }
 
@@ -415,11 +415,11 @@ get_parent_class(TypeHandle child, int index) const {
 ////////////////////////////////////////////////////////////////////
 int TypeRegistry::
 get_num_child_classes(TypeHandle child, TypedObject *child_object) const {
-  _lock.lock();
+  _lock->lock();
   TypeRegistryNode *rnode = look_up(child, child_object);
   nassertr(rnode != (TypeRegistryNode *)NULL, 0);
   int num_children = rnode->_child_classes.size();
-  _lock.release();
+  _lock->release();
   return num_children;
 }
 
@@ -432,14 +432,14 @@ get_num_child_classes(TypeHandle child, TypedObject *child_object) const {
 ////////////////////////////////////////////////////////////////////
 TypeHandle TypeRegistry::
 get_child_class(TypeHandle child, int index) const {
-  _lock.lock();
+  _lock->lock();
   TypeHandle handle;
   TypeRegistryNode *rnode = look_up(child, (TypedObject *)NULL);
   nassertr(rnode != (TypeRegistryNode *)NULL, TypeHandle::none());
   if (index >= 0 && index < (int)rnode->_child_classes.size()) {
     handle = rnode->_child_classes[index]->_handle;
   }
-  _lock.release();
+  _lock->release();
   return handle;
 }
 
@@ -460,7 +460,7 @@ get_child_class(TypeHandle child, int index) const {
 TypeHandle TypeRegistry::
 get_parent_towards(TypeHandle child, TypeHandle base,
                    TypedObject *child_object) {
-  _lock.lock();
+  _lock->lock();
   TypeHandle handle;
   const TypeRegistryNode *child_node = look_up(child, child_object);
   const TypeRegistryNode *base_node = look_up(base, NULL);
@@ -468,7 +468,7 @@ get_parent_towards(TypeHandle child, TypeHandle base,
            base_node != (TypeRegistryNode *)NULL, TypeHandle::none());
   freshen_derivations();
   handle = TypeRegistryNode::get_parent_towards(child_node, base_node);
-  _lock.release();
+  _lock->release();
   return handle;
 }
 
@@ -486,7 +486,8 @@ get_parent_towards(TypeHandle child, TypeHandle base,
 ////////////////////////////////////////////////////////////////////
 void TypeRegistry::
 reregister_types() {
-  _lock.lock();
+  init_lock();
+  _lock->lock();
   HandleRegistry::iterator ri;
   TypeRegistry *reg = ptr();
   for (ri = reg->_handle_registry.begin();
@@ -498,7 +499,7 @@ reregister_types() {
         << "Reregistering " << rnode->_name << "\n";
     }
   }
-  _lock.release();
+  _lock->release();
 }
 
 
@@ -511,7 +512,7 @@ reregister_types() {
 ////////////////////////////////////////////////////////////////////
 void TypeRegistry::
 write(ostream &out) const {
-  _lock.lock();
+  _lock->lock();
   // Recursively write out the tree, starting from each node that has
   // no parent.
   HandleRegistry::const_iterator hi;
@@ -523,7 +524,7 @@ write(ostream &out) const {
       write_node(out, 2, root);
     }
   }
-  _lock.release();
+  _lock->release();
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -534,7 +535,8 @@ write(ostream &out) const {
 ////////////////////////////////////////////////////////////////////
 TypeRegistry *TypeRegistry::
 ptr() {
-  _lock.lock();
+  init_lock();
+  _lock->lock();
   if (_global_pointer == NULL) {
 #ifdef NOTIFY_DEBUG
     if (interrogatedb_cat->is_spam()) {
@@ -544,7 +546,7 @@ ptr() {
 #endif
     init_global_pointer();
   }
-  _lock.release();
+  _lock->release();
   return _global_pointer;
 }
 

dtool/src/interrogatedb/typeRegistry.h

@@ -95,6 +95,8 @@ private:
   void write_node(ostream &out, int indent_level,
                   const TypeRegistryNode *node) const;
 
+  static INLINE void init_lock();
+
   typedef pvector<TypeRegistryNode *> HandleRegistry;
   HandleRegistry _handle_registry;
 
@@ -106,7 +108,7 @@ private:
 
   bool _derivations_fresh;
 
-  static MutexImpl _lock;
+  static MutexImpl *_lock;
   static TypeRegistry *_global_pointer;
 };
 

panda/src/pipeline/conditionVarDebug.I

@@ -25,7 +25,7 @@
 INLINE ConditionVarDebug::
 ConditionVarDebug(const ConditionVarDebug &copy) : 
   _mutex(copy._mutex), 
-  _impl(_mutex._global_mutex)
+  _impl(*_mutex._global_lock)
 {
   nassertv(false);
 }

panda/src/pipeline/conditionVarDebug.cxx

@@ -35,7 +35,7 @@
 ConditionVarDebug::
 ConditionVarDebug(MutexDebug &mutex) :
   _mutex(mutex),
-  _impl(mutex._global_mutex)
+  _impl(*mutex.get_global_lock())
 {
   nassertv(!_mutex._allow_recursion);
 }
@@ -76,14 +76,14 @@ ConditionVarDebug::
 ////////////////////////////////////////////////////////////////////
 void ConditionVarDebug::
 wait() {
-  _mutex._global_mutex.lock();
+  _mutex._global_lock->lock();
 
   if (!_mutex.do_debug_is_locked()) {
     ostringstream ostr;
     ostr << *Thread::get_current_thread() << " attempted to wait on "
          << *this << " without holding " << _mutex;
     nassert_raise(ostr.str());
-    _mutex._global_mutex.release();
+    _mutex._global_lock->release();
     return;
   }
 
@@ -101,7 +101,7 @@ wait() {
       << *Thread::get_current_thread() << " awake on " << *this << "\n";
   }
 
-  _mutex._global_mutex.release();
+  _mutex._global_lock->release();
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -122,13 +122,13 @@ wait() {
 ////////////////////////////////////////////////////////////////////
 void ConditionVarDebug::
 signal() {
-  _mutex._global_mutex.lock();
+  _mutex._global_lock->lock();
   if (!_mutex.do_debug_is_locked()) {
     ostringstream ostr;
     ostr << *Thread::get_current_thread() << " attempted to signal "
          << *this << " without holding " << _mutex;
     nassert_raise(ostr.str());
-    _mutex._global_mutex.release();
+    _mutex._global_lock->release();
     return;
   }
 
@@ -138,7 +138,7 @@ signal() {
   }
 
   _impl.signal();
-  _mutex._global_mutex.release();
+  _mutex._global_lock->release();
 }
 
 ////////////////////////////////////////////////////////////////////

panda/src/pipeline/mutexDebug.I

@@ -23,7 +23,7 @@
 //  Description: Do not attempt to copy mutexes.
 ////////////////////////////////////////////////////////////////////
 INLINE MutexDebug::
-MutexDebug(const MutexDebug &copy) : _cvar(_global_mutex) {
+MutexDebug(const MutexDebug &copy) : _cvar_impl(*get_global_lock()) {
   nassertv(false);
 }
 
@@ -55,9 +55,9 @@ operator = (const MutexDebug &copy) {
 INLINE void MutexDebug::
 lock() const {
   TAU_PROFILE("void MutexDebug::lock()", " ", TAU_USER);
-  _global_mutex.lock();
+  _global_lock->lock();
   ((MutexDebug *)this)->do_lock();
-  _global_mutex.release();
+  _global_lock->release();
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -118,9 +118,9 @@ elevate_lock() const {
 INLINE void MutexDebug::
 release() const {
   TAU_PROFILE("void MutexDebug::release()", " ", TAU_USER);
-  _global_mutex.lock();
+  _global_lock->lock();
   ((MutexDebug *)this)->do_release();
-  _global_mutex.release();
+  _global_lock->release();
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -136,8 +136,25 @@ release() const {
 INLINE bool MutexDebug::
 debug_is_locked() const {
   TAU_PROFILE("bool MutexDebug::debug_is_locked()", " ", TAU_USER);
-  _global_mutex.lock();
+  _global_lock->lock();
   bool is_locked = do_debug_is_locked();
-  _global_mutex.release();
+  _global_lock->release();
   return is_locked;
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: MutexDebug::get_global_lock
+//       Access: Private, Static
+//  Description: Ensures the global MutexImpl pointer has been
+//               created, and returns its pointer.  Since this method
+//               is called by the MutexDebug constructor, any other
+//               (non-static) methods of MutexDebug may simply assume
+//               that the pointer has already been created.
+////////////////////////////////////////////////////////////////////
+INLINE MutexImpl *MutexDebug::
+get_global_lock() {
+  if (_global_lock == (MutexImpl *)NULL) {
+    _global_lock = new MutexImpl;
+  }
+  return _global_lock;
+}

panda/src/pipeline/mutexDebug.cxx

@@ -22,7 +22,7 @@
 
 #ifdef DEBUG_THREADS
 
-MutexImpl MutexDebug::_global_mutex;
+MutexImpl *MutexDebug::_global_lock;
 
 ////////////////////////////////////////////////////////////////////
 //     Function: MutexDebug::Constructor
@@ -35,7 +35,7 @@ MutexDebug(const string &name, bool allow_recursion) :
   _allow_recursion(allow_recursion),
   _locking_thread(NULL),
   _lock_count(0),
-  _cvar(_global_mutex)
+  _cvar_impl(*get_global_lock())
 {
 }
 
@@ -72,7 +72,7 @@ output(ostream &out) const {
 //     Function: MutexDebug::do_lock
 //       Access: Private
 //  Description: The private implementation of lock() assumes that
-//               _global_mutex is held.
+//               _lock_impl is held.
 ////////////////////////////////////////////////////////////////////
 void MutexDebug::
 do_lock() {
@@ -112,7 +112,7 @@ do_lock() {
         report_deadlock(this_thread);
         nassert_raise("Deadlock");
 
-        _global_mutex.release();
+        _global_lock->release();
         return;
       }
       Thread *next_thread = next_mutex->_locking_thread;
@@ -139,7 +139,7 @@ do_lock() {
         << *_locking_thread << ")\n";
     }
     while (_locking_thread != (Thread *)NULL) {
-      _cvar.wait();
+      _cvar_impl.wait();
     }
     
     if (thread_cat.is_spam()) {
@@ -159,7 +159,7 @@ do_lock() {
 //     Function: MutexDebug::do_release
 //       Access: Private
 //  Description: The private implementation of lock() assumes that
-//               _global_mutex is held.
+//               _lock_impl is held.
 ////////////////////////////////////////////////////////////////////
 void MutexDebug::
 do_release() {
@@ -174,7 +174,7 @@ do_release() {
     ostr << *this_thread << " attempted to release "
          << *this << " which it does not own";
     nassert_raise(ostr.str());
-    _global_mutex.release();
+    _global_lock->release();
     return;
   }
 
@@ -184,7 +184,7 @@ do_release() {
   if (_lock_count == 0) {
     // That was the last lock held by this thread.  Release the lock.
     _locking_thread = (Thread *)NULL;
-    _cvar.signal();
+    _cvar_impl.signal();
   }
 }
 
@@ -192,7 +192,7 @@ do_release() {
 //     Function: MutexDebug::do_debug_is_locked
 //       Access: Private
 //  Description: The private implementation of debug_is_locked()
-//               assumes that _global_mutex is held.
+//               assumes that _lock_impl is held.
 ////////////////////////////////////////////////////////////////////
 bool MutexDebug::
 do_debug_is_locked() const {
@@ -202,7 +202,7 @@ do_debug_is_locked() const {
 ////////////////////////////////////////////////////////////////////
 //     Function: MutexDebug::report_deadlock
 //       Access: Private
-//  Description: Reports a detected deadlock situation.  _global_mutex
+//  Description: Reports a detected deadlock situation.  _lock_impl
 //               should be already held.
 ////////////////////////////////////////////////////////////////////
 void MutexDebug::

panda/src/pipeline/mutexDebug.h

@@ -59,13 +59,16 @@ private:
   void report_deadlock(Thread *this_thread);
 
 private:
+  INLINE static MutexImpl *get_global_lock();
+
   string _name;
   bool _allow_recursion;
   Thread *_locking_thread;
   int _lock_count;
-  ConditionVarImpl _cvar;
 
-  static MutexImpl _global_mutex;
+  ConditionVarImpl _cvar_impl;
+
+  static MutexImpl *_global_lock;
 
   friend class ConditionVarDebug;
 };