[OpenMP][Archer] Avoid false positive for OpenMP tasking

[protze.joachim]

Summary

OpenMP tasks can be understood as asynchronous function calls where shared variables are passed by reference and firstprivate variables are passed by value. The OpenMP runtime needs to store these arguments from the instantiation/creation to the asynchronous execution. libomp stores these values along with the internal task data structure that is allocated from the internal memory manager.
During task creation, compiler generated code copies the values into the task object. At the beginning of task execution, compiler generated loads these values to the stack. Due to the memory management, the task object will be reused after the task finished execution. The load during task execution is not synchronized by OpenMP semantics with the store during task creation with reused task object. The memory manager provides the synchronization.

The newly added test demonstrates the false positive report with current LLVM.

This patch adds annotation for the new semantics of the task object. The patch depends on two other patches for TSan runtime and OpenMP runtime.

[Hahnfeld]

The annotation is in endTask because in startTask there may already be data filled by the runtime?

openmp/tools/archer/ompt-tsan.cpp

485

Can adjust the comment style to be consistent with the others?

971

Do we need this extra check? I think PrivateDataSize is guaranteed 0 if Archer is unable to get this information, right?

[protze.joachim]

Addressing review comments

[protze.joachim]

Between creation and execution of the task we have an explicit happens-before edge.
The false positive comes from the access to the private data during execution and reuse of the memory for the next task. startTask is be before the execution of the task.
We need to store the memory information, because only in startTask we can query the task memory according to OMPT. In endTask we would get the information for the task that reached the task scheduling point where our task of interest was scheduled.

[Hahnfeld]

Ok; I still don't understand though why Archer needs to call TsanNewMemory both in startTask and again for the last block in endTask. Does the LLVM OpenMP runtime guarantee that there is only one range, or that the last range is "special" in some way?

openmp/tools/archer/ompt-tsan.cpp

 
 // Function entry/exit
 #define TsanFuncEntry(pc) __tsan_func_entry(pc)
 #define TsanFuncExit() __tsan_func_exit()
 
 /// Required OMPT inquiry functions.
 static ompt_get_parallel_info_t ompt_get_parallel_info;
 static ompt_get_thread_data_t ompt_get_thread_data;
+typedef int (*ompt_get_task_memory_t)(void **addr, size_t *size, int blocknum);
+static ompt_get_task_memory_t ompt_get_task_memory;
 
 typedef char ompt_tsan_clockid;
 
 static uint64_t my_next_id() {
   static uint64_t ID = 0;
   uint64_t ret = __sync_fetch_and_add(&ID, 1);
   return ret;
 }
 
