Re: [og12] In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE' (was: [PATCH] libgomp, OpenMP, nvptx: Low-latency memory allocator)

[Andrew Stubbs]

On 14/02/2023 12:54, Thomas Schwinge wrote:
Hi Andrew!
On 2022-01-13T11:13:51+0000, Andrew Stubbs <a...@codesourcery.com> wrote:
Updated patch: this version fixes some missed cases of malloc in the
realloc implementation.

Right, and as it seems I've run into another issue: a stray 'free'.

--- a/libgomp/allocator.c
+++ b/libgomp/allocator.c

Re 'omp_realloc':

@@ -660,9 +709,10 @@ retry:
        gomp_mutex_unlock (&allocator_data->lock);
  #endif
        if (prev_size)
-     new_ptr = realloc (data->ptr, new_size);
+     new_ptr = MEMSPACE_REALLOC (allocator_data->memspace, data->ptr,
+                                 data->size, new_size);
        else
-     new_ptr = malloc (new_size);
+     new_ptr = MEMSPACE_ALLOC (allocator_data->memspace, new_size);
        if (new_ptr == NULL)
       {
  #ifdef HAVE_SYNC_BUILTINS
@@ -690,7 +740,11 @@ retry:
          && (free_allocator_data == NULL
              || free_allocator_data->pool_size == ~(uintptr_t) 0))
      {
-      new_ptr = realloc (data->ptr, new_size);
+      omp_memspace_handle_t memspace __attribute__((unused))
+     = (allocator_data
+        ? allocator_data->memspace
+        : predefined_alloc_mapping[allocator]);
+      new_ptr = MEMSPACE_REALLOC (memspace, data->ptr, data->size, new_size);
        if (new_ptr == NULL)
       goto fail;
        ret = (char *) new_ptr + sizeof (struct omp_mem_header);
@@ -701,7 +755,11 @@ retry:
      }
    else
      {
-      new_ptr = malloc (new_size);
+      omp_memspace_handle_t memspace __attribute__((unused))
+     = (allocator_data
+        ? allocator_data->memspace
+        : predefined_alloc_mapping[allocator]);
+      new_ptr = MEMSPACE_ALLOC (memspace, new_size);
        if (new_ptr == NULL)
       goto fail;
      }
@@ -735,32 +793,35 @@ retry:
|    free (data->ptr);
    return ret;

I run into a SIGSEGV if a non-'malloc'-based allocation is 'free'd here.

The attached
"In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE'"
appears to resolve my issue, but not yet regression-tested.  Does that
look correct to you?

That looks correct. The only remaining use of "free" should be the one 
referring to the allocator object itself (i.e. the destructor).

Or, instead of invoking 'MEMSPACE_FREE', should we scrap the
'used_pool_size' bookkeeping here, and just invoke 'omp_free' instead?

     --- libgomp/allocator.c
     +++ libgomp/allocator.c
     @@ -842,19 +842,7 @@ retry:
        if (old_size - old_alignment < size)
          size = old_size - old_alignment;
        memcpy (ret, ptr, size);
     -  if (__builtin_expect (free_allocator_data
     -                   && free_allocator_data->pool_size < ~(uintptr_t) 0, 0))
     -    {
     -#ifdef HAVE_SYNC_BUILTINS
     -      __atomic_add_fetch (&free_allocator_data->used_pool_size, 
-data->size,
     -                     MEMMODEL_RELAXED);
     -#else
     -      gomp_mutex_lock (&free_allocator_data->lock);
     -      free_allocator_data->used_pool_size -= data->size;
     -      gomp_mutex_unlock (&free_allocator_data->lock);
     -#endif
     -    }
     -  free (data->ptr);
     +  ialias_call (omp_free) (ptr, free_allocator);
        return ret;

(I've not yet analyzed whether that's completely equivalent.)

The used_pool_size code comes from upstream, so if you want to go beyond 
the mechanical substitution of "free" then you're adding a new patch 
(rather than tweaking an old one). I'll leave that for others to comment on.

Andrew