[libomptarget] Keep the Shadow Pointer Map up-to-date

[grokos]

Summary

D105812 introduced a regression where if a PTR_AND_OBJ entry was mapped on the device, later the OBJ was deallocated and then reallocated at a different address, the Shadow Pointer Map would still contain an entry for the PTR but pointing to the old address. This caused test env/base_ptr_ref_count.c to fail.

[grokos]

@jdenny So it seems we found a valid OpenMP program that suffered from the corner case you warned about in D105812.

[tianshilei1992]

Don't we just update the map afterwards?

[grokos]

No. If the old entry exists in ShadowPtrMap then UpdateDevPtr will remain false, so no update will take place. This patch changes that condition: if either the entry is not in the map or it exists but contains stale data, an update should be performed.

[tianshilei1992]

I mean when OBJ is deallocated, it is not removed from the map?

[grokos]

No, only when the PTR is deallocated is the entry removed from the map. But in env/base_ptr_ref_count.c the PTR is a global declare target pointer, so it stays in the map for the lifetime of the application.

[jdenny]

Other than my comment suggestion, LGTM. Thanks.

openmp/libomptarget/src/omptarget.cpp

569

Can you extend this comment a bit more to address @tianshilei1992's question? That is, explain that the shadow map entry is added/removed only when the PTR is allocated/deallocated on the device, but TgtPtrVal becomes stale when the OBJ is allocated/deallocated on the device. I found this point confusing as well.

[grokos]

Made the comment more detailed to explain why this patch is needed.

[jdenny]

Still LGTM. Thanks for the comment.

openmp/libomptarget/src/omptarget.cpp

if (arg_types[i] & OMP_TGT_MAPTYPE_PTR_AND_OBJ && !IsHostPtr) {
       // Check whether we need to update the pointer on the device
       bool UpdateDevPtr = false;
 
       uint64_t Delta = (uint64_t)HstPtrBegin - (uint64_t)HstPtrBase;
       void *ExpectedTgtPtrBase = (void *)((uint64_t)TgtPtrBegin - Delta);
 
       Device.ShadowMtx.lock();
       auto Entry = Device.ShadowPtrMap.find(Pointer_HstPtrBegin);
-      // If this pointer is not in the map we need to insert it.
-      if (Entry == Device.ShadowPtrMap.end()) {
-        // create shadow pointers for this entry
+      // If this pointer is not in the map we need to insert it. If the map
+      // contains a stale entry, we need to update it (e.g. if the pointee was
+      // deallocated and later on is reallocated at another device address). The
+      // latter scenario is the subject of LIT test env/base_ptr_ref_count.c. An
+      // entry is removed from ShadowPtrMap only when the PTR of a PTR_AND_OBJ
+      // pair is deallocated, not when the OBJ is deallocated. In
+      // env/base_ptr_ref_count.c the PTR is a global "declare target" pointer,
+      // so it stays in the map for the lifetime of the application. When the
+      // 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};
         UpdateDevPtr = true;
       }
       Device.ShadowMtx.unlock();
 
       if (UpdateDevPtr) {
         DP("Update pointer (" DPxMOD ") -> [" DPxMOD "]\n",