+namespace {
 static int pagesize{0};
 
 // Data structure to provide a threadsafe pool of reusable objects.
 // DataPool<Type of objects>
-namespace {
 template <typename T> struct DataPool final {
   static __thread DataPool<T> *ThreadDataPool;
   std::mutex DPMutex{};
 
   // store unused objects
   std::vector<T *> DataPointer{};
   std::vector<T *> RemoteDataPointer{};
 
struct TaskData final : DataPoolEntry<TaskData> {
   char BarrierIndex{0};
 
   /// Whether this task is currently executing a barrier.
   bool InBarrier{false};
 
   /// Whether this task is an included task.
   int TaskType{0};
 
-  /// count execution phase
+  /// Count execution phase
   int execution{0};
 
   /// Count how often this structure has been put into child tasks + 1.
   std::atomic_int RefCount{1};
 
   /// Reference to the parent that created this task.
   TaskData *Parent{nullptr};
 
+  /// Store private data from start to end
+  size_t PrivateDataSize{0};
+  void *PrivateDataAddr{nullptr};
+
   /// Reference to the team of this task.
   ParallelData *Team{nullptr};
 
   /// Reference to the current taskgroup that this task either belongs to or
   /// that it just created.
   Taskgroup *TaskGroup{nullptr};
 
   /// Dependency information for this task.
if (DependencyMap) {
         i.second->Delete();
       delete DependencyMap;
     }
     DependencyMap = nullptr;
     if (Dependencies)
       free(Dependencies);
     Dependencies = nullptr;
     DependencyCount = 0;
+    PrivateDataSize = 0;
+    PrivateDataAddr = nullptr;
 #ifdef DEBUG
     freed = 0;
 #endif
   }
 
   static TaskData *New(TaskData *parent, int taskType) {
     return DataPoolEntry<TaskData>::New()->Init(parent, taskType);
   }
static void switchTasks(TaskData *FromTask, TaskData *ToTask) {
   //    FromTask->deactivate();
   //  if (ToTask)
   //    ToTask->activate();
 }
 
 static void endTask(TaskData *FromTask) {
   if (!FromTask)
     return;
+  if (FromTask->PrivateDataSize > 0)
+    TsanNewMemory(FromTask->PrivateDataAddr, FromTask->PrivateDataSize);
 }
 
 static void startTask(TaskData *ToTask) {
   if (!ToTask)
     return;
   // Handle dependencies on first execution of the task
   if (ToTask->execution == 0) {
     ToTask->execution++;
+    if (ompt_get_task_memory) {
+      void *addr;
+      size_t size;
+      int ret_task_memory = 0, block = 0;
+      do {
+        size = 0;
+        ret_task_memory = ompt_get_task_memory(&addr, &size, block++);
+        if (size > 0) {
+          TsanNewMemory(addr, size);
+          ToTask->PrivateDataAddr = addr;
+          ToTask->PrivateDataSize = size;
+        }
+      } while (ret_task_memory);
+    }
     acquireDependencies(ToTask);
   }
   // 1. Task will begin execution after it has been created.
   // 2. Task will resume after it has been switched away.
   TsanHappensAfter(ToTask->GetTaskPtr());
 }
 
 static void ompt_tsan_task_schedule(ompt_data_t *first_task_data,
ompt_set_callback_t ompt_set_callback =
       (ompt_set_callback_t)lookup("ompt_set_callback");
   if (ompt_set_callback == NULL) {
     std::cerr << "Could not set callback, exiting..." << std::endl;
     std::exit(1);
   }
   ompt_get_parallel_info =
       (ompt_get_parallel_info_t)lookup("ompt_get_parallel_info");
   ompt_get_thread_data = (ompt_get_thread_data_t)lookup("ompt_get_thread_data");
+  ompt_get_task_memory = (ompt_get_task_memory_t)lookup("ompt_get_task_memory");
 
   if (ompt_get_parallel_info == NULL) {
     fprintf(stderr, "Could not get inquiry function 'ompt_get_parallel_info', "
                     "exiting...\n");
     exit(1);
   }
 
   findTsanFunction(AnnotateHappensAfter,

openmp/tools/archer/tests/task/reuse-task-storage.c

+/*
+ * reuse-task-storage.c -- Archer testcase
+ */
+
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+//
+// See tools/archer/LICENSE.txt for details.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+
+// RUN: %libarcher-compile-and-run | FileCheck %s
+// REQUIRES: tsan
+#include <omp.h>
+#include <stdio.h>
+#include <unistd.h>
+#include "ompt/ompt-signal.h"
+
+int main(int argc, char *argv[]) {
+  int var = 0, a = 0;
+
+#pragma omp parallel num_threads(8) shared(var, a)
+  {
+#pragma omp master
+    for(int i=0; i<10; i++)
+    {
+      for(int j=0; j<100; j++)
+#pragma omp task shared(var)
+      {
+        OMPT_SIGNAL(a);
+        #pragma omp atomic update
+          var++;
+      }
+
+      // Give other threads time to steal the task.
+      OMPT_WAIT(a, (i*100));
+    }
+
+  }
+
+  fprintf(stderr, "DONE\n");
+  int error = (var != 1000);
+  return error;
+}
+
+// CHECK-NOT: ThreadSanitizer: data race
+// CHECK-NOT: ThreadSanitizer: reported
+// CHECK: DONE