openacc: Deep copy attach/detach should not affect reference counts

Attach and detach operations are not supposed to affect structural or
dynamic reference counts for OpenACC. Previously they did so, which led to
subtle problems in some circumstances. We can avoid reference-counting
attach/detach operations by extending and slightly repurposing the
do_detach field in target_var_desc. It is now called is_attach to better
reflect its new role.

2020-07-27  Julian Brown  <julian@codesourcery.com>
	    Thomas Schwinge  <thomas@codesourcery.com>

libgomp/
	* libgomp.h (struct target_var_desc): Rename do_detach field to
	is_attach.
	* oacc-mem.c (goacc_exit_datum_1): Add assert.  Don't set finalize for
	GOMP_MAP_FORCE_DETACH. Update checking to use is_attach field.
	(goacc_enter_data_internal): Don't affect reference counts
	for attach mappings.
	(goacc_exit_data_internal): Don't affect reference counts for detach
	mappings.
	* target.c (gomp_map_vars_existing): Don't affect reference counts for
	attach mappings.
	(gomp_map_vars_internal): Set renamed is_attach flag unconditionally to
	mark attach mappings.
	(gomp_unmap_vars_internal): Use is_attach flag to prevent affecting
	reference count for attach mappings.
	* testsuite/libgomp.oacc-c-c++-common/mdc-refcount-1.c: New test.
	* testsuite/libgomp.oacc-c-c++-common/mdc-refcount-2.c: New test.
	* testsuite/libgomp.oacc-c-c++-common/mdc-refcount-2.c: New test.
	* testsuite/libgomp.oacc-fortran/deep-copy-6-no_finalize.F90: Mark
	test as shouldfail.
	* testsuite/libgomp.oacc-fortran/deep-copy-6.f90: Adjust to fail
	gracefully in no-finalize mode.

Co-Authored-By: Thomas Schwinge <thomas@codesourcery.com>

5 files changed, 284 insertions, 2 deletions

diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/mdc-refcount-1.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/mdc-refcount-1.c
new file mode 100644
index 0000000..6170447
--- /dev/null
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/mdc-refcount-1.c
@@ -0,0 +1,60 @@
+/* { dg-skip-if "" { *-*-* } { "-DACC_MEM_SHARED=1" } } */
+
+#include <openacc.h>
+#include <assert.h>
+
+#define N 1024
+
+struct mystr {
+  int *data;
+};
+
+static void
+test (unsigned variant)
+{
+  int arr[N];
+  struct mystr s;
+
+  s.data = arr;
+
+  acc_copyin (&s, sizeof (s));
+  acc_create (s.data, N * sizeof (int));
+
+  for (int i = 0; i < 20; i++)
+    {
+      if ((variant + i) % 1)
+	{
+#pragma acc enter data attach(s.data)
+	}
+      else
+	acc_attach ((void **) &s.data);
+
+      if ((variant + i) % 2)
+	{
+#pragma acc exit data detach(s.data)
+	}
+      else
+	acc_detach ((void **) &s.data);
+    }
+
+  assert (acc_is_present (arr, N * sizeof (int)));
+  assert (acc_is_present (&s, sizeof (s)));
+
+  acc_delete (arr, N * sizeof (int));
+
+  assert (!acc_is_present (arr, N * sizeof (int)));
+
+  acc_copyout (&s, sizeof (s));
+
+  assert (!acc_is_present (&s, sizeof (s)));
+  assert (s.data == arr);
+}
+
+int
+main (int argc, char *argv[])
+{
+  for (unsigned variant = 0; variant < 4; ++variant)
+    test (variant);
+
+  return 0;
+}
diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/mdc-refcount-2.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/mdc-refcount-2.c
new file mode 100644
index 0000000..2431a76
--- /dev/null
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/mdc-refcount-2.c
@@ -0,0 +1,123 @@
+/* Verify that OpenACC 'attach'/'detach' doesn't interfere with reference
+   counting.  */
+
+#include <assert.h>
+#include <stdlib.h>
+#include <openacc.h>
+
+/* Need to shared this (and, in particular, implicit '&data_work' in
+   'attach'/'detach' clauses) between 'test' and 'test_'.  */
+static unsigned char *data_work;
+
+static void test_(unsigned variant,
+		  unsigned char *data,
+		  void *data_d)
+{
+  assert(acc_is_present(&data_work, sizeof data_work));
+  assert(data_work == data);
+
+  acc_update_self(&data_work, sizeof data_work);
+  assert(data_work == data);
+
+  if (variant & 1)
+    {
+#pragma acc enter data attach(data_work)
+    }
+  else
+    acc_attach((void **) &data_work);
+  acc_update_self(&data_work, sizeof data_work);
+  assert(data_work == data_d);
+
+  if (variant & 4)
+    {
+      if (variant & 2)
+	{ // attach some more
+	  data_work = data;
+	  acc_attach((void **) &data_work);
+#pragma acc enter data attach(data_work)
+	  acc_attach((void **) &data_work);
+#pragma acc enter data attach(data_work)
+#pragma acc enter data attach(data_work)
+#pragma acc enter data attach(data_work)
+	  acc_attach((void **) &data_work);
+	  acc_attach((void **) &data_work);
+#pragma acc enter data attach(data_work)
+	}
+      else
+	{}
+    }
+  else
+    { // detach
+      data_work = data;
+      if (variant & 2)
+	{
+#pragma acc exit data detach(data_work)
+	}
+      else
+	acc_detach((void **) &data_work);
+      acc_update_self(&data_work, sizeof data_work);
+      assert(data_work == data);
+
+      // now not attached anymore
+
+#if 0
+      if (TODO)
+	{
+	  acc_detach(&data_work); //TODO PR95203 "libgomp: attach count underflow"
+	  acc_update_self(&data_work, sizeof data_work);
+	  assert(data_work == data);
+	}
+#endif
+    }
+
+  assert(acc_is_present(&data_work, sizeof data_work));
+}
+
+static void test(unsigned variant)
+{
+  const int size = sizeof (void *);
+  unsigned char *data = (unsigned char *) malloc(size);
+  assert(data);
+  void *data_d = acc_create(data, size);
+  assert(data_d);
+  assert(acc_is_present(data, size));
+
+  data_work = data;
+
+  if (variant & 8)
+    {
+#pragma acc data copyin(data_work)
+      test_(variant, data, data_d);
+    }
+  else
+    {
+      acc_copyin(&data_work, sizeof data_work);
+      test_(variant, data, data_d);
+      acc_delete(&data_work, sizeof data_work);
+    }
+#if ACC_MEM_SHARED
+  assert(acc_is_present(&data_work, sizeof data_work));
+#else
+  assert(!acc_is_present(&data_work, sizeof data_work));
+#endif
+  data_work = NULL;
+
+  assert(acc_is_present(data, size));
+  acc_delete(data, size);
+  data_d = NULL;
+#if ACC_MEM_SHARED
+  assert(acc_is_present(data, size));
+#else
+  assert(!acc_is_present(data, size));
+#endif
+  free(data);
+  data = NULL;
+}
+
+int main()
+{
+  for (size_t i = 0; i < 16; ++i)
+    test(i);
+
+  return 0;
+}
diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/mdc-refcount-3.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/mdc-refcount-3.c
new file mode 100644
index 0000000..0f5e7be
--- /dev/null
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/mdc-refcount-3.c
@@ -0,0 +1,86 @@
+/* { dg-skip-if "" { *-*-* } { "-DACC_MEM_SHARED=1" } } */
+
+/* Variant of 'deep-copy-7.c'.  */
+
+#include <stdlib.h>
+#include <assert.h>
+#include <openacc.h>
+
+struct dc
+{
+  int a;
+  int *b;
+};
+
+int
+main ()
+{
+  int n = 100, i, j, k;
+  struct dc v = { .a = 3 };
+
+  v.b = (int *) malloc (sizeof (int) * n);
+
+  for (k = 0; k < 16; k++)
+    {
+      /* Here, we do not explicitly copy the enclosing structure, but work
+	 with fields directly.  Make sure attachment counters and reference
+	 counters work properly in that case.  */
+#pragma acc enter data copyin(v.a, v.b[0:n]) // 1
+      assert (acc_is_present (&v.b, sizeof v.b));
+      assert (acc_is_present (v.b, sizeof (int) * n));
+#pragma acc enter data pcopyin(v.b[0:n]) // 2
+#pragma acc enter data pcopyin(v.b[0:n]) // 3
+
+#pragma acc parallel loop present(v.a, v.b)
+      for (i = 0; i < n; i++)
+	v.b[i] = k + v.a + i;
+
+      switch (k % 5)
+	{ // All optional.
+	case 0:
+	  break;
+	case 1:
+	  ; //TODO PR95901
+#pragma acc exit data detach(v.b) finalize
+	  break;
+	case 2:
+	  ; //TODO PR95901
+#pragma acc exit data detach(v.b)
+	  break;
+	case 3:
+	  acc_detach_finalize ((void **) &v.b);
+	  break;
+	case 4:
+	  acc_detach ((void **) &v.b);
+	  break;
+	}
+      assert (acc_is_present (&v.b, sizeof v.b));
+      assert (acc_is_present (v.b, sizeof (int) * n));
+      { // 3
+	acc_delete (&v.b, sizeof v.b);
+	assert (acc_is_present (&v.b, sizeof v.b));
+	acc_copyout (v.b, sizeof (int) * n);
+	assert (acc_is_present (v.b, sizeof (int) * n));
+      }
+      { // 2
+	acc_delete (&v.b, sizeof v.b);
+	assert (acc_is_present (&v.b, sizeof v.b));
+	acc_copyout (v.b, sizeof (int) * n);
+	assert (acc_is_present (v.b, sizeof (int) * n));
+      }
+      { // 1
+	acc_delete (&v.b, sizeof v.b);
+	assert (!acc_is_present (&v.b, sizeof v.b));
+	acc_copyout (v.b, sizeof (int) * n);
+	assert (!acc_is_present (v.b, sizeof (int) * n));
+      }
+#pragma acc exit data delete(v.a)
+
+      for (i = 0; i < n; i++)
+	assert (v.b[i] == k + v.a + i);
+
+      assert (!acc_is_present (&v, sizeof (v)));
+    }
+
+  return 0;
+}
diff --git a/libgomp/testsuite/libgomp.oacc-fortran/deep-copy-6-no_finalize.F90 b/libgomp/testsuite/libgomp.oacc-fortran/deep-copy-6-no_finalize.F90
index 038f04a..1daff2d 100644
--- a/libgomp/testsuite/libgomp.oacc-fortran/deep-copy-6-no_finalize.F90
+++ b/libgomp/testsuite/libgomp.oacc-fortran/deep-copy-6-no_finalize.F90
@@ -1,5 +1,12 @@
 ! { dg-do run }
 
