Hi!
On 2021-01-20T17:40:58+0100, Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
> 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)
Yeah, I'd wondered about that, too.
> 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.
That still seems fragile; is "hope gcc doesn't decide to spill" really
sufficient?
Cannot we (easily) use symbol versioning to introduce new entry point
variants (which then internally all route to the same function)?
Grüße
Thomas
> 2021-01-20 Jakub Jelinek <ja...@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.
>
> --- libgomp/task.c.jj 2021-01-18 07:18:42.362339622 +0100
> +++ libgomp/task.c 2021-01-20 17:23:36.973758174 +0100
> @@ -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 *da
> }
> }
>
> - 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 *da
> || (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 *da
> 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 *da
> 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);
>
>
> Jakub
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander
Walter
