Hi! This is from very early days of libgomp offloading support:
On 2013-09-14T21:29:56+0200, Jakub Jelinek <ja...@redhat.com> wrote:
> --- libgomp/target.c.jj 2013-09-09 17:41:02.290429613 +0200
> +++ libgomp/target.c 2013-09-13 16:41:24.514770386 +0200
> +static void
> +gomp_unmap_tgt (struct target_mem_desc *tgt)
> +{
> + /* FIXME: Deallocate on target the tgt->tgt_start .. tgt->tgt_end
> + region. */
> + if (tgt->tgt_end)
> + free (tgt->to_free);
> +
> + free (tgt->array);
> + free (tgt);
> +}
> +
> +static void
> +gomp_unmap_vars (struct target_mem_desc *tgt)
> +{
> + if (tgt->list_count == 0)
> + {
> + free (tgt);
> + return;
> + }
> +
> + size_t i;
> + gomp_mutex_lock (&dev_env_lock);
> + for (i = 0; i < tgt->list_count; i++)
> + if (tgt->list[i]->refcount > 1)
> + tgt->list[i]->refcount--;
> + else
> + {
> + splay_tree_key k = tgt->list[i];
> + if (k->copy_from)
> + /* FIXME: device to host copy. */
> + memcpy ((void *) k->host_start,
> + (void *) (k->tgt->tgt_start + k->tgt_offset),
> + k->host_end - k->host_start);
> + splay_tree_remove (&dev_splay_tree, k);
> + if (k->tgt->refcount > 1)
> + k->tgt->refcount--;
> + else
> + gomp_unmap_tgt (k->tgt);
> + }
> +
> + if (tgt->refcount > 1)
> + tgt->refcount--;
> + else
> + gomp_unmap_tgt (tgt);
> + gomp_mutex_unlock (&dev_env_lock);
> +}
(These days, the code is structured a little bit differently.)
I was debugging an OpenACC memory mapping issue that lead to host-side
memory corruption, and asked our dear friend Valgrind for help, which
quickly pointed me to the (current revision) of the code cited above. I
fixed the things on the OpenACC side, but also propose the attached patch
adding a safeguard to "Assert in
'libgomp/target.c:gomp_unmap_vars_internal' that we're not unmapping
'tgt' while it's still in use": the following 'tgt->list_count'
iterations as well as the following 'gomp_unref_tgt' would read 'free'd
memory. OK to commit? If approving this patch, please respond with
"Reviewed-by: NAME <EMAIL>" so that your effort will be recorded in the
commit log, see <https://gcc.gnu.org/wiki/Reviewed-by>.
Grüße
Thomas
From eed754b5d9545a605ed930742df5c733927fad04 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
Date: Fri, 6 Dec 2019 19:24:26 +0100
Subject: [PATCH] Assert in 'libgomp/target.c:gomp_unmap_vars_internal' that
we're not unmapping 'tgt' while it's still in use
libgomp/
* target.c (gomp_unmap_vars_internal): Add a safeguard to
'gomp_remove_var'.
---
libgomp/target.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/libgomp/target.c b/libgomp/target.c
index 84d6daa76ca..c24d7b1b1aa 100644
--- a/libgomp/target.c
+++ b/libgomp/target.c
@@ -1193,7 +1193,15 @@ gomp_unmap_vars_internal (struct target_mem_desc *tgt, bool do_copyfrom,
+ tgt->list[i].offset),
tgt->list[i].length);
if (do_unmap)
- gomp_remove_var (devicep, k);
+ {
+ struct target_mem_desc *k_tgt = k->tgt;
+ bool is_tgt_unmapped = gomp_remove_var (devicep, k);
+ /* It would be bad if TGT got unmapped while we're still iterating
+ over its LIST_COUNT, and also expect to use it in the following
+ code. */
+ assert (!is_tgt_unmapped
+ || k_tgt != tgt);
+ }
}
if (aq)
--
2.17.1
signature.asc
Description: PGP signature
