[libomptarget] PR38704: Fix erase of ShadowPtrMap

[Hahnfeld]

Summary

erase() invalidates the iterator and returns a new one pointing
to the following element. The code now follows the example at
https://en.cppreference.com/w/cpp/container/map/erase.
(The added testcase crashes without this patch.)

[grokos]

Good catch, thanks for the fix!

[Hahnfeld]

In D51623#1223117, @grokos wrote:

Good catch, thanks for the fix!

Thank David Binderman and cppcheck ;-) (will have a link in the commit message)

[AlexEichenberger]

make sense, thanks

[sdmitriev]

Not related to this patch, but I think there is one more potentially incorrect use of containers in RTLsTy::RegisterLib(). This function may increase size of the global vector Devices when it initializes a new RTL that has not been yet used (see rtl.cpp line 219). This may result in memory reallocation and thus all pointers to the Devices elements that are stored in RTLs (see rtl.cpp line 228) would become invalid. I am not sure if these pointers are really used by RTLs now, but if yes, than it is a problem. RTLsTy::RegisterLib may be called more than once and each call may potentially initialize new RTLs.

[Hahnfeld]

In D51623#1223437, @sdmitriev wrote:

Not related to this patch, but I think there is one more potentially incorrect use of containers in RTLsTy::RegisterLib(). This function may increase size of the global vector Devices when it initializes a new RTL that has not been yet used (see rtl.cpp line 219). This may result in memory reallocation and thus all pointers to the Devices elements that are stored in RTLs (see rtl.cpp line 228) would become invalid. I am not sure if these pointers are really used by RTLs now, but if yes, than it is a problem. RTLsTy::RegisterLib may be called more than once and each call may potentially initialize new RTLs.

Hehe, turns out: Yes and no!
Yes, this is undoubtly incorrect. But no, this can never happen because nobody is accessing the RTLInfoTy::Devices :D

Do you want to submit a patch removing Devices from RTLInfoTy?

[sdmitriev]

Ok, I will submit such patch.

openmp/trunk/libomptarget/src/omptarget.cpp

if ((arg_types[i] & OMP_TGT_MAPTYPE_FROM) || DelEntry) {
       // If we copied back to the host a struct/array containing pointers, we
       // need to restore the original host pointer values from their shadow
       // copies. If the struct is going to be deallocated, remove any remaining
       // shadow pointer entries for this struct.
       uintptr_t lb = (uintptr_t) HstPtrBegin;
       uintptr_t ub = (uintptr_t) HstPtrBegin + data_size;
       Device.ShadowMtx.lock();
       for (ShadowPtrListTy::iterator it = Device.ShadowPtrMap.begin();
-          it != Device.ShadowPtrMap.end(); ++it) {
+           it != Device.ShadowPtrMap.end();) {
         void **ShadowHstPtrAddr = (void**) it->first;
 
         // An STL map is sorted on its keys; use this property
         // to quickly determine when to break out of the loop.
         if ((uintptr_t) ShadowHstPtrAddr < lb)
           continue;
         if ((uintptr_t) ShadowHstPtrAddr >= ub)
           break;
 
         // If we copied the struct to the host, we need to restore the pointer.
         if (arg_types[i] & OMP_TGT_MAPTYPE_FROM) {
           DP("Restoring original host pointer value " DPxMOD " for host "
               "pointer " DPxMOD "\n", DPxPTR(it->second.HstPtrVal),
               DPxPTR(ShadowHstPtrAddr));
           *ShadowHstPtrAddr = it->second.HstPtrVal;
         }
         // If the struct is to be deallocated, remove the shadow entry.
         if (DelEntry) {
           DP("Removing shadow pointer " DPxMOD "\n", DPxPTR(ShadowHstPtrAddr));
-          Device.ShadowPtrMap.erase(it);
+          it = Device.ShadowPtrMap.erase(it);
+        } else {
+          ++it;
         }
       }
       Device.ShadowMtx.unlock();
 
       // Deallocate map
       if (DelEntry) {
         int rt = Device.deallocTgtPtr(HstPtrBegin, data_size, ForceDelete);
         if (rt != OFFLOAD_SUCCESS) {

openmp/trunk/libomptarget/test/mapping/pr38704.c

+// RUN: %libomptarget-compile-run-and-check-aarch64-unknown-linux-gnu
+// RUN: %libomptarget-compile-run-and-check-powerpc64-ibm-linux-gnu
+// RUN: %libomptarget-compile-run-and-check-powerpc64le-ibm-linux-gnu
+// RUN: %libomptarget-compile-run-and-check-x86_64-pc-linux-gnu
+
+// Clang 6.0 doesn't use the new map interface, undefined behavior when
+// the compiler emits "old" interface code for structures.
+// UNSUPPORTED: clang-6
+
+#include <stdio.h>
+#include <stdlib.h>
+
+typedef struct {
+  int *ptr1;
+  int *ptr2;
+} StructWithPtrs;
+
+int main(int argc, char *argv[]) {
+  StructWithPtrs s;
+  s.ptr1 = malloc(sizeof(int));
+  s.ptr2 = malloc(2 * sizeof(int));
+
+#pragma omp target map(s.ptr1[0:1], s.ptr2[0:2])
+  {
+    s.ptr1[0] = 1;
+    s.ptr2[0] = 2;
+    s.ptr2[1] = 3;
+  }
+
+  // CHECK: s.ptr1[0] = 1
+  // CHECK: s.ptr2[0] = 2
+  // CHECK: s.ptr2[1] = 3
+  printf("s.ptr1[0] = %d\n", s.ptr1[0]);
+  printf("s.ptr2[0] = %d\n", s.ptr2[0]);
+  printf("s.ptr2[1] = %d\n", s.ptr2[1]);
+
+  free(s.ptr1);
+  free(s.ptr2);
+
+  return 0;
+}