Hi! On 2023-02-14T15:11:14+0000, Andrew Stubbs <a...@codesourcery.com> wrote: > On 14/02/2023 12:54, Thomas Schwinge wrote: >> 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.
No issues in testing. >> Does that >> look correct to you? > > That looks correct. Thanks. I've pushed to devel/omp/gcc-12 branch commit 3a2c07395b0a565955a7b86f0eba866937e15989 "In 'libgomp/allocator.c:omp_realloc', route 'free' through 'MEMSPACE_FREE'", see attached. > The only remaining use of "free" should be the one > referring to the allocator object itself (i.e. the destructor). ACK. >> 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. And I'll leave that for another day, and/or another person. ;-) 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 3a2c07395b0a565955a7b86f0eba866937e15989 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/ChangeLog.omp | 2 ++ libgomp/allocator.c | 12 +++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/libgomp/ChangeLog.omp b/libgomp/ChangeLog.omp index 530f5c6acf6..819a5333907 100644 --- a/libgomp/ChangeLog.omp +++ b/libgomp/ChangeLog.omp @@ -1,5 +1,7 @@ 2023-02-16 Thomas Schwinge <tho...@codesourcery.com> + * allocator.c (omp_realloc): Route 'free' through 'MEMSPACE_FREE'. + * config/linux/allocator.c (linux_memspace_alloc) (linux_memspace_calloc): Clarify zero-initialization for pinned memory. diff --git a/libgomp/allocator.c b/libgomp/allocator.c index 05b323d458e..ba9a4e17cc2 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.25.1