From 31d121e43bd2f87c33607079452f9908e52f019d Mon Sep 17 00:00:00 2001 From: David Rose Date: Sun, 30 Apr 2006 14:40:45 +0000 Subject: [PATCH] better sanity checking --- dtool/src/dtoolbase/deletedChain.T | 87 +++++++++++++++++++++++------- dtool/src/dtoolbase/deletedChain.h | 43 +++++++++++---- 2 files changed, 101 insertions(+), 29 deletions(-) diff --git a/dtool/src/dtoolbase/deletedChain.T b/dtool/src/dtoolbase/deletedChain.T index dd19fe9b0d..230fd0e821 100644 --- a/dtool/src/dtoolbase/deletedChain.T +++ b/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 INLINE void DeletedChain:: deallocate(Type *ptr) { TAU_PROFILE("void DeletedChain::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 +INLINE Type *DeletedChain:: +node_to_type(TVOLATILE TYPENAME DeletedChain::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 +INLINE TYPENAME DeletedChain::ObjectNode *DeletedChain:: +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 diff --git a/dtool/src/dtoolbase/deletedChain.h b/dtool/src/dtoolbase/deletedChain.h index a2fbd30316..7b9f3de15b 100644 --- a/dtool/src/dtoolbase/deletedChain.h +++ b/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.