On Wed, Apr 01, 2015 at 04:14:05PM +0300, Ilya Verbin wrote:
> On Wed, Apr 01, 2015 at 07:21:47 +0200, Jakub Jelinek wrote:
> > On Wed, Apr 01, 2015 at 02:53:28AM +0300, Ilya Verbin wrote:
> > > +/* Similar to gomp_fatal, but release mutexes before. */
> > > +
> > > +static void
> > > +gomp_fatal_unlock (const char *fmt, ...)
> > > +{
> > > + int i;
> > > + va_list list;
> > > +
> > > + for (i = 0; i < num_devices; i++)
> > > + gomp_mutex_unlock (&devices[i].lock);
> >
> > This is wrong. Calling gomp_mutex_unlock on a lock that you don't have
> > locked is undefined behavior.
> > You really should unlock it in the caller which should be aware which 0/1/2
> > locks it holds.
>
> I was worried about the following scenario:
> 1. Thread 1 in GOMP_target locks device 1.
> 2. Thread 2 in GOMP_target locks device 2 and calls gomp_fatal.
> 3. GOMP_offload_unregister will wait for device 1, even device 2 is unlocked.
How is that different from
1. Thread 1 in GOMP_target locks device 1.
2. Thread 2 calls exit.
? I mean when you unlock the device and register locks if you own them
before gomp_fatal.
> Anyway, it was a bad idea to unlock mutexes from non-owner thread.
>
> Here is patch, which unlocks proper mutexes in the caller, as you suggested.
> make check-target-libgomp passed.
LGTM with proper ChangeLog entry.
Jakub
