[OpenMP][FIX] Ensure exclusive access to the HDTT map

[jdoerfert]

Summary

This patch solves two problems with the HostDataToTargetMap (HDTT
map) which caused races and crashes before:

1. Any access to the HDTT map needs to be exclusive access. This was not the case for the "dump table" traversals that could collide with updates by other threads. The new Accessor and ProtectedObject wrappers will ensure we have a hard time introducing similar races in the future. Note that we could allow multiple concurrent read-accesses but that feature can be added to the Accessor API later.
2. The elements of the HDTT map were HostDataToTargetTy objects which meant that they could be copied/moved/deleted as the map was changed. However, we sometimes kept pointers to these elements around after we gave up the map lock which caused potential races again. The new indirection through HostDataToTargetMapKeyTy will allows us to modify the map while keeping the (interesting part of the) entries valid. To offset potential cost we duplicate the ordering key of the entry which avoids an additional indirect lookup.

We should replace more objects with "protected objects" as we go.

[jdoerfert]

Rename argument

[JonChesterfield]

Only comments on the helper type for now. I like the idea of a generic wrap-this-thing-in-mutex helper as a path to stepping on the remaining race conditions.

openmp/libomptarget/include/ExclusiveAccess.h

36

I don't see a way to give this thing ownership of a resource after constructing it with DoNotGetAccess. Perhaps replace that Boolean flag with a default constructor that sets the pointer to null.

42

Do we have multiple threads waiting to gain access to this thing? If not, asserting that it wasn't locked to begin with might be helpful for catching errors at the call site.

62

Null dereference looks likely here. Maybe an assert to get friendlier notice on the mistake.

84

Not sure we need the move here, it's already an R value. Also not sure we need the && on the return type.

[jdoerfert]

Addressed review comments

openmp/libomptarget/include/ExclusiveAccess.h

42

We do have multiple threads in play or this entire thing would be reasonably useless ;)

84

Because we don't want to copy it.

[JonChesterfield]

I'm ooo for a couple of days, @tianshilei1992 could you take a look?

[tianshilei1992]

LG

openmp/libomptarget/include/ExclusiveAccess.h

+//===---- ExclusiveAccess.h - Helper for exclusive access data structures -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef OMPTARGET_EXCLUSIVE_ACCESS
+#define OMPTARGET_EXCLUSIVE_ACCESS
+
+#include <cstddef>
+#include <cstdint>
+#include <mutex>
+
+/// Forward declaration.
+template <typename Ty> struct Accessor;
+
+/// A protected object is a simple wrapper to allocate an object of type \p Ty
+/// together with a mutex that guards accesses to the object. The only way to
+/// access the object is through the "exclusive accessor" which will lock the
+/// mutex accordingly.
+template <typename Ty> struct ProtectedObj {
+  using AccessorTy = Accessor<Ty>;
+
+  /// Get an exclusive access Accessor object. \p DoNotGetAccess allows to
+  /// create an accessor that is not owning anything based on a boolean
+  /// condition.
+  AccessorTy &&getExclusiveAccessor(bool DoNotGetAccess = false);
+
+private:
+  Ty Obj;
+  std::mutex Mtx;
+  friend struct Accessor<Ty>;
+};
+
+/// Helper to provide transparent exclusive access to protected objects.
+template <typename Ty> struct Accessor {
+  /// Default constructor does not own anything and cannot access anything.
+  Accessor() : Ptr(nullptr) {}
+
+  /// Constructor to get exclusive access by locking the mutex protecting the
+  /// underlying object.
+  Accessor(ProtectedObj<Ty> &PO) : Ptr(&PO) { lock(); }
+
+  /// Constructor to get exclusive access by taking it from \p Other.
+  Accessor(Accessor<Ty> &&Other) : Ptr(Other.Ptr) { Other.Ptr = nullptr; }
+
+  Accessor(Accessor &Other) = delete;
+
+  /// If the object is still owned when the lifetime ends we give up access.
+  ~Accessor() { unlock(); }
+
+  /// Give up access to the underlying object, virtually "destroying" the
+  /// accessor even if the object is still life.
+  void destroy() {
+    unlock();
+    Ptr = nullptr;
+  }
+
+  /// Provide transparent access to the underlying object.
+  Ty &operator*() {
+    assert(Ptr && "Trying to access an object through a non-owning (or "
+                  "destroyed) accessor!");
+    return Ptr->Obj;
+  }
+  Ty *operator->() {
+    assert(Ptr && "Trying to access an object through a non-owning (or "
+                  "destroyed) accessor!");
+    return &Ptr->Obj;
+  }
+
+private:
+  /// Lock the underlying object if there is one.
+  void lock() {
+    if (Ptr)
+      Ptr->Mtx.lock();
+  }
+
+  /// Unlock the underlying object if there is one.
+  void unlock() {
+    if (Ptr)
+      Ptr->Mtx.unlock();
+  }
+
+  /// Pointer to the underlying object or null if the accessor lost access,
+  /// e.g., after a destroy call.
+  ProtectedObj<Ty> *Ptr;
+};
+
+template <typename Ty>
+Accessor<Ty> &&ProtectedObj<Ty>::getExclusiveAccessor(bool DoNotGetAccess) {
+  if (DoNotGetAccess)
+    return std::move(Accessor<Ty>());
+  return std::move(Accessor<Ty>(*this));
+}
+
+#endif

openmp/libomptarget/include/device.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _OMPTARGET_DEVICE_H
 #define _OMPTARGET_DEVICE_H
 
 #include <cassert>
 #include <cstddef>
