[OpenMP][NFCI] Use RAII lock guards in libomptarget where possible

[JonChesterfield]

Great, thanks!

[ronlieb]

This set of commits seems to have broken the amdgpu buildbot, test(s) are hanging.
see

https://lab.llvm.org/buildbot/#/builders/193/builds/8002

and a force run to see if the previous hang was spurious, seems reproducible.

https://lab.llvm.org/buildbot/#/builders/193/builds/8003

it would be helpful to revert the change(s) seems like 4 commits came in ?
then we (Jon and I) could work the issues with you in the morning on Monday.
unless of course you have a fix ready to land.

thank you

[jdoerfert]

Reverted to check what the problem is.

[JonChesterfield]

Found one error in the existing code but can believe there are more. We could split this patch into two, one replacing the DIY LockGuard with std::lock_guard and clang-format the thing, then a second replacing manual locking with uses of lock guard. Would be the same behaviour as this patch but make it easier to find the change in behaviour by inspection.

openmp/libomptarget/src/omptarget.cpp

206

Before this patch we left PendingGlobalsMtx locked here on the return. That's surely a bug, and I guess we don't hit it often because it's on a failure path. Fixed by this patch, which sadly seems to deadlock somewhere else.

[protze.joachim]

openmp/libomptarget/src/device.cpp

447

This unlock is gone without translating the lock to raii lock guard.

[jdoerfert]

openmp/libomptarget/src/device.cpp

447

Good catch :)

openmp/libomptarget/src/omptarget.cpp

206

Agreed.

[jdoerfert]

Fixed rebase faux pas, thanks @protze.joachim

openmp/libomptarget/include/device.h

public:
 
   void setMayContainAttachedPointers() const {
     States->MayContainAttachedPointers = true;
   }
   bool getMayContainAttachedPointers() const {
     return States->MayContainAttachedPointers;
   }
 
-  /// Helper to make sure the entry is locked in a scope.
-  /// TODO: We should generalize this and use it for all our objects that use
-  /// lock/unlock methods.
-  struct LockGuard {
-    const HostDataToTargetTy &Entry;
-
-  public:
-    LockGuard(const HostDataToTargetTy &Entry) : Entry(Entry) { Entry.lock(); }
-    ~LockGuard() { Entry.unlock(); }
-  };
-
-private:
   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;

openmp/libomptarget/src/device.cpp

 //===--------- device.cpp - Target independent OpenMP target RTL ----------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // Functionality for managing devices that are handled by RTL plugins.
 //
 //===----------------------------------------------------------------------===//
 
 #include "device.h"
+#include "omptarget.h"
 #include "private.h"
 #include "rtl.h"
 
 #include <cassert>
 #include <climits>
 #include <cstdio>
 #include <string>
 
