From af30e0975aa33051c556e756c3aa1241b228b63a Mon Sep 17 00:00:00 2001 From: David Berry Date: Sat, 22 Jun 2024 16:35:25 +0100 Subject: [PATCH] memory: Fix bug flagging permament memory when AST is configured --with-memdebug If AST is configured with the --with-memdebug flag, then each mmemory block allocated internally by AST includes a boolean flag indicating if it is considered "permament" (i.e. it is never freed). The value of this flag is determined by whether or not the allocation occurs between a pair of matching calls to astBeginPM and astEndPM. A bug has existed for a long time which leads to spurious values sometimes being used for the flag when different threads try to allocate such permanent memory blocks simultaneously. This commit fixes this bug by using a mutex to serialise access to astBeginPM and astEndPm. There is some complication because astBeginPM/astEndPM blocks can be nested. This requires each thread to keep track of its own level of nesting, so that the mutex can be locked and unlocked only on the top level calls to astBeginPM and astEndPM. --- ast.news | 3 +++ src/memory.c | 46 ++++++++++++++++++++++++++++++++++++++++++---- src/memory.h | 1 + 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/ast.news b/ast.news index cb765a1c..6517397f 100644 --- a/ast.news +++ b/ast.news @@ -24,6 +24,9 @@ and other recent fixes to astRebinSeq were applied also to astRebin. - Some memory allocations in PolyMap were corrected. +- A bug in the flagging of permanent memory blocks when AST is configured +--with-memdebug has been fixed. + Main Changes in V9.2.9 ---------------------- diff --git a/src/memory.c b/src/memory.c index 47d3ac44..324399f4 100644 --- a/src/memory.c +++ b/src/memory.c @@ -166,6 +166,11 @@ * 5-OCT-2020 (DSB): * Fix bug in astChrCase - the text was always converted to upper case * regardless of the value of argument "upper". +* 22-JUN-2024 (DSB): +* Use a mutex to ensure that astBeginPM/astEndPM blocks in different +* threads occur sequentially rather than overlapping. Without +* this, the flagging of memory blocks as "permanent" is spurious (this +* only affects anything if AST is configured --with-memdebug). */ /* Configuration results. */ @@ -472,9 +477,17 @@ static size_t Peak_Usage = 0; static pthread_mutex_t mutex2 = PTHREAD_MUTEX_INITIALIZER; #define LOCK_DEBUG_MUTEX pthread_mutex_lock( &mutex2 ); #define UNLOCK_DEBUG_MUTEX pthread_mutex_unlock( &mutex2 ); + +static pthread_mutex_t mutex3 = PTHREAD_MUTEX_INITIALIZER; +#define LOCK_PM_MUTEX pthread_mutex_lock( &mutex3 ); +#define UNLOCK_PM_MUTEX pthread_mutex_unlock( &mutex3 ); + #else #define LOCK_DEBUG_MUTEX #define UNLOCK_DEBUG_MUTEX + +#define LOCK_PM_MUTEX +#define UNLOCK_PM_MUTEX #endif #endif @@ -487,12 +500,14 @@ static pthread_mutex_t mutex2 = PTHREAD_MUTEX_INITIALIZER; #define cache astGLOBAL(Memory,Cache) #define cache_init astGLOBAL(Memory,Cache_Init) #define use_cache astGLOBAL(Memory,Use_Cache) +#define pm_depth astGLOBAL(Memory,PM_Depth) /* Define the initial values for the global data for this module. */ #define GLOBAL_inits \ globals->Sizeof_Memory = 0; \ globals->Cache_Init = 0; \ globals->Use_Cache = 0; \ + globals->PM_Depth = 0; \ /* Create the global initialisation function. */ astMAKE_INITGLOBALS(Memory) @@ -524,6 +539,9 @@ static int cache_init = 0; /* Should the cache be used? */ static int use_cache = 0; +/* Depth of nesting of astBeginPM/astEndPM blocks */ +static int pm_depth = 0; + #endif /* Prototypes for Private Functions. */ @@ -5307,7 +5325,21 @@ void astBeginPM_( int *status ) { *- */ - LOCK_DEBUG_MUTEX; +/* Get access to the thread-specific global variables: */ + astDECLARE_GLOBALS + astGET_GLOBALS(NULL); + +/* If the current level of nesting of astBeginPM/astEndPM blocks is zero + for the current thread (i.e. this is a top level entry to this function), + then block until no other threads are in an astBeginPM/astEndPM block. + If the current level of nesting for this thread is greater than zero, we + can proceed safe in the knowledge that the top-level entry in this thread + has locked the mute, ensuring that no other threads can enter an + astBeginPM/astEndPM block. The mutex is unlocked by astEndPM. */ + if( pm_depth == 0 ) LOCK_PM_MUTEX; + +/* Increment the depth of astBeginPM/astEndPM block nesting. */ + pm_depth++; /* The global Perm_Mem flag indicates whether or not subsequent memory management functions in this module should store pointers to allocated @@ -5323,7 +5355,6 @@ void astBeginPM_( int *status ) { PM_Stack[ PM_Stack_Size++ ] = Perm_Mem; Perm_Mem = 1; } - UNLOCK_DEBUG_MUTEX; } void astEndPM_( int *status ) { @@ -5350,7 +5381,9 @@ void astEndPM_( int *status ) { *- */ - LOCK_DEBUG_MUTEX; +/* Get access to the thread-specific global variables: */ + astDECLARE_GLOBALS + astGET_GLOBALS(NULL); /* The global Perm_Mem flag indicates whether or not subsequent memory management functions in this module should store pointers to allocated @@ -5365,7 +5398,12 @@ void astEndPM_( int *status ) { Perm_Mem = PM_Stack[ --PM_Stack_Size ]; } - UNLOCK_DEBUG_MUTEX; +/* Decrement the level of nesting of astBeginPM/astEndPM blocks. */ + pm_depth--; + +/* If this thread has now ended the top level block, unlock the PM + mutex so that other threads can start an astBeginPM/astEndPM block */ + if( pm_depth == 0 ) UNLOCK_PM_MUTEX; } void astFlushMemory_( int leak, int *status ) { diff --git a/src/memory.h b/src/memory.h index 14c2b828..6e874677 100644 --- a/src/memory.h +++ b/src/memory.h @@ -188,6 +188,7 @@ typedef struct AstMemoryGlobals { int Cache_Init; int Use_Cache; Memory *Cache[ MXCSIZE + 1 ]; + int PM_Depth; } AstMemoryGlobals;