[llvm] [OpenMP][Offload] Handle `present/to/from` when a different entry did `alloc/delete`. (PR #165494)

[Robert Imschweiler via llvm-commits]

================
@@ -1193,10 +1243,128 @@ int targetDataEnd(ident_t *Loc, DeviceTy &Device, int32_t ArgNum,
       // copy-back was issued but before it completed. Since the reuse might
       // also copy-back a value we would race.
       if (TPR.Flags.IsLast) {
-        if (TPR.getEntry()->addEventIfNecessary(Device, AsyncInfo) !=
-            OFFLOAD_SUCCESS)
+        if (Entry->addEventIfNecessary(Device, AsyncInfo) != OFFLOAD_SUCCESS)
           return OFFLOAD_FAIL;
       }
+
+      // Track this transfer to avoid duplicate transfers later on.
+      StateInfo->addTransferredFromEntry(HstPtr, Size);
+
+      return OFFLOAD_SUCCESS;
+    };
+
+    // Lambda to check if this pointer was previously released.
+    //
+    // This is needed to handle cases like the following:
+    //   p1 = p2 = &x;
+    //   ... map(delete: p1[:]) map(from: p2[0:1])
+    // The ref-count becomes zero before encountering the FROM entry, but we
+    // still need to do a transfer, if it went from non-zero to zero.
+    //
+    // OpenMP 6.0, sec. 7.9.6 "map Clause", p. 284 L24-26:
+    // If the reference count of the corresponding list item is one or if
+    // the always-modifier or delete-modifier is specified, and if the map
+    // type is from, the original list item is updated as if the list item
+    // appeared in a from clause on a target_update directive.
+    auto WasPreviouslyReleased = [&]() -> bool {
+      auto ReleasedEntry = StateInfo->wasPreviouslyReleased(HstPtrBegin);
+      if (!ReleasedEntry)
+        return false;
+
+      void *ReleasedPtr = ReleasedEntry->first;
+      int64_t ReleasedSize = ReleasedEntry->second;
+      ODBG(ODT_Mapping) << "Pointer HstPtr=" << HstPtrBegin
+                        << " falls within a range previously released ["
+                        << ReleasedPtr << ", "
+                        << static_cast<void *>(
+                               static_cast<char *>(ReleasedPtr) + ReleasedSize)
+                        << ") with size=" << ReleasedSize;
+      return true;
+    };
+
+    bool IsMapFromOnNonHostNonZeroData =
+        HasFrom && !TPR.Flags.IsHostPointer && DataSize != 0;
+
+    auto IsLastOrHasAlwaysOrWasReleased = [&]() {
+      return TPR.Flags.IsLast || HasAlways || WasPreviouslyReleased();
+    };
+
+    if (IsMapFromOnNonHostNonZeroData && IsLastOrHasAlwaysOrWasReleased()) {
+      Ret = PerformFromRetrieval(HstPtrBegin, TgtPtrBegin, DataSize,
+                                 TPR.getEntry());
+      if (Ret != OFFLOAD_SUCCESS)
+        return OFFLOAD_FAIL;
+    } else if (IsMapFromOnNonHostNonZeroData) {
+      // We can have cases like the following:
+      //   p1 = p2 = &x;
+      //  ... map(storage: p1[:]) map(from: p2[1:1])
+      //
+      // where it's possible that when the FROM entry is processed, the
+      // ref count is not zero, so no data transfer happens for it. But
+      // the ref-count can go down to zero once all maps have been processed
+      // for the current construct, in which case a transfer should happen.
+      //
+      // So, we keep track of any skipped FROM data-transfers, in case
+      // the ref-count goes down to zero later on.
+      //
+      // This cannot be handled in the compiler for all cases because the
+      // list-items may look very different, as shown in the example above,
+      // which is allowed with OpenMP 6.0:
+      //
+      // OpenMP 6.0, sec. 7.9.6 "map Clause", p. 286 L18-21:
+      // Two list items of the map clauses on the same construct must not share
+      // original storage unless one of the following is true: they are the same
+      // list item, one is the containing structure of the other, at least one
+      // is an assumed-size array, or at least one is implicitly mapped due to
+      // the list item also appearing in a use_device_addr clause.
+      StateInfo->addSkippedFromEntry(HstPtrBegin, DataSize);
+      ODBG(ODT_Mapping) << "Skipping FROM map transfer for HstPtr="
+                        << HstPtrBegin << " size=" << DataSize
+                        << " (IsLast=" << TPR.Flags.IsLast << ", TotalRefCount="
+                        << TPR.getEntry()->getTotalRefCount() << ")";
+    }
+
+    // If the ref-count went to zero (IsLast=true), check if any previously
+    // skipped FROM entries fall within this released entry's range.
+    if (TPR.Flags.IsLast && !StateInfo->SkippedFromEntries.empty()) {
+      uintptr_t ReleasedBeginPtrInt = TPR.getEntry()->HstPtrBegin;
+      uintptr_t ReleasedEndPtrInt = TPR.getEntry()->HstPtrEnd;
+      SmallVector<void *, 32> ToRemove;
+
+      for (auto &SkippedFromEntry : StateInfo->SkippedFromEntries) {
+        void *FromBeginPtr = SkippedFromEntry.first;
+        int64_t FromDataSize = SkippedFromEntry.second;
+        uintptr_t FromBeginPtrInt = reinterpret_cast<uintptr_t>(FromBeginPtr);
+
+        // Check if this skipped FROM entry's starting pointer falls within this
+        // released entry
+        if (FromBeginPtrInt >= ReleasedBeginPtrInt &&
+            FromBeginPtrInt < ReleasedEndPtrInt) {
----------------
ro-i wrote:

why do we only check if the starting pointer is within the released entry? What if `FromDataSize` would lead to an out-of-bounds read below when fetching that size from `FromTgtBeginPtr`?

https://github.com/llvm/llvm-project/pull/165494