-int HostDataToTargetTy::addEventIfNecessary(
-    DeviceTy &Device, AsyncInfoTy &AsyncInfo) const {
+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;
 
   void *Event = getEvent();
   bool NeedNewEvent = Event == nullptr;
   if (NeedNewEvent && Device.createEvent(&Event) != OFFLOAD_SUCCESS) {
     REPORT("Failed to create event\n");
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) {
-  DataMapMtx.lock();
+  std::lock_guard<decltype(DataMapMtx)> LG(DataMapMtx);
 
   // Check if entry exists
   auto search = HostDataToTargetMap.find(HstPtrBeginTy{(uintptr_t)HstPtrBegin});
   if (search != HostDataToTargetMap.end()) {
     // Mapping already exists
     bool isValid = search->HstPtrEnd == (uintptr_t)HstPtrBegin + Size &&
                    search->TgtPtrBegin == (uintptr_t)TgtPtrBegin;
-    DataMapMtx.unlock();
     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;
int DeviceTy::associatePtr(void *HstPtrBegin, void *TgtPtrBegin, int64_t Size) {
   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;
 
-  DataMapMtx.unlock();
-
   return OFFLOAD_SUCCESS;
 }
 
 int DeviceTy::disassociatePtr(void *HstPtrBegin) {
-  DataMapMtx.lock();
+  std::lock_guard<decltype(DataMapMtx)> LG(DataMapMtx);
 
   auto search = HostDataToTargetMap.find(HstPtrBeginTy{(uintptr_t)HstPtrBegin});
   if (search != HostDataToTargetMap.end()) {
     // Mapping exists
     if (search->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()) {
       DP("Association found, removing it\n");
       void *Event = search->getEvent();
       if (Event)
         destroyEvent(Event);
       HostDataToTargetMap.erase(search);
-      DataMapMtx.unlock();
       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
-  DataMapMtx.unlock();
   return OFFLOAD_FAIL;
 }
 
 LookupResult DeviceTy::lookupMapping(void *HstPtrBegin, int64_t Size) {
   uintptr_t hp = (uintptr_t)HstPtrBegin;
   LookupResult lr;
 
   DP("Looking up mapping(HstPtrBegin=" DPxMOD ", Size=%" PRId64 ")...\n",
LookupResult DeviceTy::lookupMapping(void *HstPtrBegin, int64_t Size) {
   if (lr.Flags.ExtendsAfter) {
     DP("WARNING: Pointer is already mapped but section extends beyond mapped "
        "region\n");
   }
 
   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) {
+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) {
   void *TargetPointer = nullptr;
   bool IsHostPtr = false;
   bool IsNew = false;
 
   DataMapMtx.lock();
 
   LookupResult LR = lookupMapping(HstPtrBegin, Size);
   auto Entry = LR.Entry;
if (LR.Flags.IsContained ||
     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.
-    HostDataToTargetTy::LockGuard LG(*Entry);
+    std::lock_guard<decltype(*Entry)> LG(*Entry);
     // Release the mapping table lock right after the entry is locked.
     DataMapMtx.unlock();
 
     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)
+    } else if (Entry->addEventIfNecessary(*this, AsyncInfo) != OFFLOAD_SUCCESS)
       return {{false /* IsNewEntry */, false /* IsHostPointer */},
               {} /* MapTableEntry */,
               nullptr /* TargetPointer */};
   } else {
     // Release the mapping table lock directly.
     DataMapMtx.unlock();
     // 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) {
-      HostDataToTargetTy::LockGuard LG(*Entry);
+      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 */},

 TargetPointerResultTy
 DeviceTy::getTgtPtrBegin(void *HstPtrBegin, int64_t Size, bool &IsLast,
                          bool UpdateRefCount, bool UseHoldRefCount,
                          bool &IsHostPtr, bool MustContain, bool ForceDelete) {
   void *TargetPointer = NULL;
   bool IsNew = false;
   IsHostPtr = false;
   IsLast = false;
-  DataMapMtx.lock();
+  std::lock_guard<decltype(DataMapMtx)> LG(DataMapMtx);
   LookupResult lr = lookupMapping(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
if (lr.Flags.IsContained ||
     // use directly from the host.
     DP("Get HstPtrBegin " DPxMOD " Size=%" PRId64 " for unified shared "
        "memory\n",
        DPxPTR((uintptr_t)HstPtrBegin), Size);
     IsHostPtr = true;
     TargetPointer = HstPtrBegin;
   }
 
-  DataMapMtx.unlock();
   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) {
   uintptr_t hp = (uintptr_t)HstPtrBegin;
   LookupResult lr = lookupMapping(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);
+
   // Check if the pointer is contained in any sub-nodes.
   int Ret = OFFLOAD_SUCCESS;
-  DataMapMtx.lock();
   LookupResult lr = lookupMapping(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,
if (lr.Flags.IsContained || lr.Flags.ExtendsBefore || lr.Flags.ExtendsAfter) {
     }
   } else {
     REPORT("Section to delete (hst addr " DPxMOD ") does not exist in the"
            " allocated memory\n",
            DPxPTR(HstPtrBegin));
     Ret = OFFLOAD_FAIL;
   }
 
-  DataMapMtx.unlock();
   return Ret;
 }
 
 /// Init device, should not be called directly.
 void DeviceTy::init() {
   // Make call to init_requires if it exists for this plugin.
   if (RTL->init_requires)
     RTL->init_requires(PM->RTLs.RequiresFlags);
int32_t DeviceTy::initOnce() {
   if (IsInit)
     return OFFLOAD_SUCCESS;
   else
     return OFFLOAD_FAIL;
 }
 
 // Load binary to device.
 __tgt_target_table *DeviceTy::load_binary(void *Img) {
-  RTL->Mtx.lock();
+  std::lock_guard<decltype(RTL->Mtx)> LG(RTL->Mtx);
   __tgt_target_table *rc = RTL->load_binary(RTLDeviceID, Img);
-  RTL->Mtx.unlock();
   return rc;
 }
 
 void *DeviceTy::allocData(int64_t Size, void *HstPtr, int32_t Kind) {
   return RTL->data_alloc(RTLDeviceID, Size, HstPtr, Kind);
 }
 
 int32_t DeviceTy::deleteData(void *TgtPtrBegin) {

 }
 
 /// Check whether a device has an associated RTL and initialize it if it's not
 /// already initialized.
 bool device_is_ready(int device_num) {
   DP("Checking whether device %d is ready.\n", device_num);
   // Devices.size() can only change while registering a new
   // library, so try to acquire the lock of RTLs' mutex.
-  PM->RTLsMtx.lock();
-  size_t DevicesSize = PM->Devices.size();
-  PM->RTLsMtx.unlock();
+  size_t DevicesSize;
+  {
+    std::lock_guard<decltype(PM->RTLsMtx)> LG(PM->RTLsMtx);
+    DevicesSize = PM->Devices.size();
+  }
   if (DevicesSize <= (size_t)device_num) {
     DP("Device ID  %d does not have a matching RTL\n", device_num);
     return false;
   }
 
   // Get device info
   DeviceTy &Device = *PM->Devices[device_num];
 

openmp/libomptarget/src/omptarget.cpp

static int InitLibrary(DeviceTy &Device) {
   /*
    * Map global data
    */
   int32_t device_id = Device.DeviceID;
   int rc = OFFLOAD_SUCCESS;
   bool supportsEmptyImages = Device.RTL->supports_empty_images &&
                              Device.RTL->supports_empty_images() > 0;
 
-  Device.PendingGlobalsMtx.lock();
-  PM->TrlTblMtx.lock();
+  std::lock_guard<decltype(Device.PendingGlobalsMtx)> LG(
+      Device.PendingGlobalsMtx);
+  {
+    std::lock_guard<decltype(PM->TrlTblMtx)> LG(PM->TrlTblMtx);
     for (auto *HostEntriesBegin : PM->HostEntriesBeginRegistrationOrder) {
       TranslationTable *TransTable =
           &PM->HostEntriesBeginToTransTable[HostEntriesBegin];
       if (TransTable->HostTable.EntriesBegin ==
               TransTable->HostTable.EntriesEnd &&
           !supportsEmptyImages) {
         // No host entry so no need to proceed
         continue;
       }
 
       if (TransTable->TargetsTable[device_id] != 0) {
         // Library entries have already been processed
         continue;
       }
 
       // 1) get image.
       assert(TransTable->TargetsImages.size() > (size_t)device_id &&
              "Not expecting a device ID outside the table's bounds!");
       __tgt_device_image *img = TransTable->TargetsImages[device_id];
       if (!img) {
         REPORT("No image loaded for device id %d.\n", device_id);
         rc = OFFLOAD_FAIL;
         break;
       }
       // 2) load image into the target table.
       __tgt_target_table *TargetTable = TransTable->TargetsTable[device_id] =
           Device.load_binary(img);
       // Unable to get table for this image: invalidate image and fail.
       if (!TargetTable) {
-      REPORT("Unable to generate entries table for device id %d.\n", device_id);
+        REPORT("Unable to generate entries table for device id %d.\n",
+               device_id);
         TransTable->TargetsImages[device_id] = 0;
         rc = OFFLOAD_FAIL;
         break;
       }
 
       // Verify whether the two table sizes match.
       size_t hsize =
           TransTable->HostTable.EntriesEnd - TransTable->HostTable.EntriesBegin;
       size_t tsize = TargetTable->EntriesEnd - TargetTable->EntriesBegin;
 
       // Invalid image for these host entries!
       if (hsize != tsize) {
-      REPORT("Host and Target tables mismatch for device id %d [%zx != %zx].\n",
-             device_id, hsize, tsize);
+        REPORT(
+            "Host and Target tables mismatch for device id %d [%zx != %zx].\n",
         TransTable->TargetsImages[device_id] = 0;
         TransTable->TargetsTable[device_id] = 0;
         rc = OFFLOAD_FAIL;
         break;
       }
 
       // process global data that needs to be mapped.
-    Device.DataMapMtx.lock();
+      std::lock_guard<decltype(Device.DataMapMtx)> LG(Device.DataMapMtx);
+
       __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.
+          // the fat binary. Treat these mappings as any other "regular"
+          // mapping. Add entry to map.
           if (Device.getTgtPtrBegin(CurrHostEntry->addr, CurrHostEntry->size))
             continue;
-        DP("Add mapping from host " DPxMOD " to device " DPxMOD " with size %zu"
+          DP("Add mapping from host " DPxMOD " to device " DPxMOD
+             " with size %zu"
              "\n",
              DPxPTR(CurrHostEntry->addr), DPxPTR(CurrDeviceEntry->addr),
              CurrDeviceEntry->size);
           Device.HostDataToTargetMap.emplace(
               (uintptr_t)CurrHostEntry->addr /*HstPtrBase*/,
               (uintptr_t)CurrHostEntry->addr /*HstPtrBegin*/,
-            (uintptr_t)CurrHostEntry->addr + CurrHostEntry->size /*HstPtrEnd*/,
+              (uintptr_t)CurrHostEntry->addr +
+                  CurrHostEntry->size /*HstPtrEnd*/,
               (uintptr_t)CurrDeviceEntry->addr /*TgtPtrBegin*/,
               false /*UseHoldRefCount*/, nullptr /*Name*/,
               true /*IsRefCountINF*/);
         }
       }
-    Device.DataMapMtx.unlock();
     }
-  PM->TrlTblMtx.unlock();
+  }
 
   if (rc != OFFLOAD_SUCCESS) {
-    Device.PendingGlobalsMtx.unlock();
     return rc;
   }
 
   /*
    * Run ctors for static objects
    */
   if (!Device.PendingCtorsDtors.empty()) {
     AsyncInfoTy AsyncInfo(Device);
     // Call all ctors for all libraries registered so far
     for (auto &lib : Device.PendingCtorsDtors) {
       if (!lib.second.PendingCtors.empty()) {
         DP("Has pending ctors... call now\n");
         for (auto &entry : lib.second.PendingCtors) {
           void *ctor = entry;
           int rc =
               target(nullptr, Device, ctor, 0, nullptr, nullptr, nullptr,
                      nullptr, nullptr, nullptr, 1, 1, true /*team*/, AsyncInfo);
           if (rc != OFFLOAD_SUCCESS) {
             REPORT("Running ctor " DPxMOD " failed.\n", DPxPTR(ctor));
-            Device.PendingGlobalsMtx.unlock();
             return OFFLOAD_FAIL;
           }
         }
         // Clear the list to indicate that this device has been used
         lib.second.PendingCtors.clear();
         DP("Done with pending ctors for lib " DPxMOD "\n", DPxPTR(lib.first));
       }
     }
     // All constructors have been issued, wait for them now.
     if (AsyncInfo.synchronize() != OFFLOAD_SUCCESS)
       return OFFLOAD_FAIL;
   }
   Device.HasPendingGlobals = false;
-  Device.PendingGlobalsMtx.unlock();
 
   return OFFLOAD_SUCCESS;
 }
 
 void handleTargetOutcome(bool Success, ident_t *Loc) {
   switch (PM->TargetOffloadPolicy) {
   case tgt_disabled:
     if (Success) {
if (!Success) {
         for (auto &Device : PM->Devices)
           dumpTargetPointerMappings(Loc, *Device);
     }
     break;
   }
 }
 
 static void handleDefaultTargetOffload() {
-  PM->TargetOffloadMtx.lock();
+  std::lock_guard<decltype(PM->TargetOffloadMtx)> LG(PM->TargetOffloadMtx);
   if (PM->TargetOffloadPolicy == tgt_default) {
     if (omp_get_num_devices() > 0) {
       DP("Default TARGET OFFLOAD policy is now mandatory "
          "(devices were found)\n");
       PM->TargetOffloadPolicy = tgt_mandatory;
     } else {
       DP("Default TARGET OFFLOAD policy is now disabled "
          "(no devices were found)\n");
       PM->TargetOffloadPolicy = tgt_disabled;
     }
   }
-  PM->TargetOffloadMtx.unlock();
 }
 
 static bool isOffloadDisabled() {
   if (PM->TargetOffloadPolicy == tgt_default)
     handleDefaultTargetOffload();
   return PM->TargetOffloadPolicy == tgt_disabled;
 }
 
if (!device_is_ready(DeviceID)) {
     handleTargetOutcome(false, Loc);
     return true;
   }
 
   // Get device info.
   DeviceTy &Device = *PM->Devices[DeviceID];
 
   // Check whether global data has been mapped for this device
-  Device.PendingGlobalsMtx.lock();
-  bool hasPendingGlobals = Device.HasPendingGlobals;
-  Device.PendingGlobalsMtx.unlock();
-  if (hasPendingGlobals && InitLibrary(Device) != OFFLOAD_SUCCESS) {
+  bool HasPendingGlobals;
+  {
+    std::lock_guard<decltype(Device.PendingGlobalsMtx)> LG(
+        Device.PendingGlobalsMtx);
+    HasPendingGlobals = Device.HasPendingGlobals;
+  }
+  if (HasPendingGlobals && InitLibrary(Device) != OFFLOAD_SUCCESS) {
     REPORT("Failed to init globals on device %" PRId64 "\n", DeviceID);
     handleTargetOutcome(false, Loc);
     return true;
   }
 
   return false;
 }
 
if (arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ && !IsHostPtr) {
         // create or update shadow pointers for this entry
         Device.ShadowPtrMap[Pointer_HstPtrBegin] = {
             HstPtrBase, PointerTgtPtrBegin, ExpectedTgtPtrBase};
         Pointer_TPR.MapTableEntry->setMayContainAttachedPointers();
         UpdateDevPtr = true;
       }
 
       if (UpdateDevPtr) {
-        HostDataToTargetTy::LockGuard LG(*Pointer_TPR.MapTableEntry);
+        std::lock_guard<decltype(*Pointer_TPR.MapTableEntry)> LG(
+            *Pointer_TPR.MapTableEntry);
         Device.ShadowMtx.unlock();
 
         DP("Update pointer (" DPxMOD ") -> [" DPxMOD "]\n",
            DPxPTR(PointerTgtPtrBegin), DPxPTR(TgtPtrBegin));
 
         void *&TgtPtrBase = AsyncInfo.getVoidPtrLocation();
         TgtPtrBase = ExpectedTgtPtrBase;
 
static void applyToShadowMapEntries(DeviceTy &Device, CBTy CB, void *Begin,
   // pointers, no need to do any checking.
   if (TPR.MapTableEntry == HostDataToTargetListTy::iterator{} ||
       !TPR.MapTableEntry->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.
-  Device.ShadowMtx.lock();
+  std::lock_guard<decltype(Device.ShadowMtx)> LG(Device.ShadowMtx);
   for (ShadowPtrListTy::iterator Itr = Device.ShadowPtrMap.begin();
        Itr != Device.ShadowPtrMap.end();) {
     uintptr_t ShadowHstPtrAddr = (uintptr_t)Itr->first;
 
     // An STL map is sorted on its keys; use this property
     // to quickly determine when to break out of the loop.
     if (ShadowHstPtrAddr < LB) {
       ++Itr;
       continue;
     }
     if (ShadowHstPtrAddr >= UB)
       break;
 
     if (CB(Itr) == OFFLOAD_FAIL)
       break;
   }
-  Device.ShadowMtx.unlock();
 }
 
 } // namespace
 
 /// Internal function to undo the mapping and retrieve the data from the device.
 int targetDataEnd(ident_t *loc, DeviceTy &Device, int32_t ArgNum,
                   void **ArgBases, void **Args, int64_t *ArgSizes,
                   int64_t *ArgTypes, map_var_info_t *ArgNames,