diff mbox

[libgfortran] proposed fix for SPEC CPU2017 621.wrf_s failures

Message ID CABXYE2W97i1hxp=o17wjjezekDUzonLM3DjP2aHOfKMQd5=zSg@mail.gmail.com
State New
Headers show

Commit Message

Jim Wilson June 26, 2017, 12:50 a.m. UTC 
As mentioned in bug 81195, I see openmp related failures due to a lack
of locking of the newunit_stack and newunit_tos variables.  The code
locks when pushing onto the stack, but does not lock when popping from
the stack.  This can cause multiple threads to pop the same structure,
which then eventually leads to an error.

This patch was tested with an aarch64 bootstrap and make check.  There
were no regressions.  It was also tested with a SPEC CPU2017 run, and
solves the 621.wrf_s failure I get without the patch.

gcc 7 has the same problem.  gcc 6 is OK.

Jim

Comments

Jerry DeLisle June 26, 2017, 4:14 a.m. UTC | #1 
On 06/25/2017 05:50 PM, Jim Wilson wrote:
> As mentioned in bug 81195, I see openmp related failures due to a lack

> of locking of the newunit_stack and newunit_tos variables.  The code

> locks when pushing onto the stack, but does not lock when popping from

> the stack.  This can cause multiple threads to pop the same structure,

> which then eventually leads to an error.

> 

> This patch was tested with an aarch64 bootstrap and make check.  There

> were no regressions.  It was also tested with a SPEC CPU2017 run, and

> solves the 621.wrf_s failure I get without the patch.

> 

> gcc 7 has the same problem.  gcc 6 is OK.

> 

> Jim

> 


OK to commit for trunk and 7. Thanks for spotting this,

Jerry
diff mbox

Patch

	libgfortran/
	PR libfortran/81195
	* io/unit.c (get_unit): Call __gthread_mutex_lock before newunit_stack
	and newunit_tos references.  Call __gthread_mutex_unlock afterward.

Index: libgfortran/io/unit.c
===================================================================
--- libgfortran/io/unit.c	(revision 249613)
+++ libgfortran/io/unit.c	(working copy)
@@ -583,14 +583,17 @@ 
 	}
       else
 	{
+	  __gthread_mutex_lock (&unit_lock);
 	  if (newunit_tos)
 	    {
 	      dtp->common.unit = newunit_stack[newunit_tos].unit_number;
 	      unit = newunit_stack[newunit_tos--].unit;
+	      __gthread_mutex_unlock (&unit_lock);
 	      unit->fbuf->act = unit->fbuf->pos = 0;
 	    }
 	  else
 	    {
+	      __gthread_mutex_unlock (&unit_lock);
 	      dtp->common.unit = newunit_alloc ();
 	      unit = xcalloc (1, sizeof (gfc_unit));
 	      fbuf_init (unit, 128);
@@ -603,12 +606,15 @@ 
   /* If an internal unit number is passed from the parent to the child
      it should have been stashed on the newunit_stack ready to be used.
      Check for it now and return the internal unit if found.  */
+  __gthread_mutex_lock (&unit_lock);
   if (newunit_tos && (dtp->common.unit <= NEWUNIT_START)
       && (dtp->common.unit == newunit_stack[newunit_tos].unit_number))
     {
       unit = newunit_stack[newunit_tos--].unit;
+      __gthread_mutex_unlock (&unit_lock);
       return unit;
     }
+  __gthread_mutex_unlock (&unit_lock);
 
   /* Has to be an external unit.  */
   dtp->u.p.unit_is_internal = 0;