better sanity checking

dtool/src/dtoolbase/deletedChain.T

@@ -44,11 +44,11 @@ allocate(size_t size) {
     obj = _deleted_chain;
     _deleted_chain = _deleted_chain->_next;
     _lock->release();
-#ifndef NDEBUG
-    assert(obj->_flag == ((PN_int32)obj ^ deleted_chain_flag_hash));
-    obj->_flag = 0;
+#ifdef USE_DELETEDCHAINFLAG
+    assert(obj->_flag == (PN_int32)DCF_deleted);
+    obj->_flag = DCF_alive;
 #endif  // NDEBUG
-    return (Type *)obj;
+    return node_to_type(obj);
   }
   _lock->release();
 
@@ -59,11 +59,11 @@ allocate(size_t size) {
     TVOLATILE ObjectNode *result = (ObjectNode *)AtomicAdjust::compare_and_exchange_ptr((void * TVOLATILE &)_deleted_chain, (void *)obj, (void *)next);
     if (result == obj) {
       // We got it.
-#ifndef NDEBUG
-      assert(obj->_flag == ((PN_int32)obj ^ deleted_chain_flag_hash));
-      obj->_flag = 0;
+#ifdef USE_DELETEDCHAINFLAG
+      PN_int32 orig_flag = AtomicAdjust::compare_and_exchange(obj->_flag, DCF_deleted, DCF_alive);
+      assert(orig_flag == (PN_int32)DCF_deleted);
 #endif  // NDEBUG
-      return (Type *)obj;
+      return node_to_type(obj);
     }
     // Someone else grabbed the top link first.  Try again.
     obj = _deleted_chain;
@@ -74,16 +74,23 @@ allocate(size_t size) {
   // If we get here, the deleted_chain is empty; we have to allocate a
   // new object from the system pool.
 
+  size_t alloc_size = sizeof(Type);
+#ifdef USE_DELETEDCHAINFLAG
+  // In development mode, we also need to reserve space for _flag.
+  alloc_size += sizeof(PN_int32);
+#endif  // NDEBUG
+  size = max(alloc_size, sizeof(ObjectNode));
+
 #ifdef DO_MEMORY_USAGE
-  obj = (ObjectNode *)(*global_operator_new)(max(sizeof(Type), sizeof(ObjectNode)));
+  obj = (ObjectNode *)(*global_operator_new)(alloc_size);
 #else
-  obj = (ObjectNode *)malloc(max(sizeof(Type), sizeof(ObjectNode)));
+  obj = (ObjectNode *)malloc(alloc_size);
 #endif
-#ifndef NDEBUG
-  obj->_flag = 0;
+#ifdef USE_DELETEDCHAINFLAG
+  obj->_flag = DCF_alive;
 #endif  // NDEBUG
 
-  return (Type *)obj;
+  return node_to_type(obj);
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -95,13 +102,19 @@ template<class Type>
 INLINE void DeletedChain<Type>::
 deallocate(Type *ptr) {
   TAU_PROFILE("void DeletedChain<Type>::deallocate(Type *)", " ", TAU_USER);
-  TVOLATILE ObjectNode *obj = (ObjectNode *)ptr;
+  assert(ptr != (Type *)NULL);
 
-#ifndef NDEBUG
-  // We can't *guarantee* that this value is not a legitimate value of
-  // the deleted object, so we can't safely make this assertion.
-  //  assert(obj->_flag != ((PN_int32)obj ^ deleted_chain_flag_hash));
-  obj->_flag = (PN_int32)obj ^ deleted_chain_flag_hash;
+  TVOLATILE ObjectNode *obj = type_to_node(ptr);
+
+#ifdef USE_DELETEDCHAINFLAG
+  PN_int32 orig_flag = AtomicAdjust::compare_and_exchange(obj->_flag, DCF_alive, DCF_deleted);
+
+  // If this assertion is triggered, you double-deleted an object.
+  assert(orig_flag != (PN_int32)DCF_deleted);
+
+  // If this assertion is triggered, you tried to delete an object
+  // that was never allocated, or you have heap corruption.
+  assert(orig_flag == (PN_int32)DCF_alive);
 #endif  // NDEBUG
 
 #ifndef DELETED_CHAIN_USE_ATOMIC_EXCHANGE
@@ -126,13 +139,47 @@ deallocate(Type *ptr) {
   do {
     next = _deleted_chain;
     obj->_next = next;
-    result = (ObjectNode *)AtomicAdjust::compare_and_exchange_ptr((void * TVOLATILE &)_deleted_chain, (void *)next, (void *)obj);
+    result = type_to_node(AtomicAdjust::compare_and_exchange_ptr((void * TVOLATILE &)_deleted_chain, (void *)next, (void *)obj));
     // Keep trying until no one else got to _deleted_chain first.
   } while (result != next);
 
 #endif  // DELETED_CHAIN_USE_ATOMIC_EXCHANGE
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: DeletedChain::node_to_type
+//       Access: Private, Static
+//  Description: Casts an ObjectNode* to a Type*.
+////////////////////////////////////////////////////////////////////
+template<class Type>
+INLINE Type *DeletedChain<Type>::
+node_to_type(TVOLATILE TYPENAME DeletedChain<Type>::ObjectNode *node) {
+#ifdef USE_DELETEDCHAINFLAG
+  // In development mode, we increment the pointer so that the
+  // returned data does not overlap our _flags member.
+  return (Type *)(((PN_int32 *)node) + 1);
+#else
+  return (Type *)node;
+#endif  // NDEBUG
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: DeletedChain::type_to_node
+//       Access: Private, Static
+//  Description: Casts a Type* to an ObjectNode* .
+////////////////////////////////////////////////////////////////////
+template<class Type>
+INLINE TYPENAME DeletedChain<Type>::ObjectNode *DeletedChain<Type>::
+type_to_node(Type *ptr) {
+#ifdef USE_DELETEDCHAINFLAG
+  // In development mode, we decrement the pointer to undo the
+  // increment we did above.
+  return (ObjectNode *)(((PN_int32 *)ptr) - 1);
+#else
+  return (ObjectNode *)ptr;
+#endif  // NDEBUG
+}
+
 #ifndef DELETED_CHAIN_USE_ATOMIC_EXCHANGE
 ////////////////////////////////////////////////////////////////////
 //     Function: DeletedChain::init_lock

dtool/src/dtoolbase/deletedChain.h

@@ -28,14 +28,29 @@
 
 #ifdef HAVE_ATOMIC_COMPARE_AND_EXCHANGE_PTR
 // Actually, there appears to be a (maybe fatal) flaw in our
-//implementation of DeletedChain via the atomic exchange operation.
-//Specifically, a pointer may be removed from the head of the chain,
-//then the same pointer reinserted in the chain, while another thread
-//is waiting; and that thread will not detect the change.  For now,
-//then, let's not use this implmentation, and fall back to the mutex.
+// implementation of DeletedChain via the atomic exchange operation.
+// Specifically, a pointer may be removed from the head of the chain,
+// then the same pointer reinserted in the chain, while another thread
+// is waiting; and that thread will not detect the change.  For now,
+// then, let's not use this implementation, and fall back to the mutex.
 //#define DELETED_CHAIN_USE_ATOMIC_EXCHANGE
 #endif
 
+#ifndef NDEBUG
+// In development mode, we defined USE_DELETEDCHAINFLAG, which
+// triggers the piggyback of an additional word of data on every
+// allocated block, so we can ensure that an object is not
+// double-deleted and that the deleted chain remains intact.
+#define USE_DELETEDCHAINFLAG 1
+#endif // NDEBUG
+
+#ifdef USE_DELETEDCHAINFLAG
+enum DeletedChainFlag {
+  DCF_deleted = 0xfeedba0f,
+  DCF_alive = 0x12487654,
+};
+#endif
+
 ////////////////////////////////////////////////////////////////////
 //       Class : DeletedChain
 // Description : This template class can be used to provide faster
@@ -71,12 +86,24 @@ public:
 private:
   class ObjectNode {
   public:
-    TVOLATILE ObjectNode * TVOLATILE _next;
-#ifndef NDEBUG
+#ifdef USE_DELETEDCHAINFLAG
+    // In development mode, we piggyback this extra data.  This is
+    // maintained out-of-band from the actual pointer returned, so we
+    // can safely use this flag to indicate the difference between
+    // allocated and freed pointers.
     TVOLATILE PN_int32 _flag;
 #endif
+
+    // This pointer sits in the same space referenced by the actual
+    // pointer returned (unlike _flag, above).  It's only used when
+    // the object is deleted, so there's no harm in sharing space with
+    // the undeleted object.
+    TVOLATILE ObjectNode * TVOLATILE _next;
   };
 
+  INLINE static Type *node_to_type(TVOLATILE ObjectNode *node);
+  INLINE static ObjectNode *type_to_node(Type *ptr);
+
   // Ideally, the compiler and linker will unify all references to
   // this static pointer for a given type, as per the C++ spec.
   // However, if the compiler fails to do this (*cough* Microsoft), it
@@ -92,8 +119,6 @@ private:
 #endif
 };
 
-static const PN_int32 deleted_chain_flag_hash = 0x12345678;
-
 // Place this macro within a class definition to define appropriate
 // operator new and delete methods that take advantage of
 // DeletedChain.