[patch, libfortran] Fix thead sanitizer issue with libgfortran

[Thomas Koenig <tkoenig@netcologne.de>]

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

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  <tkoenig@gcc.gnu.org>

         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.

[-- Attachment #2: p3a.diff --]
[-- Type: text/x-patch, Size: 2476 bytes --]

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.