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

[Thomas Schwinge]

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?

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.)


Note that this likewise applies to the current upstream submission:
<https://inbox.sourceware.org/gcc-patches/400092d8ce44340cece0e2e38f88edbad6400b03.1657188329.git....@codesourcery.com>
"libgomp, nvptx: low-latency memory allocator".


Grüße
 Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955

>From d49d0b9dc4f96c496afb2d5caac4addb382fdf39 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
Date: Tue, 14 Feb 2023 13:35:03 +0100
Subject: [PATCH] In 'libgomp/allocator.c:omp_realloc', route 'free' through
 'MEMSPACE_FREE'

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

Fix-up for og12 commit c5d1d7651297a273321154a5fe1b01eba9dcf604
"libgomp, nvptx: low-latency memory allocator".

	libgomp/
	* allocator.c (omp_realloc): Route 'free' through 'MEMSPACE_FREE'.
---
 libgomp/allocator.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/libgomp/allocator.c b/libgomp/allocator.c
index 05b323d458e2..ba9a4e17cc20 100644
--- a/libgomp/allocator.c
+++ b/libgomp/allocator.c
@@ -854,7 +854,17 @@ retry:
       gomp_mutex_unlock (&free_allocator_data->lock);
 #endif
     }
-  free (data->ptr);
+  {
+    omp_memspace_handle_t was_memspace __attribute__((unused))
+      = (free_allocator_data
+	 ? free_allocator_data->memspace
+	 : predefined_alloc_mapping[free_allocator]);
+    int was_pinned __attribute__((unused))
+      = (free_allocator_data
+	 ? free_allocator_data->pinned
+	 : free_allocator == ompx_pinned_mem_alloc);
+    MEMSPACE_FREE (was_memspace, data->ptr, data->size, was_pinned);
+  }
   return ret;
 
 fail:
-- 
2.39.1