+#include <cstdint>
 #include <list>
 #include <map>
 #include <memory>
 #include <mutex>
 #include <set>
 #include <vector>
 
+#include "ExclusiveAccess.h"
 #include "omptarget.h"
 #include "rtl.h"
 
 // Forward declarations.
 struct RTLInfoTy;
 struct __tgt_bin_desc;
 struct __tgt_target_table;
 
bool getMayContainAttachedPointers() const {
     return States->MayContainAttachedPointers;
   }
 
   void lock() const { States->UpdateMtx.lock(); }
 
   void unlock() const { States->UpdateMtx.unlock(); }
 };
 
-typedef uintptr_t HstPtrBeginTy;
-inline bool operator<(const HostDataToTargetTy &lhs, const HstPtrBeginTy &rhs) {
-  return lhs.HstPtrBegin < rhs;
-}
-inline bool operator<(const HstPtrBeginTy &lhs, const HostDataToTargetTy &rhs) {
-  return lhs < rhs.HstPtrBegin;
-}
-inline bool operator<(const HostDataToTargetTy &lhs,
-                      const HostDataToTargetTy &rhs) {
-  return lhs.HstPtrBegin < rhs.HstPtrBegin;
+/// Wrapper around the HostDataToTargetTy to be used in the HDTT map. In
+/// addition to the HDTT pointer we store the key value explicitly. This
+/// allows the set to inspect (sort/search/...) this entry without an additional
+/// load of HDTT. HDTT is a pointer to allow the modification of the set without
+/// invalidating HDTT entries which can now be inspected at the same time.
+struct HostDataToTargetMapKeyTy {
+  uintptr_t KeyValue;
+
+  HostDataToTargetMapKeyTy(void *Key) : KeyValue(uintptr_t(Key)) {}
+  HostDataToTargetMapKeyTy(HostDataToTargetTy *HDTT)
+      : KeyValue(HDTT->HstPtrBegin), HDTT(HDTT) {}
+  HostDataToTargetTy *HDTT;
+};
+inline bool operator<(const HostDataToTargetMapKeyTy &lhs,
+                      const uintptr_t &rhs) {
+  return lhs.KeyValue < rhs;
+}
+inline bool operator<(const uintptr_t &lhs,
+                      const HostDataToTargetMapKeyTy &rhs) {
+  return lhs < rhs.KeyValue;
+}
+inline bool operator<(const HostDataToTargetMapKeyTy &lhs,
+                      const HostDataToTargetMapKeyTy &rhs) {
+  return lhs.KeyValue < rhs.KeyValue;
 }
 
-typedef std::set<HostDataToTargetTy, std::less<>> HostDataToTargetListTy;
-
 struct LookupResult {
   struct {
     unsigned IsContained : 1;
     unsigned ExtendsBefore : 1;
     unsigned ExtendsAfter : 1;
   } Flags;
 
-  HostDataToTargetListTy::iterator Entry;
+  /// The corresponding map table entry which is stable.
+  HostDataToTargetTy *Entry = nullptr;
 
   LookupResult() : Flags({0, 0, 0}), Entry() {}
 };
 
 /// This struct will be returned by \p DeviceTy::getTargetPointer which provides
 /// more data than just a target pointer.
 struct TargetPointerResultTy {
   struct {
     /// If the map table entry is just created
     unsigned IsNewEntry : 1;
     /// If the pointer is actually a host pointer (when unified memory enabled)
     unsigned IsHostPointer : 1;
   } Flags = {0, 0};
 
-  /// The iterator to the corresponding map table entry
-  HostDataToTargetListTy::iterator MapTableEntry{};
+  /// The corresponding map table entry which is stable.
+  HostDataToTargetTy *Entry = nullptr;
 
   /// The corresponding target pointer
   void *TargetPointer = nullptr;
 };
 
 /// Map for shadow pointers
 struct ShadowPtrValTy {
   void *HstPtrVal;
struct DeviceTy {
   int32_t DeviceID;
   RTLInfoTy *RTL;
   int32_t RTLDeviceID;
 
   bool IsInit;
   std::once_flag InitFlag;
   bool HasPendingGlobals;
 
-  HostDataToTargetListTy HostDataToTargetMap;
+  /// Host data to device map type with a wrapper key indirection that allows
+  /// concurrent modification of the entries without invalidating the underlying
+  /// entries.
+  using HostDataToTargetListTy =
+      std::set<HostDataToTargetMapKeyTy, std::less<>>;
+
+  /// The HDTTMap is a protected object that can only be accessed by one thread
+  /// at a time.
+  ProtectedObj<HostDataToTargetListTy> HostDataToTargetMap;
+
+  /// The type used to access the HDTT map.
+  using HDTTMapAccessorTy = decltype(HostDataToTargetMap)::AccessorTy;
+
   PendingCtorsDtorsPerLibrary PendingCtorsDtors;
 
   ShadowPtrListTy ShadowPtrMap;
 
-  std::mutex DataMapMtx, PendingGlobalsMtx, ShadowMtx;
+  std::mutex PendingGlobalsMtx, ShadowMtx;
 
   // NOTE: Once libomp gains full target-task support, this state should be
   // moved into the target task in libomp.
   std::map<int32_t, uint64_t> LoopTripCnt;
 
   DeviceTy(RTLInfoTy *RTL);
   // DeviceTy is not copyable
   DeviceTy(const DeviceTy &D) = delete;
   DeviceTy &operator=(const DeviceTy &D) = delete;
 
   ~DeviceTy();
 
   // Return true if data can be copied to DstDevice directly
   bool isDataExchangable(const DeviceTy &DstDevice);
 
-  LookupResult lookupMapping(void *HstPtrBegin, int64_t Size);
+  /// Lookup the mapping of \p HstPtrBegin in \p HDTTMap. The accessor ensures
+  /// exclusive access to the HDTT map.
+  LookupResult lookupMapping(HDTTMapAccessorTy &HDTTMap, void *HstPtrBegin,
+                             int64_t Size);
+
   /// Get the target pointer based on host pointer begin and base. If the
   /// mapping already exists, the target pointer will be returned directly. In
   /// addition, if required, the memory region pointed by \p HstPtrBegin of size
   /// \p Size will also be transferred to the device. If the mapping doesn't
   /// exist, and if unified shared memory is not enabled, a new mapping will be
   /// created and the data will also be transferred accordingly. nullptr will be
   /// returned because of any of following reasons:
   /// - Data allocation failed;
   /// - The user tried to do an illegal mapping;
   /// - Data transfer issue fails.
   TargetPointerResultTy
   getTargetPointer(void *HstPtrBegin, void *HstPtrBase, int64_t Size,
                    map_var_info_t HstPtrName, bool HasFlagTo,
                    bool HasFlagAlways, bool IsImplicit, bool UpdateRefCount,
                    bool HasCloseModifier, bool HasPresentModifier,
                    bool HasHoldModifier, AsyncInfoTy &AsyncInfo);
-  void *getTgtPtrBegin(void *HstPtrBegin, int64_t Size);
+
+  /// Return the target pointer for \p HstPtrBegin in \p HDTTMap. The accessor
+  /// ensures exclusive access to the HDTT map.
+  void *getTgtPtrBegin(HDTTMapAccessorTy &HDTTMap, void *HstPtrBegin,
+                       int64_t Size);
+
   TargetPointerResultTy getTgtPtrBegin(void *HstPtrBegin, int64_t Size,
                                        bool &IsLast, bool UpdateRefCount,
                                        bool UseHoldRefCount, bool &IsHostPtr,
                                        bool MustContain = false,
                                        bool ForceDelete = false);
   /// For the map entry for \p HstPtrBegin, decrement the reference count
   /// specified by \p HasHoldModifier and, if the the total reference count is
   /// then zero, deallocate the corresponding device storage and remove the map

openmp/libomptarget/src/device.cpp

 
 #include "device.h"
 #include "omptarget.h"
 #include "private.h"
 #include "rtl.h"
 
 #include <cassert>
 #include <climits>
+#include <cstdint>
 #include <cstdio>
 #include <string>
 
 int HostDataToTargetTy::addEventIfNecessary(DeviceTy &Device,
                                             AsyncInfoTy &AsyncInfo) const {
   // First, check if the user disabled atomic map transfer/malloc/dealloc.
   if (!PM->UseEventsForAtomicTransfers)
     return OFFLOAD_SUCCESS;
int HostDataToTargetTy::addEventIfNecessary(DeviceTy &Device,
   if (NeedNewEvent)
     setEvent(Event);
 
   return OFFLOAD_SUCCESS;
 }
 
 DeviceTy::DeviceTy(RTLInfoTy *RTL)
     : DeviceID(-1), RTL(RTL), RTLDeviceID(-1), IsInit(false), InitFlag(),
-      HasPendingGlobals(false), HostDataToTargetMap(), PendingCtorsDtors(),
-      ShadowPtrMap(), DataMapMtx(), PendingGlobalsMtx(), ShadowMtx() {}
+      HasPendingGlobals(false), PendingCtorsDtors(), ShadowPtrMap(),
+      PendingGlobalsMtx(), ShadowMtx() {}
 
 DeviceTy::~DeviceTy() {
   if (DeviceID == -1 || !(getInfoLevel() & OMP_INFOTYPE_DUMP_TABLE))
     return;
 
   ident_t loc = {0, 0, 0, 0, ";libomptarget;libomptarget;0;0;;"};
   dumpTargetPointerMappings(&loc, *this);
 }
 
 int DeviceTy::associatePtr(void *HstPtrBegin, void *TgtPtrBegin, int64_t Size) {
-  std::lock_guard<decltype(DataMapMtx)> LG(DataMapMtx);
+  HDTTMapAccessorTy HDTTMap = HostDataToTargetMap.getExclusiveAccessor();
 
   // Check if entry exists
-  auto search = HostDataToTargetMap.find(HstPtrBeginTy{(uintptr_t)HstPtrBegin});
-  if (search != HostDataToTargetMap.end()) {
+  auto It = HDTTMap->find(HstPtrBegin);
+  if (It != HDTTMap->end()) {
+    HostDataToTargetTy &HDTT = *It->HDTT;
     // Mapping already exists
-    bool isValid = search->HstPtrEnd == (uintptr_t)HstPtrBegin + Size &&
-                   search->TgtPtrBegin == (uintptr_t)TgtPtrBegin;
+    bool isValid = HDTT.HstPtrEnd == (uintptr_t)HstPtrBegin + Size &&
+                   HDTT.TgtPtrBegin == (uintptr_t)TgtPtrBegin;
     if (isValid) {
       DP("Attempt to re-associate the same device ptr+offset with the same "
          "host ptr, nothing to do\n");
       return OFFLOAD_SUCCESS;
     } else {
       REPORT("Not allowed to re-associate a different device ptr+offset with "
              "the same host ptr\n");
       return OFFLOAD_FAIL;
     }
   }
 
   // Mapping does not exist, allocate it with refCount=INF
   const HostDataToTargetTy &newEntry =
-      *HostDataToTargetMap
-           .emplace(
+      *HDTTMap
+           ->emplace(new HostDataToTargetTy(
                /*HstPtrBase=*/(uintptr_t)HstPtrBegin,
                /*HstPtrBegin=*/(uintptr_t)HstPtrBegin,
                /*HstPtrEnd=*/(uintptr_t)HstPtrBegin + Size,
                /*TgtPtrBegin=*/(uintptr_t)TgtPtrBegin,
                /*UseHoldRefCount=*/false, /*Name=*/nullptr,
-               /*IsRefCountINF=*/true)
-           .first;
+               /*IsRefCountINF=*/true))
+           .first->HDTT;
   DP("Creating new map entry: HstBase=" DPxMOD ", HstBegin=" DPxMOD
      ", HstEnd=" DPxMOD ", TgtBegin=" DPxMOD ", DynRefCount=%s, "
      "HoldRefCount=%s\n",
      DPxPTR(newEntry.HstPtrBase), DPxPTR(newEntry.HstPtrBegin),
      DPxPTR(newEntry.HstPtrEnd), DPxPTR(newEntry.TgtPtrBegin),
      newEntry.dynRefCountToStr().c_str(), newEntry.holdRefCountToStr().c_str());
   (void)newEntry;
 
   return OFFLOAD_SUCCESS;
 }
 
 int DeviceTy::disassociatePtr(void *HstPtrBegin) {
-  std::lock_guard<decltype(DataMapMtx)> LG(DataMapMtx);
+  HDTTMapAccessorTy HDTTMap = HostDataToTargetMap.getExclusiveAccessor();
 
-  auto search = HostDataToTargetMap.find(HstPtrBeginTy{(uintptr_t)HstPtrBegin});
-  if (search != HostDataToTargetMap.end()) {
+  auto It = HDTTMap->find(HstPtrBegin);
+  if (It != HDTTMap->end()) {
+    HostDataToTargetTy &HDTT = *It->HDTT;
     // Mapping exists
-    if (search->getHoldRefCount()) {
+    if (HDTT.getHoldRefCount()) {
       // This is based on OpenACC 3.1, sec 3.2.33 "acc_unmap_data", L3656-3657:
       // "It is an error to call acc_unmap_data if the structured reference
       // count for the pointer is not zero."
       REPORT("Trying to disassociate a pointer with a non-zero hold reference "
              "count\n");
-    } else if (search->isDynRefCountInf()) {
+    } else if (HDTT.isDynRefCountInf()) {
       DP("Association found, removing it\n");
-      void *Event = search->getEvent();
+      void *Event = HDTT.getEvent();
+      delete &HDTT;
       if (Event)
         destroyEvent(Event);
-      HostDataToTargetMap.erase(search);
+      HDTTMap->erase(It);
       return OFFLOAD_SUCCESS;
     } else {
       REPORT("Trying to disassociate a pointer which was not mapped via "
              "omp_target_associate_ptr\n");
     }
   } else {
     REPORT("Association not found\n");
   }
 
   // Mapping not found
   return OFFLOAD_FAIL;
 }
 
-LookupResult DeviceTy::lookupMapping(void *HstPtrBegin, int64_t Size) {
+LookupResult DeviceTy::lookupMapping(HDTTMapAccessorTy &HDTTMap,
+                                     void *HstPtrBegin, int64_t Size) {
+
   uintptr_t hp = (uintptr_t)HstPtrBegin;
   LookupResult lr;
 
   DP("Looking up mapping(HstPtrBegin=" DPxMOD ", Size=%" PRId64 ")...\n",
      DPxPTR(hp), Size);
 
-  if (HostDataToTargetMap.empty())
+  if (HDTTMap->empty())
     return lr;
 
-  auto upper = HostDataToTargetMap.upper_bound(hp);
+  auto upper = HDTTMap->upper_bound(hp);
   // check the left bin
-  if (upper != HostDataToTargetMap.begin()) {
-    lr.Entry = std::prev(upper);
+  if (upper != HDTTMap->begin()) {
+    lr.Entry = std::prev(upper)->HDTT;
     auto &HT = *lr.Entry;
     // Is it contained?
     lr.Flags.IsContained = hp >= HT.HstPtrBegin && hp < HT.HstPtrEnd &&
                            (hp + Size) <= HT.HstPtrEnd;
     // Does it extend beyond the mapped region?
     lr.Flags.ExtendsAfter = hp < HT.HstPtrEnd && (hp + Size) > HT.HstPtrEnd;
   }
 
   // check the right bin
   if (!(lr.Flags.IsContained || lr.Flags.ExtendsAfter) &&
-      upper != HostDataToTargetMap.end()) {
-    lr.Entry = upper;
+      upper != HDTTMap->end()) {
+    lr.Entry = upper->HDTT;
     auto &HT = *lr.Entry;
     // Does it extend into an already mapped region?
     lr.Flags.ExtendsBefore =
         hp < HT.HstPtrBegin && (hp + Size) > HT.HstPtrBegin;
     // Does it extend beyond the mapped region?
     lr.Flags.ExtendsAfter = hp < HT.HstPtrEnd && (hp + Size) > HT.HstPtrEnd;
   }
 
LookupResult DeviceTy::lookupMapping(HDTTMapAccessorTy &HDTTMap,
   return lr;
 }
 
 TargetPointerResultTy DeviceTy::getTargetPointer(
     void *HstPtrBegin, void *HstPtrBase, int64_t Size,
     map_var_info_t HstPtrName, bool HasFlagTo, bool HasFlagAlways,
     bool IsImplicit, bool UpdateRefCount, bool HasCloseModifier,
     bool HasPresentModifier, bool HasHoldModifier, AsyncInfoTy &AsyncInfo) {
+  HDTTMapAccessorTy HDTTMap = HostDataToTargetMap.getExclusiveAccessor();
+
   void *TargetPointer = nullptr;
   bool IsHostPtr = false;
   bool IsNew = false;
 
-  DataMapMtx.lock();
-
-  LookupResult LR = lookupMapping(HstPtrBegin, Size);
-  auto Entry = LR.Entry;
+  LookupResult LR = lookupMapping(HDTTMap, HstPtrBegin, Size);
+  auto *Entry = LR.Entry;
 
   // Check if the pointer is contained.
   // If a variable is mapped to the device manually by the user - which would
   // lead to the IsContained flag to be true - then we must ensure that the
   // device address is returned even under unified memory conditions.
   if (LR.Flags.IsContained ||
       ((LR.Flags.ExtendsBefore || LR.Flags.ExtendsAfter) && IsImplicit)) {
     auto &HT = *LR.Entry;
DP("Mapping required by 'present' map type modifier does not exist for "
        DPxPTR(HstPtrBegin), Size);
     MESSAGE("device mapping required by 'present' map type modifier does not "
             "exist for host address " DPxMOD " (%" PRId64 " bytes)",
             DPxPTR(HstPtrBegin), Size);
   } else if (Size) {
     // If it is not contained and Size > 0, we should create a new entry for it.
     IsNew = true;
     uintptr_t Ptr = (uintptr_t)allocData(Size, HstPtrBegin);
-    Entry = HostDataToTargetMap
-                .emplace((uintptr_t)HstPtrBase, (uintptr_t)HstPtrBegin,
-                         (uintptr_t)HstPtrBegin + Size, Ptr, HasHoldModifier,
-                         HstPtrName)
-                .first;
+    Entry = HDTTMap
+                ->emplace(new HostDataToTargetTy(
+                    (uintptr_t)HstPtrBase, (uintptr_t)HstPtrBegin,
+                    HstPtrName))
+                .first->HDTT;
     INFO(OMP_INFOTYPE_MAPPING_CHANGED, DeviceID,
          "Creating new map entry with "
          "HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD ", Size=%ld, "
          "DynRefCount=%s, HoldRefCount=%s, Name=%s\n",
          DPxPTR(HstPtrBegin), DPxPTR(Ptr), Size,
          Entry->dynRefCountToStr().c_str(), Entry->holdRefCountToStr().c_str(),
          (HstPtrName) ? getNameFromMapping(HstPtrName).c_str() : "unknown");
     TargetPointer = (void *)Ptr;
   }
 
   // If the target pointer is valid, and we need to transfer data, issue the
   // data transfer.
   if (TargetPointer && !IsHostPtr && HasFlagTo && (IsNew || HasFlagAlways)) {
     // Lock the entry before releasing the mapping table lock such that another
     // thread that could issue data movement will get the right result.
     std::lock_guard<decltype(*Entry)> LG(*Entry);
     // Release the mapping table lock right after the entry is locked.
-    DataMapMtx.unlock();
+    HDTTMap.destroy();
 
     DP("Moving %" PRId64 " bytes (hst:" DPxMOD ") -> (tgt:" DPxMOD ")\n", Size,
        DPxPTR(HstPtrBegin), DPxPTR(TargetPointer));
 
     int Ret = submitData(TargetPointer, HstPtrBegin, Size, AsyncInfo);
     if (Ret != OFFLOAD_SUCCESS) {
       REPORT("Copying data to device failed.\n");
       // We will also return nullptr if the data movement fails because that
       // pointer points to a corrupted memory region so it doesn't make any
       // sense to continue to use it.
       TargetPointer = nullptr;
     } else if (Entry->addEventIfNecessary(*this, AsyncInfo) != OFFLOAD_SUCCESS)
       return {{false /* IsNewEntry */, false /* IsHostPointer */},
-              {} /* MapTableEntry */,
+              nullptr /* Entry */,
               nullptr /* TargetPointer */};
   } else {
     // Release the mapping table lock directly.
-    DataMapMtx.unlock();
+    HDTTMap.destroy();
     // If not a host pointer and no present modifier, we need to wait for the
     // event if it exists.
     // Note: Entry might be nullptr because of zero length array section.
-    if (Entry != HostDataToTargetListTy::iterator() && !IsHostPtr &&
-        !HasPresentModifier) {
+    if (Entry && !IsHostPtr && !HasPresentModifier) {
       std::lock_guard<decltype(*Entry)> LG(*Entry);
       void *Event = Entry->getEvent();
       if (Event) {
         int Ret = waitEvent(Event, AsyncInfo);
         if (Ret != OFFLOAD_SUCCESS) {
           // If it fails to wait for the event, we need to return nullptr in
           // case of any data race.
           REPORT("Failed to wait for event " DPxMOD ".\n", DPxPTR(Event));
           return {{false /* IsNewEntry */, false /* IsHostPointer */},
-                  {} /* MapTableEntry */,
+                  nullptr /* Entry */,
                   nullptr /* TargetPointer */};
         }
       }
     }
   }
 
   return {{IsNew, IsHostPtr}, Entry, TargetPointer};
 }
 
 // Used by targetDataBegin, targetDataEnd, targetDataUpdate and target.
 // Return the target pointer begin (where the data will be moved).
 // Decrement the reference counter if called from targetDataEnd.
 TargetPointerResultTy
 DeviceTy::getTgtPtrBegin(void *HstPtrBegin, int64_t Size, bool &IsLast,
                          bool UpdateRefCount, bool UseHoldRefCount,
                          bool &IsHostPtr, bool MustContain, bool ForceDelete) {
+  HDTTMapAccessorTy HDTTMap = HostDataToTargetMap.getExclusiveAccessor();
+
   void *TargetPointer = NULL;
   bool IsNew = false;
   IsHostPtr = false;
   IsLast = false;
-  std::lock_guard<decltype(DataMapMtx)> LG(DataMapMtx);
-  LookupResult lr = lookupMapping(HstPtrBegin, Size);
+  LookupResult lr = lookupMapping(HDTTMap, HstPtrBegin, Size);
 
   if (lr.Flags.IsContained ||
       (!MustContain && (lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter))) {
     auto &HT = *lr.Entry;
     // We do not zero the total reference count here.  deallocTgtPtr does that
     // atomically with removing the mapping.  Otherwise, before this thread
     // removed the mapping in deallocTgtPtr, another thread could retrieve the
     // mapping, increment and decrement back to zero, and then both threads
if (lr.Flags.IsContained ||
     IsHostPtr = true;
     TargetPointer = HstPtrBegin;
   }
 
   return {{IsNew, IsHostPtr}, lr.Entry, TargetPointer};
 }
 
 // Return the target pointer begin (where the data will be moved).
-// Lock-free version called when loading global symbols from the fat binary.
-void *DeviceTy::getTgtPtrBegin(void *HstPtrBegin, int64_t Size) {
+void *DeviceTy::getTgtPtrBegin(HDTTMapAccessorTy &HDTTMap, void *HstPtrBegin,
+                               int64_t Size) {
   uintptr_t hp = (uintptr_t)HstPtrBegin;
-  LookupResult lr = lookupMapping(HstPtrBegin, Size);
+  LookupResult lr = lookupMapping(HDTTMap, HstPtrBegin, Size);
   if (lr.Flags.IsContained || lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) {
     auto &HT = *lr.Entry;
     uintptr_t tp = HT.TgtPtrBegin + (hp - HT.HstPtrBegin);
     return (void *)tp;
   }
 
   return NULL;
 }
 
 int DeviceTy::deallocTgtPtr(void *HstPtrBegin, int64_t Size,
                             bool HasHoldModifier) {
-  std::lock_guard<decltype(DataMapMtx)> LG(DataMapMtx);
+  HDTTMapAccessorTy HDTTMap = HostDataToTargetMap.getExclusiveAccessor();
 
   // Check if the pointer is contained in any sub-nodes.
   int Ret = OFFLOAD_SUCCESS;
-  LookupResult lr = lookupMapping(HstPtrBegin, Size);
+  LookupResult lr = lookupMapping(HDTTMap, HstPtrBegin, Size);
   if (lr.Flags.IsContained || lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) {
     auto &HT = *lr.Entry;
     if (HT.decRefCount(HasHoldModifier) == 0) {
       DP("Deleting tgt data " DPxMOD " of size %" PRId64 "\n",
          DPxPTR(HT.TgtPtrBegin), Size);
       deleteData((void *)HT.TgtPtrBegin);
       INFO(OMP_INFOTYPE_MAPPING_CHANGED, DeviceID,
            "Removing map entry with HstPtrBegin=" DPxMOD ", TgtPtrBegin=" DPxMOD
            ", Size=%" PRId64 ", Name=%s\n",
            DPxPTR(HT.HstPtrBegin), DPxPTR(HT.TgtPtrBegin), Size,
            (HT.HstPtrName) ? getNameFromMapping(HT.HstPtrName).c_str()
                            : "unknown");
       void *Event = lr.Entry->getEvent();
-      HostDataToTargetMap.erase(lr.Entry);
+      HDTTMap->erase(lr.Entry);
+      delete lr.Entry;
       if (Event && destroyEvent(Event) != OFFLOAD_SUCCESS) {
         REPORT("Failed to destroy event " DPxMOD "\n", DPxPTR(Event));
         Ret = OFFLOAD_FAIL;
       }
     }
   } else {
     REPORT("Section to delete (hst addr " DPxMOD ") does not exist in the"
            " allocated memory\n",

 int32_t DeviceTy::deleteData(void *TgtPtrBegin) {
   return RTL->data_delete(RTLDeviceID, TgtPtrBegin);
 }
 
 // Submit data to device
 int32_t DeviceTy::submitData(void *TgtPtrBegin, void *HstPtrBegin, int64_t Size,
                              AsyncInfoTy &AsyncInfo) {
   if (getInfoLevel() & OMP_INFOTYPE_DATA_TRANSFER) {
-    LookupResult LR = lookupMapping(HstPtrBegin, Size);
+    HDTTMapAccessorTy HDTTMap = HostDataToTargetMap.getExclusiveAccessor();
+    LookupResult LR = lookupMapping(HDTTMap, HstPtrBegin, Size);
     auto *HT = &*LR.Entry;
 
     INFO(OMP_INFOTYPE_DATA_TRANSFER, DeviceID,
          "Copying data from host to device, HstPtr=" DPxMOD ", TgtPtr=" DPxMOD
          ", Size=%" PRId64 ", Name=%s\n",
          DPxPTR(HstPtrBegin), DPxPTR(TgtPtrBegin), Size,
          (HT && HT->HstPtrName) ? getNameFromMapping(HT->HstPtrName).c_str()
                                 : "unknown");
   }
 
   if (!AsyncInfo || !RTL->data_submit_async || !RTL->synchronize)
     return RTL->data_submit(RTLDeviceID, TgtPtrBegin, HstPtrBegin, Size);
   else
     return RTL->data_submit_async(RTLDeviceID, TgtPtrBegin, HstPtrBegin, Size,
                                   AsyncInfo);
 }
 
 // Retrieve data from device
 int32_t DeviceTy::retrieveData(void *HstPtrBegin, void *TgtPtrBegin,
                                int64_t Size, AsyncInfoTy &AsyncInfo) {
   if (getInfoLevel() & OMP_INFOTYPE_DATA_TRANSFER) {
-    LookupResult LR = lookupMapping(HstPtrBegin, Size);
+    HDTTMapAccessorTy HDTTMap = HostDataToTargetMap.getExclusiveAccessor();
+    LookupResult LR = lookupMapping(HDTTMap, HstPtrBegin, Size);
     auto *HT = &*LR.Entry;
     INFO(OMP_INFOTYPE_DATA_TRANSFER, DeviceID,
          "Copying data from device to host, TgtPtr=" DPxMOD ", HstPtr=" DPxMOD
          ", Size=%" PRId64 ", Name=%s\n",
          DPxPTR(TgtPtrBegin), DPxPTR(HstPtrBegin), Size,
          (HT && HT->HstPtrName) ? getNameFromMapping(HT->HstPtrName).c_str()
                                 : "unknown");
   }

openmp/libomptarget/src/omptarget.cpp

for (auto *HostEntriesBegin : PM->HostEntriesBeginRegistrationOrder) {
             "Host and Target tables mismatch for device id %d [%zx != %zx].\n",
             device_id, hsize, tsize);
         TransTable->TargetsImages[device_id] = 0;
         TransTable->TargetsTable[device_id] = 0;
         rc = OFFLOAD_FAIL;
         break;
       }
 
-      // process global data that needs to be mapped.
-      std::lock_guard<decltype(Device.DataMapMtx)> LG(Device.DataMapMtx);
+      DeviceTy::HDTTMapAccessorTy HDTTMap =
+          Device.HostDataToTargetMap.getExclusiveAccessor();
 
       __tgt_target_table *HostTable = &TransTable->HostTable;
       for (__tgt_offload_entry *CurrDeviceEntry = TargetTable->EntriesBegin,
                                *CurrHostEntry = HostTable->EntriesBegin,
                                *EntryDeviceEnd = TargetTable->EntriesEnd;
            CurrDeviceEntry != EntryDeviceEnd;
            CurrDeviceEntry++, CurrHostEntry++) {
         if (CurrDeviceEntry->size != 0) {
           // has data.
           assert(CurrDeviceEntry->size == CurrHostEntry->size &&
                  "data size mismatch");
 
           // Fortran may use multiple weak declarations for the same symbol,
           // therefore we must allow for multiple weak symbols to be loaded from
           // the fat binary. Treat these mappings as any other "regular"
           // mapping. Add entry to map.
-          if (Device.getTgtPtrBegin(CurrHostEntry->addr, CurrHostEntry->size))
+          if (Device.getTgtPtrBegin(HDTTMap, CurrHostEntry->addr,
+                                    CurrHostEntry->size))
             continue;
+
           DP("Add mapping from host " DPxMOD " to device " DPxMOD
              " with size %zu"
              "\n",
              DPxPTR(CurrHostEntry->addr), DPxPTR(CurrDeviceEntry->addr),
              CurrDeviceEntry->size);
-          Device.HostDataToTargetMap.emplace(
+          HDTTMap->emplace(new HostDataToTargetTy(
               (uintptr_t)CurrHostEntry->addr /*HstPtrBase*/,
               (uintptr_t)CurrHostEntry->addr /*HstPtrBegin*/,
               (uintptr_t)CurrHostEntry->addr +
                   CurrHostEntry->size /*HstPtrEnd*/,
               (uintptr_t)CurrDeviceEntry->addr /*TgtPtrBegin*/,
               false /*UseHoldRefCount*/, nullptr /*Name*/,
-              true /*IsRefCountINF*/);
+              true /*IsRefCountINF*/));
         }
       }
     }
   }
 
   if (rc != OFFLOAD_SUCCESS) {
     return rc;
   }
if (arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ && !IsHostPtr) {
       // OBJ is deallocated and later on allocated again (at a different device
       // address), ShadowPtrMap still contains an entry for Pointer_HstPtrBegin
       // which is stale, pointing to the old ExpectedTgtPtrBase of the OBJ.
       if (Entry == Device.ShadowPtrMap.end() ||
           Entry->second.TgtPtrVal != ExpectedTgtPtrBase) {
         // create or update shadow pointers for this entry
         Device.ShadowPtrMap[Pointer_HstPtrBegin] = {
             HstPtrBase, PointerTgtPtrBegin, ExpectedTgtPtrBase};
-        Pointer_TPR.MapTableEntry->setMayContainAttachedPointers();
+        Pointer_TPR.Entry->setMayContainAttachedPointers();
         UpdateDevPtr = true;
       }
 
       if (UpdateDevPtr) {
-        std::lock_guard<decltype(*Pointer_TPR.MapTableEntry)> LG(
-            *Pointer_TPR.MapTableEntry);
+        std::lock_guard<decltype(*Pointer_TPR.Entry)> LG(*Pointer_TPR.Entry);
         Device.ShadowMtx.unlock();
 
         DP("Update pointer (" DPxMOD ") -> [" DPxMOD "]\n",
            DPxPTR(PointerTgtPtrBegin), DPxPTR(TgtPtrBegin));
 
         void *&TgtPtrBase = AsyncInfo.getVoidPtrLocation();
         TgtPtrBase = ExpectedTgtPtrBase;
 
         int Ret = Device.submitData(PointerTgtPtrBegin, &TgtPtrBase,
                                     sizeof(void *), AsyncInfo);
         if (Ret != OFFLOAD_SUCCESS) {
           REPORT("Copying data to device failed.\n");
           return OFFLOAD_FAIL;
         }
-        if (Pointer_TPR.MapTableEntry->addEventIfNecessary(Device, AsyncInfo) !=
+        if (Pointer_TPR.Entry->addEventIfNecessary(Device, AsyncInfo) !=
             OFFLOAD_SUCCESS)
           return OFFLOAD_FAIL;
       } else
         Device.ShadowMtx.unlock();
     }
   }
 
   return OFFLOAD_SUCCESS;
static void applyToShadowMapEntries(DeviceTy &Device, CBTy CB, void *Begin,
                                     const TargetPointerResultTy &TPR) {
   // If we have an object that is too small to hold a pointer subobject, no need
   // to do any checking.
   if (Size < sizeof(void *))
     return;
 
   // If the map entry for the object was never marked as containing attached
   // pointers, no need to do any checking.
-  if (TPR.MapTableEntry == HostDataToTargetListTy::iterator{} ||
-      !TPR.MapTableEntry->getMayContainAttachedPointers())
+  if (!TPR.Entry || !TPR.Entry->getMayContainAttachedPointers())
     return;
 
   uintptr_t LB = (uintptr_t)Begin;
   uintptr_t UB = LB + Size;
   // Now we are looking into the shadow map so we need to lock it.
   std::lock_guard<decltype(Device.ShadowMtx)> LG(Device.ShadowMtx);
   for (ShadowPtrListTy::iterator Itr = Device.ShadowPtrMap.begin();
        Itr != Device.ShadowPtrMap.end();) {

openmp/libomptarget/src/private.h

 
 #define TARGET_NAME Libomptarget
 #define DEBUG_PREFIX GETNAME(TARGET_NAME)
 
 ////////////////////////////////////////////////////////////////////////////////
 /// dump a table of all the host-target pointer pairs on failure
 static inline void dumpTargetPointerMappings(const ident_t *Loc,
                                              DeviceTy &Device) {
-  if (Device.HostDataToTargetMap.empty())
+  DeviceTy::HDTTMapAccessorTy HDTTMap =
+      Device.HostDataToTargetMap.getExclusiveAccessor();
+  if (HDTTMap->empty())
     return;
 
   SourceInfo Kernel(Loc);
   INFO(OMP_INFOTYPE_ALL, Device.DeviceID,
        "OpenMP Host-Device pointer mappings after block at %s:%d:%d:\n",
        Kernel.getFilename(), Kernel.getLine(), Kernel.getColumn());
   INFO(OMP_INFOTYPE_ALL, Device.DeviceID, "%-18s %-18s %s %s %s %s\n",
        "Host Ptr", "Target Ptr", "Size (B)", "DynRefCount", "HoldRefCount",
        "Declaration");
-  Device.DataMapMtx.lock();
-  for (const auto &HostTargetMap : Device.HostDataToTargetMap) {
-    SourceInfo Info(HostTargetMap.HstPtrName);
+  for (const auto &It : *HDTTMap) {
+    HostDataToTargetTy &HDTT = *It.HDTT;
+    SourceInfo Info(HDTT.HstPtrName);
     INFO(OMP_INFOTYPE_ALL, Device.DeviceID,
          DPxMOD " " DPxMOD " %-8" PRIuPTR " %-11s %-12s %s at %s:%d:%d\n",
-         DPxPTR(HostTargetMap.HstPtrBegin), DPxPTR(HostTargetMap.TgtPtrBegin),
-         HostTargetMap.HstPtrEnd - HostTargetMap.HstPtrBegin,
-         HostTargetMap.dynRefCountToStr().c_str(),
-         HostTargetMap.holdRefCountToStr().c_str(), Info.getName(),
-         Info.getFilename(), Info.getLine(), Info.getColumn());
+         DPxPTR(HDTT.HstPtrBegin), DPxPTR(HDTT.TgtPtrBegin),
+         HDTT.HstPtrEnd - HDTT.HstPtrBegin, HDTT.dynRefCountToStr().c_str(),
+         HDTT.holdRefCountToStr().c_str(), Info.getName(), Info.getFilename(),
+         Info.getLine(), Info.getColumn());
   }
-  Device.DataMapMtx.unlock();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 /// Print out the names and properties of the arguments to each kernel
 static inline void
 printKernelArguments(const ident_t *Loc, const int64_t DeviceId,
                      const int32_t ArgNum, const int64_t *ArgSizes,
                      const int64_t *ArgTypes, const map_var_info_t *ArgNames,