-/* Nullify the 'finalize' clause.  */
+/* Nullify the 'finalize' clause.
+
+   That means, we do not detach properly, the host sees a device pointer, and
+   we fail as follows.
+   { dg-output "STOP 30(\n|\r\n|\r)+" { target { ! openacc_host_selected } } }
+   { dg-shouldfail "" { ! openacc_host_selected } }
+*/
 #define finalize
 #include "deep-copy-6.f90"
+
diff --git a/libgomp/testsuite/libgomp.oacc-fortran/deep-copy-6.f90 b/libgomp/testsuite/libgomp.oacc-fortran/deep-copy-6.f90
index 6aab6a4..94ddca3 100644
--- a/libgomp/testsuite/libgomp.oacc-fortran/deep-copy-6.f90
+++ b/libgomp/testsuite/libgomp.oacc-fortran/deep-copy-6.f90
@@ -12,11 +12,14 @@ program dtype
   end type mytype
   integer i
 
-  type(mytype) :: var
+  type(mytype), target :: var
+  integer, pointer :: hostptr(:)
 
   allocate(var%a(1:n))
   allocate(var%b(1:n))
 
+  hostptr => var%a
+
 !$acc data copy(var)
 
   do i = 1, n
@@ -49,6 +52,9 @@ program dtype
 
 !$acc end data
 
+  ! See 'deep-copy-6-no_finalize.F90'.
+  if (.not. associated(hostptr, var%a)) stop 30
+
   do i = 1,4
     if (var%a(i) .ne. 0) stop 1
     if (var%b(i) .ne. 0) stop 2