public inbox for fortran@gcc.gnu.org
 help / color / mirror / Atom feed
* [patch, libfortran] Fix thead sanitizer issue with libgfortran
@ 2017-09-28 17:29 Thomas Koenig
  2017-09-29  6:03 ` Thomas Koenig
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Koenig @ 2017-09-28 17:29 UTC (permalink / raw)
  To: fortran, gcc-patches

[-- 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.

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [patch, libfortran] Fix thead sanitizer issue with libgfortran
@ 2017-10-01  9:02 Bernd Edlinger
  2017-10-01 13:41 ` Thomas Koenig
  0 siblings, 1 reply; 9+ messages in thread
From: Bernd Edlinger @ 2017-10-01  9:02 UTC (permalink / raw)
  To: Janne Blomqvist; +Cc: Thomas Koenig, fortran, gcc-patches

Hi,

I think this might be a false positive tsan warning.
Maybe tsan does not see why a desctructor function
can not execute before the last detached thread
terminates.

I created a small test case that receives a warning
which is similar to the one you tried to fix with your
patch:

cat test.c
#include <stdio.h>
#include <pthread.h>

int test;

static void *
t1(void* p)
{
   printf("t1\n");
   test = 1;
   return NULL;
}

static void __attribute__((destructor))
t2 (void)
{
   test = 2;
   printf("t2\n");
}

int
main()
{
   pthread_t t;
   pthread_attr_t a;
   pthread_attr_init(&a);
   pthread_attr_setdetachstate(&a, PTHREAD_CREATE_DETACHED);
   pthread_create(&t, &a, t1, NULL);
   pthread_attr_destroy(&a);
   return 0;
}

gcc -Wall -fsanitize=thread test.c
./a.out
t1
t2
==================
WARNING: ThreadSanitizer: data race (pid=3564)
   Write of size 4 at 0x00000060107c by thread T1:
     #0 t1 <null> (a.out+0x00000040088e)

   Previous write of size 4 at 0x00000060107c by main thread:
     #0 t2 <null> (a.out+0x0000004008c6)
     #1 <null> <null> (ld-linux-x86-64.so.2+0x0000000108d9)

   Location is global 'test' of size 4 at 0x00000060107c 
(a.out+0x00000060107c)

   Thread T1 (tid=3566, running) created by main thread at:
     #0 pthread_create 
../../../../gcc-trunk/libsanitizer/tsan/tsan_interceptors.cc:900 
(libtsan.so.0+0x00000002914e)
     #1 main <null> (a.out+0x00000040092e)

SUMMARY: ThreadSanitizer: data race 
(/home/ed/gnu/gcc-test/a.out+0x40088e) in t1
==================
ThreadSanitizer: reported 1 warnings


The warning goes away when the main function explicitly
joins the thread t1.

That is what I do with all my threads whenever possible,
maybe there is a way how you could explicitly join
all running threads?


Bernd.

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-10-01 16:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-28 17:29 [patch, libfortran] Fix thead sanitizer issue with libgfortran Thomas Koenig
2017-09-29  6:03 ` Thomas Koenig
2017-09-29  8:04   ` Janne Blomqvist
2017-09-29 18:53     ` Thomas Koenig
2017-10-01  7:42       ` Janne Blomqvist
2017-10-01  9:02 Bernd Edlinger
2017-10-01 13:41 ` Thomas Koenig
2017-10-01 14:23   ` Bernd Edlinger
2017-10-01 16:54     ` Thomas Koenig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).