[Openmp-commits] [PATCH] D121057: [OpenMP][FIX] Ensure exclusive access to the HDTT map

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

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.



================
Comment at: openmp/libomptarget/include/ExclusiveAccess.h:35
+
+/// Helper to provide trnasparent exclusive access to protected objects.
+template <typename Ty> struct Accessor {
----------------
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.


================
Comment at: openmp/libomptarget/include/ExclusiveAccess.h:41
+  Accessor(ProtectedObj<Ty> &PO, bool DoNotGetAccess = false)
+      : Ptr(DoNotGetAccess ? nullptr : &PO) {
+    lock();
----------------
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.


================
Comment at: openmp/libomptarget/include/ExclusiveAccess.h:61
+  /// Provide transparent access to the underlying object.
+  Ty &operator*() { return Ptr->Obj; }
+  Ty *operator->() { return &Ptr->Obj; }
----------------
Null dereference looks likely here. Maybe an assert to get friendlier notice on the mistake.


================
Comment at: openmp/libomptarget/include/ExclusiveAccess.h:83
+template <typename Ty>
+Accessor<Ty> &&ProtectedObj<Ty>::getExclusiveAccessor(bool DoNotGetAccess) {
+  return std::move(Accessor<Ty>(*this, DoNotGetAccess));
----------------
Not sure we need the move here, it's already an R value. Also not sure we need the && on the return type.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121057/new/

https://reviews.llvm.org/D121057