[OpenMP] libomp: taskwait depend implementation fixed

[AndreyChurbanov]

Summary

Eliminated references from task dependency hash to node allocated on stack, thus eliminated accesses to stale memory. So the node now never freed.

Uncommented assertion which triggered when stale memory accessed. Removed unneeded ref count increment for stack allocated node.

[hbae]

LGTM

openmp/runtime/src/kmp_taskdeps.h

 #define KMP_ACQUIRE_DEPNODE(gtid, n) __kmp_acquire_lock(&(n)->dn.lock, (gtid))
 #define KMP_RELEASE_DEPNODE(gtid, n) __kmp_release_lock(&(n)->dn.lock, (gtid))
 
 static inline void __kmp_node_deref(kmp_info_t *thread, kmp_depnode_t *node) {
   if (!node)
     return;
 
   kmp_int32 n = KMP_ATOMIC_DEC(&node->dn.nrefs) - 1;
-  // TODO: temporarily disable assertion until the bug with dependences is fixed
-  //  KMP_DEBUG_ASSERT(n >= 0);
+  KMP_DEBUG_ASSERT(n >= 0);
   if (n == 0) {
     KMP_ASSERT(node->dn.nrefs == 0);
 #if USE_FAST_MEMORY
     __kmp_fast_free(thread, node);
 #else
     __kmp_thread_free(thread, node);
 #endif
   }

openmp/runtime/src/kmp_taskdeps.cpp

if (dep->flags.out) { // out or inout --> clean lists if any
       if (info->last_flag == 0 || info->last_flag == dep->flag) {
         // last_set either didn't exist or of same dep kind
         // link node as successor of the last_out if any
         npredecessors +=
             __kmp_depnode_link_successor(gtid, thread, task, node, last_out);
         // link node as successor of all nodes in the prev_set if any
         npredecessors +=
             __kmp_depnode_link_successor(gtid, thread, task, node, prev_set);
+        if (dep_barrier) {
+          // clean last_out and prev_set if any; don't touch last_set
+          __kmp_node_deref(thread, last_out);
+          info->last_out = NULL;
+          __kmp_depnode_list_free(thread, prev_set);
+          info->prev_set = NULL;
+        }
       } else { // last_set is of different dep kind, make it prev_set
         // link node as successor of all nodes in the last_set
         npredecessors +=
             __kmp_depnode_link_successor(gtid, thread, task, node, last_set);
         // clean last_out if any
         __kmp_node_deref(thread, last_out);
         info->last_out = NULL;
         // clean prev_set if any
         __kmp_depnode_list_free(thread, prev_set);
+        if (!dep_barrier) {
           // move last_set to prev_set, new last_set will be allocated
           info->prev_set = last_set;
+        } else {
+          info->prev_set = NULL;
+          info->last_flag = 0;
+        }
         info->last_set = NULL;
       }
+      // for dep_barrier last_flag value should remain:
+      // 0 if last_set is empty, unchanged otherwise
+      if (!dep_barrier) {
         info->last_flag = dep->flag; // store dep kind of the last_set
         info->last_set = __kmp_add_node(thread, info->last_set, node);
-
+      }
       // check if we are processing MTX dependency
       if (dep->flag == KMP_DEP_MTX) {
         if (info->mtx_lock == NULL) {
           info->mtx_lock = (kmp_lock_t *)__kmp_allocate(sizeof(kmp_lock_t));
           __kmp_init_lock(info->mtx_lock);
         }
         KMP_DEBUG_ASSERT(node->dn.mtx_num_locks < MAX_MTX_DEPS);
         kmp_int32 m;

 #if OMPT_SUPPORT
     __ompt_taskwait_dep_finish(current_task, taskwait_task_data);
 #endif /* OMPT_SUPPORT */
     return;
   }
 
   kmp_depnode_t node = {0};
   __kmp_init_node(&node);
-  // the stack owns the node
-  __kmp_node_ref(&node);
 
   if (!__kmp_check_deps(gtid, &node, NULL, &current_task->td_dephash,
                         DEP_BARRIER, ndeps, dep_list, ndeps_noalias,
                         noalias_dep_list)) {
     KA_TRACE(10, ("__kmpc_omp_wait_deps(exit): T#%d has no blocking "
                   "dependences : loc=%p\n",
                   gtid, loc_ref));
 #if OMPT_SUPPORT

openmp/runtime/test/tasking/kmp_taskwait_depend_in.c

+// RUN: %libomp-compile-and-run
+
+// test checks IN dep kind in depend clause on taskwait construct
+// uses codegen emulation
+#include <stdio.h>
+#include <omp.h>
+// ---------------------------------------------------------------------------
+// internal data to emulate compiler codegen
+typedef struct DEP {
+  size_t addr;
+  size_t len;
+  unsigned char flags;
+} _dep;
+typedef struct ID {
+  int reserved_1;
+  int flags;
+  int reserved_2;
+  int reserved_3;
+  char *psource;
+} _id;
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+extern int __kmpc_global_thread_num(_id*);
+extern void __kmpc_omp_wait_deps(_id *, int, int, _dep *, int, _dep *);
+#ifdef __cplusplus
+} // extern "C"
+#endif
+
+int main()
+{
+  int i1,i2,i3;
+  omp_set_num_threads(2);
+  printf("addresses: %p %p %p\n", &i1, &i2, &i3);
+  #pragma omp parallel
+  {
+    int t = omp_get_thread_num();
+    printf("thread %d enters parallel\n", t);
+    #pragma omp single
+    {
+      #pragma omp task depend(in: i3)
+      {
+        int th = omp_get_thread_num();
+        printf("task 0 created by th %d, executed by th %d\n", t, th);
+      }
+      #pragma omp task depend(in: i2)
+      {
+        int th = omp_get_thread_num();
+        printf("task 1 created by th %d, executed by th %d\n", t, th);
+      }
+//      #pragma omp taskwait depend(in: i1, i2)
+      {
+        _dep sdep[2];
+        static _id loc = {0, 2, 0, 0, ";test9.c;func;60;0;;"};
+        int gtid = __kmpc_global_thread_num(&loc);
+        sdep[0].addr = (size_t)&i2;
+        sdep[0].flags = 1; // 1-in, 2-out, 3-inout, 4-mtx, 8-inoutset
+        sdep[1].addr = (size_t)&i1;
+        sdep[1].flags = 1; // in
+        __kmpc_omp_wait_deps(&loc, gtid, 2, sdep, 0, NULL);
+      }
+      printf("single done\n");
+    }
+  }
+  printf("passed\n");
+  return 0;
+}