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

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.

Reviewed By: AndreyChurbanov

Differential Revision: https://reviews.llvm.org/D79702

openmp/runtime/src/kmp_tasking.cpp

@@ -861,7 +861,37 @@ static void __kmp_task_finish(kmp_int32 gtid, kmp_task_t *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
+  }
+
+  /* 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 ==
@@ -870,14 +900,17 @@ static void __kmp_task_finish(kmp_int32 gtid, kmp_task_t *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
@@ -897,45 +930,19 @@ static void __kmp_task_finish(kmp_int32 gtid, kmp_task_t *task,
       // 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
+    KMP_DEBUG_ASSERT(taskdata->td_flags.executing == 1);
+    taskdata->td_flags.executing = 0; // suspend the finishing task
   }
 
-  // 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
-  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
@@ -3847,20 +3854,14 @@ void __kmp_fulfill_event(kmp_event_t *event) {
     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.
+    // The associated task might have completed or 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;
-    } 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);
-    }
+    event->type = KMP_EVENT_UNINITIALIZED;
+    __kmp_release_tas_lock(&event->lock, gtid);
 
     if (detached) {
       // If the task detached complete the proxy task