From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29104 invoked by alias); 29 Sep 2017 06:03:56 -0000 Mailing-List: contact fortran-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: fortran-owner@gcc.gnu.org Received: (qmail 29076 invoked by uid 89); 29 Sep 2017 06:03:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=locked X-Spam-User: qpsmtpd, 2 recipients X-HELO: cc-smtpout3.netcologne.de Received: from cc-smtpout3.netcologne.de (HELO cc-smtpout3.netcologne.de) (89.1.8.213) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 29 Sep 2017 06:03:53 +0000 Received: from cc-smtpin3.netcologne.de (cc-smtpin3.netcologne.de [89.1.8.203]) by cc-smtpout3.netcologne.de (Postfix) with ESMTP id C6662129DA; Fri, 29 Sep 2017 08:03:48 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by cc-smtpin3.netcologne.de (Postfix) with ESMTP id B9A8611D87; Fri, 29 Sep 2017 08:03:48 +0200 (CEST) Received: from [78.35.161.68] (helo=cc-smtpin3.netcologne.de) by localhost with ESMTP (eXpurgate 4.1.9) (envelope-from ) id 59cde244-02b7-7f0000012729-7f000001c250-1 for ; Fri, 29 Sep 2017 08:03:48 +0200 Received: from [192.168.178.20] (xdsl-78-35-161-68.netcologne.de [78.35.161.68]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by cc-smtpin3.netcologne.de (Postfix) with ESMTPSA; Fri, 29 Sep 2017 08:03:47 +0200 (CEST) Subject: Re: [patch, libfortran] Fix thead sanitizer issue with libgfortran From: Thomas Koenig To: "fortran@gcc.gnu.org" , gcc-patches References: <43bc4547-abe8-312d-e66e-ad9e2d3034bd@netcologne.de> Message-ID: Date: Fri, 29 Sep 2017 06:03:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <43bc4547-abe8-312d-e66e-ad9e2d3034bd@netcologne.de> Content-Type: multipart/mixed; boundary="------------35D7795AC9E095C6ADB5F9E9" X-SW-Source: 2017-09/txt/msg00131.txt.bz2 This is a multi-part message in MIME format. --------------35D7795AC9E095C6ADB5F9E9 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-length: 1064 I wrote: > 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. > Here is an update: The previous version of the patch had a regression, which is removed (with a test case) with the current version. Also regression-tested. So, > Comments? Suggestions for improvements/other approaches? Close the PR > as WONTFIX instead? OK for trunk? Regards Thomas 2017-09-29 Thomas Koenig PR fortran/66756 * io/fbuf.c (fbuf_destroy): Add argument "locked". Lock unit before freeing the buffer if necessary. * 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. 2017-09-29 Thomas Koenig PR fortran/66756 * gfortran.dg/openmp-close.f90: New test. --------------35D7795AC9E095C6ADB5F9E9 Content-Type: text/x-patch; name="p4.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="p4.diff" Content-length: 3241 Index: io/fbuf.c =================================================================== --- io/fbuf.c (Revision 253162) +++ io/fbuf.c (Arbeitskopie) @@ -46,13 +46,17 @@ fbuf_init (gfc_unit *u, int len) void -fbuf_destroy (gfc_unit *u) +fbuf_destroy (gfc_unit *u, int locked) { if (u->fbuf == NULL) return; + if (locked) + __gthread_mutex_lock (&u->lock); free (u->fbuf->buf); free (u->fbuf); u->fbuf = NULL; + if (locked) + __gthread_mutex_unlock (&u->lock); } Index: io/fbuf.h =================================================================== --- io/fbuf.h (Revision 253162) +++ io/fbuf.h (Arbeitskopie) @@ -47,7 +47,7 @@ struct fbuf extern void fbuf_init (gfc_unit *, int); internal_proto(fbuf_init); -extern void fbuf_destroy (gfc_unit *); +extern void fbuf_destroy (gfc_unit *, int); internal_proto(fbuf_destroy); extern int fbuf_reset (gfc_unit *); 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. @@ -719,7 +715,7 @@ close_unit_1 (gfc_unit *u, int locked) u->filename = NULL; free_format_hash_table (u); - fbuf_destroy (u); + fbuf_destroy (u, locked); if (u->unit_number <= NEWUNIT_START) newunit_free (u->unit_number); --------------35D7795AC9E095C6ADB5F9E9 Content-Type: text/x-fortran; name="openmp-close.f90" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="openmp-close.f90" Content-length: 247 ! { dg-do run } ! { dg-require-effective-target fopenmp } ! { dg-additional-options "-fopenmp" } program main use omp_lib !$OMP PARALLEL NUM_THREADS(100) write (10,*) 'asdf' !$OMP END PARALLEL close(10,status="delete") end program main --------------35D7795AC9E095C6ADB5F9E9--