libgomp: Compile-time error for non-portable gomp_mutex_t initialization (was: libgomp nvptx plugin: rework initialisation and support the proposed load/unload hooks)

[Thomas Schwinge]

Hi!

It's Friday afternoon -- but anyway, is the following analysis correct?

On Thu, 26 Mar 2015 23:41:30 +0300, Ilya Verbin <iver...@gmail.com> wrote:
> On Thu, Mar 26, 2015 at 13:09:19 +0100, Jakub Jelinek wrote:
> > the current code is majorly broken.  As I've said earlier, e.g. the lack
> > of mutex guarding gomp_target_init (which is using pthread_once guaranteed
> > to be run just once) vs. concurrent GOMP_offload_register calls
> > (if those are run from ctors, then I guess something like dl_load_lock
> > ensures at least on glibc that multiple GOMP_offload_register calls aren't
> > performed at the same time) in accessing/reallocating offload_images
> > and num_offload_images and the lack of support to register further
> > images after the gomp_target_init call (if you dlopen further shared
> > libraries) is really bad.  And it would be really nice to support the
> > unloading.

> Here is the latest patch for libgomp and mic plugin.

> libgomp/

>       * target.c (register_lock): New mutex for offload image registration.

>       (GOMP_offload_register): Add mutex lock.

> --- a/libgomp/target.c
> +++ b/libgomp/target.c
> @@ -49,6 +49,9 @@ static void gomp_target_init (void);
>  /* The whole initialization code for offloading plugins is only run one.  */
>  static pthread_once_t gomp_is_initialized = PTHREAD_ONCE_INIT;
>  
> +/* Mutex for offload image registration.  */
> +static gomp_mutex_t register_lock;
> +
>  /* This structure describes an offload image.
>     It contains type of the target device, pointer to host table descriptor, 
> and
>     pointer to target data.  */

No gomp_mutex_init call for register_lock has been added -- there is no
sensible place to put it, because...

> @@ -654,6 +727,18 @@ void
>  GOMP_offload_register (void *host_table, enum offload_target_type 
> target_type,
>                      void *target_data)
>  {
> +  int i;
> +  gomp_mutex_lock (&register_lock);
> +
> +  /* Load image to all initialized devices.  */
> +  for (i = 0; i < num_devices; i++)
> +    {
> +      struct gomp_device_descr *devicep = &devices[i];
> +      if (devicep->type == target_type && devicep->is_initialized)
> +     gomp_offload_image_to_device (devicep, host_table, target_data);
> +    }
> +
> +  /* Insert image to array of pending images.  */
>    offload_images = gomp_realloc (offload_images,
>                                (num_offload_images + 1)
>                                * sizeof (struct offload_image_descr));
> @@ -663,74 +748,105 @@ GOMP_offload_register (void *host_table, enum 
> offload_target_type target_type,
>    offload_images[num_offload_images].target_data = target_data;
>  
>    num_offload_images++;
> +  gomp_mutex_unlock (&register_lock);
>  }

... it's used in this function, which is invoked from
__attributed__((constructor)) functions generated by
gcc/config/*/*mkoffload.c, and there is no guaranteed ordering for
constructor functions [... -- see source code comment added in the
following patch]:

commit 81d25f393d9b0fdca309354e6a61f707ff403fe2
Author: Thomas Schwinge <tho...@codesourcery.com>
Date:   Fri Sep 25 16:41:29 2015 +0200

    libgomp: Compile-time error for non-portable gomp_mutex_t initialization
    
        libgomp/
        * target.c [PLUGIN_SUPPORT]: Compile-time error if
        !GOMP_MUTEX_INIT_0.
---
 libgomp/target.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git libgomp/target.c libgomp/target.c
index 758ece5..49cb395 100644
--- libgomp/target.c
+++ libgomp/target.c
@@ -52,6 +52,16 @@ static pthread_once_t gomp_is_initialized = 
PTHREAD_ONCE_INIT;
 /* Mutex for offload image registration.  */
 static gomp_mutex_t register_lock;
 
+#if !GOMP_MUTEX_INIT_0
+# error Missing initialization for gomp_mutex_t register_lock.
+/* The problem is: gomp_mutex_t register_lock is used in
+   GOMP_offload_register_ver/GOMP_offload_register, which is called from a
+   __attribute__((constructor)) function (see the mkoffload files), so due to
+   non-deterministic constructor ordering, we can't have an initialization
+   constructor for gomp_mutex_t register_lock, such as
+   critical.c:initialize_critical, for example.  */
+#endif
+
 /* This structure describes an offload image.
    It contains type of the target device, pointer to host table descriptor, and
    pointer to target data.  */

I guess we don't have to worry about this non-portable gomp_mutex_t usage
right now; OK to commit the patch to at least document it (patch not yet
tested)?


Grüße,
 Thomas

signature.asc
Description: PGP signature