Fix for mistake done by https://reviews.llvm.org/D23115

[AndreyChurbanov]

Summary

Condition (team_serial || tasking_ser) was mistakenly replaced with task_serial. As a result serialized tasks in active parallel region got lost causing memory leak.
This patch restores the original condition leaving the rest of the D23115 in place. The temp variable also renamed to correspond with the condition.

[Hahnfeld]

LGTM, I think this was left over from my previous tries to fix the problem.

When exactly does this happen? Something like this?

#pragma omp parallel num_threads(2)
#pragma omp single
{
  #pragma omp task if(0)
  { }
}

Would it be possible to add a test case for this? Maybe with a destructor of a firstprivate C++ object?

[AndreyChurbanov]

I haven't debugged your particular example, probably it should have the problem, you cannot see it without debugger of some memory checker tool. We saw it on another pattern - when each thread launches tons of tasks so that all above first 256 were serialized in each thread (as the task queue is full) and the test crashes because of lack of memory after a long time execution.

I doubt writing small test case is feasible. User level code works with and without the fix, as the leak is in the library internals. Compiler-generated code also works in both cases.

We found the bug accidentally on Spec OMP2012 gross tests, but the failure we saw took about an hour of running before the crash on our machine. On small tests the problem is not visible.

[Hahnfeld]

Agreed. So we would need to run the runtime with a leak checker...

src/kmp_tasking.c

 // taskdata: task to free
 // thread: thread data structure of caller
 
 static void
 __kmp_free_task_and_ancestors( kmp_int32 gtid, kmp_taskdata_t * taskdata, kmp_info_t * thread )
 {
     // Proxy tasks must always be allowed to free their parents
     // because they can be run in background even in serial mode.
-    kmp_int32 task_serial = taskdata->td_flags.task_serial && !taskdata->td_flags.proxy;
+    kmp_int32 team_serial = ( taskdata->td_flags.team_serial ||
+        taskdata->td_flags.tasking_ser ) && !taskdata->td_flags.proxy;
     KMP_DEBUG_ASSERT( taskdata -> td_flags.tasktype == TASK_EXPLICIT );
 
     kmp_int32 children = KMP_TEST_THEN_DEC32( (kmp_int32 *)(& taskdata -> td_allocated_child_tasks) ) - 1;
     KMP_DEBUG_ASSERT( children >= 0 );
 
     // Now, go up the ancestor tree to see if any ancestors can now be freed.
     while ( children == 0 )
     {
         kmp_taskdata_t * parent_taskdata = taskdata -> td_parent;
 
         KA_TRACE(20, ("__kmp_free_task_and_ancestors(enter): T#%d task %p complete "
                       "and freeing itself\n", gtid, taskdata) );
 
         // --- Deallocate my ancestor task ---
         __kmp_free_task( gtid, taskdata, thread );
 
         taskdata = parent_taskdata;
 
         // Stop checking ancestors at implicit task
         // instead of walking up ancestor tree to avoid premature deallocation of ancestors.
-        if ( task_serial || taskdata -> td_flags.tasktype == TASK_IMPLICIT )
+        if ( team_serial || taskdata -> td_flags.tasktype == TASK_IMPLICIT )
             return;
 
         // Predecrement simulated by "- 1" calculation
         children = KMP_TEST_THEN_DEC32( (kmp_int32 *)(& taskdata -> td_allocated_child_tasks) ) - 1;
         KMP_DEBUG_ASSERT( children >= 0 );
     }
 
     KA_TRACE(20, ("__kmp_free_task_and_ancestors(exit): T#%d task %p has %d children; "