Hello world,
the attached patch fixes the problem reported in PR 66756: When
opeing a file, the main lock for all units was acquired, the unit lock
was acquired, and then the main lock was released and re-aquired. To
the thread sanitizer, this is a lock-order inversion.
One option would have been to simply close the bug, because this
only occurs in opening a file, when the gfc_unit has not yet had
a chance to escape to another thread. However, it appears that
this causes trouble debugging parallel applications, hence this
patch.
What this patch does is to change the assumptions for insert_unit:
Previously, this used to lock the newly created unit, and the caller
had to unlock. Now, gfc_get_unit can do the locking after releasing
the global lock.
This gets rid of the thread sanitizer issue; the thread sanitizer
output is clean. However, I would appreciate feedback about whether
this approach (and my code) is correct.
Regression-tested.
Comments? Suggestions for improvements/other approaches? Close the PR
as WONTFIX instead? OK for trunk?
Regards
Thomas
2017-09-28 Thomas Koenig <[email protected]>
PR fortran/66756
* io/fbuf.c (fbuf_destroy): Lock unit before freeing the buffer.
* io/unit.c (insert_unit): Do not create lock and lock, move to
(gfc_get_unit): here; lock after insert_unit has succeded.
(init_units): Do not unlock unit locks for stdin, stdout and
stderr.
Index: io/fbuf.c
===================================================================
--- io/fbuf.c (Revision 253162)
+++ io/fbuf.c (Arbeitskopie)
@@ -50,9 +50,11 @@ fbuf_destroy (gfc_unit *u)
{
if (u->fbuf == NULL)
return;
+ __gthread_mutex_lock (&u->lock);
free (u->fbuf->buf);
free (u->fbuf);
u->fbuf = NULL;
+ __gthread_mutex_unlock (&u->lock);
}
Index: io/unit.c
===================================================================
--- io/unit.c (Revision 253162)
+++ io/unit.c (Arbeitskopie)
@@ -221,23 +221,14 @@ insert (gfc_unit *new, gfc_unit *t)
return t;
}
+/* insert_unit()-- Create a new node, insert it into the treap. It is assumed
+ that the caller holds unit_lock. */
-/* insert_unit()-- Create a new node, insert it into the treap. */
-
static gfc_unit *
insert_unit (int n)
{
gfc_unit *u = xcalloc (1, sizeof (gfc_unit));
u->unit_number = n;
-#ifdef __GTHREAD_MUTEX_INIT
- {
- __gthread_mutex_t tmp = __GTHREAD_MUTEX_INIT;
- u->lock = tmp;
- }
-#else
- __GTHREAD_MUTEX_INIT_FUNCTION (&u->lock);
-#endif
- __gthread_mutex_lock (&u->lock);
u->priority = pseudo_random ();
unit_root = insert (u, unit_root);
return u;
@@ -361,9 +352,20 @@ retry:
if (created)
{
- /* Newly created units have their lock held already
- from insert_unit. Just unlock UNIT_LOCK and return. */
+#ifdef __GTHREAD_MUTEX_INIT
+ {
+ __gthread_mutex_t tmp = __GTHREAD_MUTEX_INIT;
+ p->lock = tmp;
+ }
+#else
+ __GTHREAD_MUTEX_INIT_FUNCTION (&p->lock);
+#endif
__gthread_mutex_unlock (&unit_lock);
+
+ /* Nobody outside this address has seen this unit yet. We could safely
+ keep it unlocked until now. */
+
+ __gthread_mutex_lock (&p->lock);
return p;
}
@@ -618,8 +620,6 @@ init_units (void)
u->filename = strdup (stdin_name);
fbuf_init (u, 0);
-
- __gthread_mutex_unlock (&u->lock);
}
if (options.stdout_unit >= 0)
@@ -649,8 +649,6 @@ init_units (void)
u->filename = strdup (stdout_name);
fbuf_init (u, 0);
-
- __gthread_mutex_unlock (&u->lock);
}
if (options.stderr_unit >= 0)
@@ -680,8 +678,6 @@ init_units (void)
fbuf_init (u, 256); /* 256 bytes should be enough, probably not doing
any kind of exotic formatting to stderr. */
-
- __gthread_mutex_unlock (&u->lock);
}
/* Calculate the maximum file offset in a portable manner.
