[OpenMP] Fix race condition in the completion/freeing of detached tasks

[protze.joachim]

Summary

Spurious assertion failures are symptoms of a race condition for the handling of detached tasks:
Assertion failure at kmp_tasking.cpp(3744): taskdata->td_flags.complete == 1.
Assertion failure at kmp_tasking.cpp(710): taskdata->td_flags.executing == 0.

in the case of detach=true, all accesses to taskdata in __kmp_task_finish need to happen before (~line 873):

taskdata->td_flags.proxy = TASK_PROXY;

This assignment signals to __kmp_fulfill_event, that the task will need to be freed there. So, conceptionally the ownership of taskdata is moved.

Is it safe to move the call to the destructors up in the code?
Do we need to acquire the lock in __kmp_fulfill_event to make sure the lock is released in __kmp_task_finish, before taskdata is freed?

[protze.joachim]

Applied clang-format

Take the lock in __kmp_fulfill_event to avoid the race between unlocking in __kmp_task_finish and freeing taskdata (which contains the lock).

[AndreyChurbanov]

LGTM

Joachim, thanks for taking care of this bug.

[AndreyChurbanov]

openmp/runtime/src/kmp_tasking.cpp

900

completion_lock looks a better name than competion_lock (typo?).

[protze.joachim]

Fixed the typo by reverting this local change

[protze.joachim]

openmp/runtime/src/kmp_tasking.cpp

900

I revert this change to use a temporal variable (not sure whether use of auto is accepted in general). With the locking in kmp_fulfill_event this is not needed.

openmp/runtime/src/kmp_tasking.cpp

if (node && (node->dn.mtx_num_locks < 0)) {
     // negative num_locks means all locks were acquired
     node->dn.mtx_num_locks = -node->dn.mtx_num_locks;
     for (int i = node->dn.mtx_num_locks - 1; i >= 0; --i) {
       KMP_DEBUG_ASSERT(node->dn.mtx_locks[i] != NULL);
       __kmp_release_lock(node->dn.mtx_locks[i], gtid);
     }
   }
 
+  // bookkeeping for resuming task:
+  // GEH - note tasking_ser => task_serial
+  KMP_DEBUG_ASSERT(
+      (taskdata->td_flags.tasking_ser || taskdata->td_flags.task_serial) ==
+      taskdata->td_flags.task_serial);
+  if (taskdata->td_flags.task_serial) {
+    if (resumed_task == NULL) {
+      resumed_task = taskdata->td_parent; // In a serialized task, the resumed
+      // task is the parent
+    }
+  } else {
+    KMP_DEBUG_ASSERT(resumed_task !=
+                     NULL); // verify that resumed task is passed as argument
+  }
+
+  /* If the tasks' destructor thunk flag has been set, we need to invoke the
+     destructor thunk that has been generated by the compiler. The code is
+     placed here, since at this point other tasks might have been released
+     hence overlapping the destructor invocations with some other work in the
+     released tasks.  The OpenMP spec is not specific on when the destructors
+     are invoked, so we should be free to choose. */
+  if (taskdata->td_flags.destructors_thunk) {
+    kmp_routine_entry_t destr_thunk = task->data1.destructors;
+    KMP_ASSERT(destr_thunk);
+    destr_thunk(gtid, task);
+  }
+
   KMP_DEBUG_ASSERT(taskdata->td_flags.complete == 0);
+  KMP_DEBUG_ASSERT(taskdata->td_flags.started == 1);
+  KMP_DEBUG_ASSERT(taskdata->td_flags.freed == 0);
+
   bool detach = false;
   if (taskdata->td_flags.detachable == TASK_DETACHABLE) {
     if (taskdata->td_allow_completion_event.type ==
         KMP_EVENT_ALLOW_COMPLETION) {
       // event hasn't been fulfilled yet. Try to detach task.
       __kmp_acquire_tas_lock(&taskdata->td_allow_completion_event.lock, gtid);
       if (taskdata->td_allow_completion_event.type ==
           KMP_EVENT_ALLOW_COMPLETION) {
+        // task finished execution
+        KMP_DEBUG_ASSERT(taskdata->td_flags.executing == 1);
+        taskdata->td_flags.executing = 0; // suspend the finishing task
+        // no access to taskdata after this point!
+        // __kmp_fulfill_event might free taskdata at any time from now
         taskdata->td_flags.proxy = TASK_PROXY; // proxify!
         detach = true;
       }
       __kmp_release_tas_lock(&taskdata->td_allow_completion_event.lock, gtid);
     }
   }
