Re: [PATCH 1/1] libgomp: Add destructor to delete runtime env keys

Re: [PATCH 1/1] libgomp: Add destructor to delete runtime env keys

[Jakub Jelinek]

On Tue, Dec 24, 2019 at 07:21:53PM +0530, Ayush Mittal wrote:
> [BUG: 93065] libgomp: destructor missing to delete goacc_cleanup_key
> libgomp constructor creates goacc_cleanup_key on dlopen but doesn't delete key on dlclose. 
> dlopen and dlclose of libgomp.so exhausts pthread keys, which results in pthread_key_create failure.
> 
> pthread_key_delete needs to be called by libgomp destructor.
> 
> Signed-off-by: Ayush Mittal <ayush.m@samsung.com>
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>

Please provide a ChangeLog entry, see
https://gcc.gnu.org/codingconventions.html#ChangeLogs
for details.

> --- a/libgomp/env.c
> +++ b/libgomp/env.c
> @@ -1303,7 +1303,10 @@ initialize_env (void)
>    goacc_runtime_initialize ();
>  }
>  
> -\f

Please don't remove the form feed character if there is one, though
looking at the sources I really don't know what you are patching, the
patch can't apply to trunk which has something quite different in there.
env.c ends in
  goacc_runtime_initialize ();

  goacc_profiling_initialize ();
}
#endif /* LIBGOMP_OFFLOADED_ONLY */

> +static void __attribute__((destructor)) env_destroy(){
> +	goacc_runtime_deinitialize();
> +}

Wrong formatting.  The function name needs to start a new line,
space before (, there should be void in between ()s and { should be on yet
another line, indentation is by 2 columns rather than 5 and another space
before (.  That said, I really don't see the point of adding a destructor
function in one file that just calls a function in another file.

>  /* The public OpenMP API routines that access these variables.  */
>  
>  void
> diff --git a/libgomp/oacc-init.c b/libgomp/oacc-init.c
> index 42d005d..9bd0c1a 100644
> --- a/libgomp/oacc-init.c
> +++ b/libgomp/oacc-init.c
> @@ -658,6 +658,16 @@ goacc_runtime_initialize (void)
>    goacc_host_init ();
>  }
>  
> +attribute_hidden void 
> +goacc_runtime_deinitialize() {
> +
> +#if !(defined HAVE_TLS || defined USE_EMUTLS)
> +  pthread_key_delete (goacc_tls_key);
> +#endif
> +  pthread_key_delete(goacc_cleanup_key);
> +
> +}

As in, why isn't this function simply
static void __attribute__((destructor))
goacc_runtime_deinitialize (void)
{
#if !(defined HAVE_TLS || defined USE_EMUTLS)
  pthread_key_delete (goacc_tls_key);
#endif
  pthread_key_delete (goacc_cleanup_key);
}

(note again formatting issues, including a useless line at the end).
I think all those attribute_hidden uses in oacc-init.c are useless,
because oacc-int.h wraps all the declarations inside of a hidden
block and so all those decls have hidden visibility, but that is a
preexisting problem.

> --- a/libgomp/oacc-int.h
> +++ b/libgomp/oacc-int.h
> @@ -94,6 +94,7 @@ goacc_thread (void)
>  void goacc_register (struct gomp_device_descr *) __GOACC_NOTHROW;
>  void goacc_attach_host_thread_to_device (int);
>  void goacc_runtime_initialize (void);
> +void goacc_runtime_deinitialize (void);
>  void goacc_save_and_set_bind (acc_device_t);
>  void goacc_restore_bind (void);
>  void goacc_lazy_initialize (void);

And if goacc_runtime_deinitialize is static, it can't be declared here.

	Jakub