libgomp: Fix up GOMP_task on s390x

On Wed, Jan 20, 2021 at 05:04:39PM +0100, Florian Weimer wrote:
> Sorry, this appears to cause OpenMP task state corruption in RPM.  We
> have only seen this on s390x.

Haven't actually verified it, but my suspection is that this is a caller
stack corruption.

We play with fire with the GOMP_task API/ABI extensions, the GOMP_task
function used to be:
void
GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
           long arg_size, long arg_align, bool if_clause, unsigned flags);
and later:
void
GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
           long arg_size, long arg_align, bool if_clause, unsigned flags,
           void **depend);
and later:
void
GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
           long arg_size, long arg_align, bool if_clause, unsigned flags,
           void **depend, int priority);
and now:
void
GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
           long arg_size, long arg_align, bool if_clause, unsigned flags,
           void **depend, int priority, void *detach)
and which of those depend, priority and detach argument is present depends
on the bits in flags.
I'm afraid the compiler just decided to spill the detach = NULL store in
  if ((flags & GOMP_TASK_FLAG_DETACH) == 0)
    detach = NULL;
on s390x into the argument stack slot.  Not a problem if the caller passes
all those 10 arguments, but if not, can clobber random stack location.

This hack should fix it up.  Priority doesn't need changing, but I've
changed it anyway just to be safe.  With the patch none of the 3 arguments
are ever modified, so I'd hope gcc doesn't decide to spill something
unrelated there.

2021-01-20  Jakub Jelinek  <jakub@redhat.com>

	* task.c (GOMP_task): Rename priority argument to priority_arg,
	add priority automatic variable and modify that variable.  Instead of
	clearing detach argument when GOMP_TASK_FLAG_DETACH bit is not set,
	check flags for that bit.

(cherry picked from commit 0bb27b81a762d3c607bd25409337c749f836c0cd)

diff --git a/libgomp/ChangeLog.omp b/libgomp/ChangeLog.omp
index 3518bc8eae518cd0d47ffd818b87ab86f7c11326..dbaa30d3381d3fda01b660120c57ea6ba09c0e09 100644 (file)
--- a/libgomp/ChangeLog.omp
+++ b/libgomp/ChangeLog.omp
@@ -1,3 +1,13 @@
+2021-01-22  Kwok Cheung Yeung  <kcy@codesourcery.com>
+
+       Backport from mainline
+       2021-01-20  Jakub Jelinek  <jakub@redhat.com>
+
+       * task.c (GOMP_task): Rename priority argument to priority_arg,
+       add priority automatic variable and modify that variable.  Instead of
+       clearing detach argument when GOMP_TASK_FLAG_DETACH bit is not set,
+       check flags for that bit.
+
 2021-01-22  Kwok Cheung Yeung  <kcy@codesourcery.com>
 
        Backport from mainline

diff --git a/libgomp/task.c b/libgomp/task.c
index 9689a3edef099d2c1faf28b2bbe26622587f086d..78627726e0c813c30a1c3e7d308bf663cd5f5000 100644 (file)
--- a/libgomp/task.c
+++ b/libgomp/task.c
@@ -354,10 +354,11 @@ task_fulfilled_p (struct gomp_task *task)
 void
 GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
           long arg_size, long arg_align, bool if_clause, unsigned flags,
-          void **depend, int priority, void *detach)
+          void **depend, int priority_arg, void *detach)
 {
   struct gomp_thread *thr = gomp_thread ();
   struct gomp_team *team = thr->ts.team;
+  int priority = 0;
 
 #ifdef HAVE_BROKEN_POSIX_SEMAPHORES
   /* If pthread_mutex_* is used for omp_*lock*, then each task must be
@@ -385,13 +386,12 @@ GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
        }
     }
 
-  if ((flags & GOMP_TASK_FLAG_PRIORITY) == 0)
-    priority = 0;
-  else if (priority > gomp_max_task_priority_var)
-    priority = gomp_max_task_priority_var;
-
-  if ((flags & GOMP_TASK_FLAG_DETACH) == 0)
-    detach = NULL;
+  if (__builtin_expect ((flags & GOMP_TASK_FLAG_PRIORITY) != 0, 0))
+    {
+      priority = priority_arg;
+      if (priority > gomp_max_task_priority_var)
+       priority = gomp_max_task_priority_var;
+    }
 
   if (!if_clause || team == NULL
       || (thr->task && thr->task->final_task)
@@ -415,7 +415,7 @@ GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
                        || (flags & GOMP_TASK_FLAG_FINAL);
       task.priority = priority;
 
-      if (detach)
+      if ((flags & GOMP_TASK_FLAG_DETACH) != 0)
        {
          task.detach = true;
          gomp_sem_init (&task.completion_sem, 0);
@@ -443,7 +443,7 @@ GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
       else
        fn (data);
 
-      if (detach && !task_fulfilled_p (&task))
+      if (task.detach && !task_fulfilled_p (&task))
        gomp_sem_wait (&task.completion_sem);
 
       /* Access to "children" is normally done inside a task_lock
@@ -484,7 +484,7 @@ GOMP_task (void (*fn) (void *), void *data, void (*cpyfn) (void *, void *),
       task->kind = GOMP_TASK_UNDEFERRED;
       task->in_tied_task = parent->in_tied_task;
       task->taskgroup = taskgroup;
-      if (detach)
+      if ((flags & GOMP_TASK_FLAG_DETACH) != 0)
        {
          task->detach = true;
          gomp_sem_init (&task->completion_sem, 0);