-  KMP_DEBUG_ASSERT(taskdata->td_flags.started == 1);
-  KMP_DEBUG_ASSERT(taskdata->td_flags.freed == 0);
 
   if (!detach) {
     taskdata->td_flags.complete = 1; // mark the task as completed
 
     // Only need to keep track of count if team parallel and tasking not
     // serialized
     if (!(taskdata->td_flags.team_serial || taskdata->td_flags.tasking_ser)) {
       // Predecrement simulated by "- 1" calculation
       children =
           KMP_ATOMIC_DEC(&taskdata->td_parent->td_incomplete_child_tasks) - 1;
       KMP_DEBUG_ASSERT(children >= 0);
       if (taskdata->td_taskgroup)
         KMP_ATOMIC_DEC(&taskdata->td_taskgroup->count);
       __kmp_release_deps(gtid, taskdata);
     } else if (task_team && task_team->tt.tt_found_proxy_tasks) {
       // if we found proxy tasks there could exist a dependency chain
       // with the proxy task as origin
       __kmp_release_deps(gtid, taskdata);
     }
-  }
-
     // td_flags.executing must be marked as 0 after __kmp_release_deps has been
-  // called. Othertwise, if a task is executed immediately from the release_deps
-  // code, the flag will be reset to 1 again by this same function
+    // called. Othertwise, if a task is executed immediately from the
+    // release_deps code, the flag will be reset to 1 again by this same
+    // function
     KMP_DEBUG_ASSERT(taskdata->td_flags.executing == 1);
     taskdata->td_flags.executing = 0; // suspend the finishing task
+  }
+
 
   KA_TRACE(
       20, ("__kmp_task_finish: T#%d finished task %p, %d incomplete children\n",
            gtid, taskdata, children));
 
-  /* If the tasks' destructor thunk flag has been set, we need to invoke the
-     destructor thunk that has been generated by the compiler. The code is
-     placed here, since at this point other tasks might have been released
-     hence overlapping the destructor invocations with some other work in the
-     released tasks.  The OpenMP spec is not specific on when the destructors
-     are invoked, so we should be free to choose. */
-  if (taskdata->td_flags.destructors_thunk) {
-    kmp_routine_entry_t destr_thunk = task->data1.destructors;
-    KMP_ASSERT(destr_thunk);
-    destr_thunk(gtid, task);
-  }
-
-  // bookkeeping for resuming task:
-  // GEH - note tasking_ser => task_serial
-  KMP_DEBUG_ASSERT(
-      (taskdata->td_flags.tasking_ser || taskdata->td_flags.task_serial) ==
-      taskdata->td_flags.task_serial);
-  if (taskdata->td_flags.task_serial) {
-    if (resumed_task == NULL) {
-      resumed_task = taskdata->td_parent; // In a serialized task, the resumed
-      // task is the parent
-    }
-  } else {
-    KMP_DEBUG_ASSERT(resumed_task !=
-                     NULL); // verify that resumed task is passed as argument
-  }
-
   // Free this task and then ancestor tasks if they have no children.
   // Restore th_current_task first as suggested by John:
   // johnmc: if an asynchronous inquiry peers into the runtime system
   // it doesn't see the freed task as the current task.
   thread->th.th_current_task = resumed_task;
   if (!detach)
     __kmp_free_task_and_ancestors(gtid, taskdata, thread);
 

 
 void __kmp_fulfill_event(kmp_event_t *event) {
   if (event->type == KMP_EVENT_ALLOW_COMPLETION) {
     kmp_task_t *ptask = event->ed.task;
     kmp_taskdata_t *taskdata = KMP_TASK_TO_TASKDATA(ptask);
     bool detached = false;
     int gtid = __kmp_get_gtid();
 
-    if (taskdata->td_flags.proxy == TASK_PROXY) {
-      // The associated task code completed before this call and detached.
-      detached = true;
-      event->type = KMP_EVENT_UNINITIALIZED;
-    } else {
-      // The associated task has not completed but could be completing at this
-      // point.
-      // We need to take the lock to avoid races
-      __kmp_acquire_tas_lock(&event->lock, gtid);
-      if (taskdata->td_flags.proxy == TASK_PROXY)
-        detached = true;
-      event->type = KMP_EVENT_UNINITIALIZED;
-      __kmp_release_tas_lock(&event->lock, gtid);
-    }
+    // The associated task might have completed or could be completing at this
 
     if (detached) {
       // If the task detached complete the proxy task
       if (gtid >= 0) {
         kmp_team_t *team = taskdata->td_team;
         kmp_info_t *thread = __kmp_get_thread();
         if (thread->th.th_team == team) {
           __kmpc_proxy_task_completed(gtid, ptask);