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

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

[Jim Wilson]

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

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

[-- Attachment #2: newunit-locking.patch --]
[-- Type: text/x-patch, Size: 1513 bytes --]

	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;

Re: [PATCH, libgfortran] proposed fix for SPEC CPU2017 621.wrf_s failures

[Jerry DeLisle]

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