[PATCH, libgomp] PR 67141, uninitialized acc_device_lock mutex

[PATCH, libgomp] PR 67141, uninitialized acc_device_lock mutex

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

Hi,
this patch fixes the uninitialized acc_device_lock mutex situation
reported in PR 67141. The patch attached on the bugzilla page
tries to solve it by constructor priorities, which we think will
probably be less manageable in general.

This patch changes goacc_host_init() to be called from
goacc_runtime_initialize() instead, thereby ensuring the init order.
libgomp testsuite was re-run without regressions, okay for trunk?

Thanks,
Chung-Lin

2015-09-18  Chung-Lin Tang  <cltang@codesourcery.com>

	PR libgomp/67141

	* oacc-int.h (goacc_host_init): Add declaration.
	* oacc-host.c (goacc_host_init): Remove static and
	constructor attribute
	* oacc-init.c (goacc_runtime_initialize): Call goacc_host_init()
	at end.

[-- Attachment #2: libgomp-acc-mutex.patch --]
[-- Type: text/x-patch, Size: 1204 bytes --]

Index: oacc-host.c
===================================================================
--- oacc-host.c	(revision 227895)
+++ oacc-host.c	(working copy)
@@ -256,7 +256,7 @@ static struct gomp_device_descr host_dispatch =
   };
 
 /* Initialize and register this device type.  */
-static __attribute__ ((constructor)) void
+void
 goacc_host_init (void)
 {
   gomp_mutex_init (&host_dispatch.lock);
Index: oacc-int.h
===================================================================
--- oacc-int.h	(revision 227895)
+++ oacc-int.h	(working copy)
@@ -97,6 +97,7 @@ void goacc_runtime_initialize (void);
 void goacc_save_and_set_bind (acc_device_t);
 void goacc_restore_bind (void);
 void goacc_lazy_initialize (void);
+void goacc_host_init (void);
 
 #ifdef HAVE_ATTRIBUTE_VISIBILITY
 # pragma GCC visibility pop
Index: oacc-init.c
===================================================================
--- oacc-init.c	(revision 227895)
+++ oacc-init.c	(working copy)
@@ -644,6 +644,9 @@ goacc_runtime_initialize (void)
 
   goacc_threads = NULL;
   gomp_mutex_init (&goacc_thread_lock);
+
+  /* Initialize and register the 'host' device type.  */
+  goacc_host_init ();
 }
 
 /* Compiler helper functions */

Re: [PATCH, libgomp] PR 67141, uninitialized acc_device_lock mutex

[Jakub Jelinek]

On Fri, Sep 18, 2015 at 03:41:30PM +0800, Chung-Lin Tang wrote:
> this patch fixes the uninitialized acc_device_lock mutex situation
> reported in PR 67141. The patch attached on the bugzilla page
> tries to solve it by constructor priorities, which we think will
> probably be less manageable in general.
> 
> This patch changes goacc_host_init() to be called from
> goacc_runtime_initialize() instead, thereby ensuring the init order.
> libgomp testsuite was re-run without regressions, okay for trunk?
> 
> Thanks,
> Chung-Lin
> 
> 2015-09-18  Chung-Lin Tang  <cltang@codesourcery.com>
> 
> 	PR libgomp/67141
> 

No vertical space in between PR line and subsequent entries.

> 	* oacc-int.h (goacc_host_init): Add declaration.
> 	* oacc-host.c (goacc_host_init): Remove static and
> 	constructor attribute

Full stop at the end of entry.

> 	* oacc-init.c (goacc_runtime_initialize): Call goacc_host_init()
> 	at end.

The patch is ok.  Though, perhaps as a follow-up, I think I'd prefer getting
rid of pthread_key_create (&goacc_cleanup_key, goacc_destroy_thread);,
it is wasteful if we do the same thing in initialize_team.  As the
goacc_tls_data pointer is __thread anyway, I think just putting it into
struct gomp_thread, arranging for init_team to be called from the env.c
ctor and from the team TLS destructor call also some oacc freeing if
the goacc_tls_data pointer is non-NULL (perhaps with __builtin_expect
unlikely).

	Jakub

Re: [PATCH, libgomp] PR 67141, uninitialized acc_device_lock mutex

[Chung-Lin Tang]

On 2015/9/18 04:02 PM, Jakub Jelinek wrote:
> On Fri, Sep 18, 2015 at 03:41:30PM +0800, Chung-Lin Tang wrote:
>> this patch fixes the uninitialized acc_device_lock mutex situation
>> reported in PR 67141. The patch attached on the bugzilla page
>> tries to solve it by constructor priorities, which we think will
>> probably be less manageable in general.
>>
>> This patch changes goacc_host_init() to be called from
>> goacc_runtime_initialize() instead, thereby ensuring the init order.
>> libgomp testsuite was re-run without regressions, okay for trunk?
>>
>> Thanks,
>> Chung-Lin
>>
>> 2015-09-18  Chung-Lin Tang  <cltang@codesourcery.com>
>>
>> 	PR libgomp/67141
>>
> 
> No vertical space in between PR line and subsequent entries.
> 
>> 	* oacc-int.h (goacc_host_init): Add declaration.
>> 	* oacc-host.c (goacc_host_init): Remove static and
>> 	constructor attribute
> 
> Full stop at the end of entry.
> 
>> 	* oacc-init.c (goacc_runtime_initialize): Call goacc_host_init()
>> 	at end.
> 
> The patch is ok.  Though, perhaps as a follow-up, I think I'd prefer getting
> rid of pthread_key_create (&goacc_cleanup_key, goacc_destroy_thread);,
> it is wasteful if we do the same thing in initialize_team.  As the
> goacc_tls_data pointer is __thread anyway, I think just putting it into
> struct gomp_thread, arranging for init_team to be called from the env.c
> ctor and from the team TLS destructor call also some oacc freeing if
> the goacc_tls_data pointer is non-NULL (perhaps with __builtin_expect
> unlikely).
> 
> 	Jakub

Committed, thanks for the review.
I believe this patch is also needed for 5.x, okay for that branch as well?

Thanks,
Chung-Lin

Re: [PATCH, libgomp] PR 67141, uninitialized acc_device_lock mutex

[Jakub Jelinek]

On Tue, Sep 22, 2015 at 02:49:04PM +0800, Chung-Lin Tang wrote:
> Committed, thanks for the review.
> I believe this patch is also needed for 5.x, okay for that branch as well?

Ok.
	Jakub