upcxx_memberof_general prohibits member designators that end with an array access

[Amir Kamil]

Test program that fails:

#include <upcxx/upcxx.hpp>

struct A {
  int arr[3];
};

int main() {
  upcxx::init();
  auto ptr = upcxx::new_<A>();
  auto fut = upcxx_memberof_general(ptr, arr[1]);
  upcxx::delete_(ptr);
  upcxx::finalize();
}

Failure:

In file included from /Users/akamil/work/upcxx-akamil/bld/upcxx.assert1.optlev0.dbgsym1.gasnet_seq.smp/include/upcxx/backend_fwd.hpp:195,
                 from /Users/akamil/work/upcxx-akamil/bld/upcxx.assert1.optlev0.dbgsym1.gasnet_seq.smp/include/upcxx/backend.hpp:4,
                 from /Users/akamil/work/upcxx-akamil/bld/upcxx.assert1.optlev0.dbgsym1.gasnet_seq.smp/include/upcxx/allocate.hpp:8,
                 from /Users/akamil/work/upcxx-akamil/bld/upcxx.assert1.optlev0.dbgsym1.gasnet_seq.smp/include/upcxx/upcxx.hpp:5,
                 from /Users/akamil/work/upcxx-akamil/test/foo.cpp:1:
/Users/akamil/work/upcxx-akamil/test/foo.cpp: In lambda function:
/Users/akamil/work/upcxx-akamil/bld/upcxx.assert1.optlev0.dbgsym1.gasnet_seq.smp/include/upcxx/memberof.hpp:134:23: error: static assertion failed: upcxx_memberof_general may not be used on fields with reference type.
  134 |   UPCXX_STATIC_ASSERT(!::std::is_reference<decltype(::std::declval<UPCXX_ETYPE(gp)>().FIELD)>::value, \
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/akamil/work/upcxx-akamil/bld/upcxx.assert1.optlev0.dbgsym1.gasnet_seq.smp/include/upcxx/diagnostic.hpp:56:60: note: in definition of macro 'UPCXX_STATIC_ASSERT'
   56 | #define UPCXX_STATIC_ASSERT(cnd, msg) ([=](){static_assert(cnd, msg);})
      |                                                            ^~~
/Users/akamil/work/upcxx-akamil/test/foo.cpp:10:14: note: in expansion of macro 'upcxx_memberof_general'
   10 |   auto fut = upcxx_memberof_general(ptr, arr[1]);
      |              ^~~~~~~~~~~~~~~~~~~~~~

The problem is our diagnostic for erroneously using upcxx_memberof_general to obtain a member that is a reference. Unfortunately, we do not have a mechanism for distinguishing between this erroneous case and the valid case of accessing an array element. An array access is semantically equivalent to a pointer dereference, so the result is of reference type.

Options:

1. Keep the diagnostic and explicitly prohibit member designators that end with array access in the spec.
2. Remove the diagnostic. The implementation seems to handle this case correctly outside of the diagnostic.
3. Replace the current compile-time diagnostic with a runtime heuristic that checks whether the resulting global_ptr references memory within the bounds of the target object. I believe this will catch most erroneous cases of interest.

‌

[Dan Bonachea]

It's worth noting this compile error does not affect the 2020.3.0 release or 2020.3.2 hotfix branches, which predated the assertion added by d7a9576 in pull request #201. However I agree this is a problem we need to address (pun intended).

Setting aside the case of the array field, the assertion is meant to catch genuinely erroneous cases with a somewhat friendly error message. Without the assertion I believe it's possible to construct erroneous cases that fail in less friendly ways, including silent data corruption (eg if one creates a global_ptr to a reference-typed field). Example:

struct B {
  int &x;
  B(int &v) : x(v) {}
};
int main() {
  upcxx::init();
  int stack_var;
  auto pB = upcxx::new_<B>(stack_var);
  auto fB = upcxx_memberof_general(pB, x);
  upcxx::global_ptr<int> gpx = fB.wait(); // trouble: this is a gptr to a stack address, which is invalid
  upcxx::rput(42,gpx); // BOOM
}

In the previous release, this compiles without error. In debug mode the gptr debug checking will detect the pointer outside the shared heap and issue a fatal error, but in ndebug mode this could lead to silent data corruption or obscure crashes. Of course one can also create cases where that reference field points outside the object but still within the shared heap, which no current checking will catch (but still seems contrary to the purpose of memberof).

In pull request #201 and spec issue #158 we prohibited memberof on reference fields and added the static_assert to detect this mistake statically. Removing the diagnostic (Amir's option 2) reverts that safety check, so I don't really like that idea in isolation.

Option 3 still relinquishes static checking, but at least in debug mode we can provide a more specific fatal error at runtime, something like: (lightly tested)

--- a/src/memberof.hpp
+++ b/src/memberof.hpp
@@ -122,10 +122,14 @@ auto memberof_general_helper(global_ptr<Obj,Kind> gptr, Get getter)
 }} // namespace upcxx::detail

 #define upcxx_memberof_general(gp, FIELD) ( \
-  UPCXX_STATIC_ASSERT(!::std::is_reference<decltype(::std::declval<UPCXX_ETYPE(gp)>().FIELD)>::value, \
-    "upcxx_memberof_general may not be used on fields with reference type."), \
   ::upcxx::detail::memberof_general_helper((gp), \
-    [](UPCXX_ETYPE(gp) *lptr) { return ::std::addressof(lptr->FIELD); } \
+    [](UPCXX_ETYPE(gp) *lptr) { \
+      auto field_ptr = ::std::addressof(lptr->FIELD); \
+      UPCXX_ASSERT(std::uintptr_t(field_ptr) - ::std::uintptr_t(lptr) < (uintptr_t)sizeof(*lptr), \
+        "upcxx_memberof_general may not be used on fields with reference type." \
+      ); \
+      return field_ptr; \
+    } \
   ) \
 )

Of course this doesn't reliably detect use on reference-typed fields (which the dev spec forbids), as such fields that reference another part of the same object will not generate an error. So we'd be enforcing a weaker restriction (and if we adopt this resolution, the spec should possibly be weakened to match?).

I don't like Option 1 as-is because it leaves users with no way to access one element of a bottom-level field that has array type. In particular, note that something like upcxx_memberof_general(ptr, arr).wait() actually generates a upcxx::global_ptr<int[3]>, which is a rather unfriendly type. However, if we decided to decree/implement that this call decayed into a upcxx::global_ptr<int> to the base of the array field, then the user could compute the gptr he wants via pointer arithmetic (ie upcxx_memberof_general(ptr, arr).wait() + 1).

Thoughts?

[Amir Kamil]

I agree that global_ptr<int[3]> is rather unfriendly (though it could be useful if we decide that arrays are Serializable). However, array-to-pointer decay only works for the last dimension of a multidimensional array, possibly leading to even more confusion for a user if they access a multidimensional array and expect it to decay to a simple pointer. So I’m not too fond of that solution.

[Dan Bonachea]

However, array-to-pointer decay only works for the last dimension of a multidimensional array

You're correct for std::decay, but we can specify upcxx_memberof(_general) to do whatever we want.

If we specify these macros decay multi-dimensional array fields down to a global_ptr<base_element_type>, then the user gets exactly the type they need for passing to RMA functions, which seems like a very good reason to do it that way (as that's the main use case here). It also means they can use these macros to generate a base pointer to the first element in their field and use pointer arithmetic to easily select the element (or range of elements) they want, with no funny casts. This seems better than anything that generates a confusing upcxx::global_ptr<int[3]> type, and it means we can even restore the static prohibition against reference fields.

Proof-of-concept of a multi-D decay utility I just whipped up in vanilla C++11:

#include <type_traits>
#include <utility>


template<typename T1, typename T2>
struct assert_same {
  static_assert(std::is_same<T1, T2>::value, "types differ");
};

template<typename T>
struct global_ptr { }; // demo proxy

//------------------------------------
template<typename T>
struct decay_helper {
  using type = T;
};
template<typename T>
struct decay_helper<T[]> {
  using type = typename decay_helper<T>::type;
};
template<typename T, std::size_t N>
struct decay_helper<T[N]> {
  using type = typename decay_helper<T>::type;
};

template<typename T>
struct decayfield {
  static_assert(!std::is_reference<T>::value, "memberof is prohibited on reference fields");
  using type = global_ptr<typename std::remove_reference<T>::type>;
};
template<typename T>
struct decayfield<T[]> {
  using type = global_ptr<typename decay_helper<T>::type>;
};
template<typename T, std::size_t N>
struct decayfield<T[N]> {
  using type = global_ptr<typename decay_helper<T>::type>;
};

//------------------------------------
struct foo {
  int v;
  int arr[3];
  int MDarr[3][4][5];
  int &ref;
  int (&yuk)[3];
  int (&superyuk)[3][4][5];
  foo() : ref(v), yuk(arr), superyuk(MDarr) {}
};

int main() {
  foo f;

  #define _NAME2(a,b) a ## b
  #define _NAME1(a,b) _NAME2(a,b)
  #define NAME _NAME1(anon_name,__LINE__)

  // decayfield<fieldtype>                             produces output:    
  assert_same<decayfield<int>::type,                   global_ptr<int>> NAME;
  assert_same<decayfield<const int>::type,             global_ptr<const int>> NAME;
  assert_same<decayfield<int[3]>::type,                global_ptr<int>> NAME;
  assert_same<decayfield<int *>::type,                 global_ptr<int *>> NAME;
  assert_same<decayfield<int * const>::type,           global_ptr<int * const>> NAME;
  assert_same<decayfield<int[]>::type,                 global_ptr<int>> NAME;
  assert_same<decayfield<int[3][4]>::type,             global_ptr<int>> NAME;
  assert_same<decayfield<int[3][4][5][6]>::type,       global_ptr<int>> NAME;
  assert_same<decayfield<const int[3][4]>::type,       global_ptr<const int>> NAME;
  assert_same<decayfield<const int[3][4][5][6]>::type, global_ptr<const int>> NAME;

  assert_same<decayfield<decltype(f.v)>::type,         global_ptr<int>> NAME;
  assert_same<decayfield<decltype(f.arr)>::type,       global_ptr<int>> NAME;
  assert_same<decayfield<decltype(f.MDarr)>::type,     global_ptr<int>> NAME;

  #if 0
  // error cases: these all generate static_assert
  assert_same<decayfield<decltype(f.ref)>::type, global_ptr<int>> NAME;
  assert_same<decayfield<decltype(f.yuk)>::type, global_ptr<int>> NAME; 
  assert_same<decayfield<decltype(f.superyuk)>::type, global_ptr<int>> NAME; 
  #endif
}

This would need some minor adjustment depending on exactly how we plug it into the macros, but I think it demonstrates the basic idea is tractable.
I've confirmed this proof-of-concept works as expected on floor and current versions of gcc, clang, intel, PGI.

global_ptr doesn't support operator[], so IMO global_ptr<T[n]> is not something anyone should ever want. If someone really wants to move an array by-value, they can still use global_ptr<std::array<T,n>> to make that happen (which I think already works).

[Amir Kamil]

This looks reasonable to me. But I think it would still prohibit array access as a member designator, which doesn’t sit right with me.

[Dan Bonachea]

We discussed this in the 2020-07-15 meeting and the consensus was we should deploy the array member->global_ptr decay approach advocated above, and augment the "no references" assertion message with an explanatory comment to the effect of:

if your call is of the form upcxx_memberof_general(obj, array_field[idx]), then you should instead do: upcxx_memberof_general(obj, array_field)+idx

This issue is currently assigned to Amir, but I'm also capable/willing if Amir doesn't have the time to work on this.

[Amir Kamil]

Implement decaying of array fields in upcxx_memberof[_general]. Fixes #386.

→ <<cset 39412e7b56de>>

[Amir Kamil]

Merged in akamil/upcxx/issue386 (pull request #233)

Implement decaying of array fields in upcxx_memberof[_general]. Fixes #386.

Approved-by: Dan Bonachea [e-mail]

→ <<cset c94d81ceb16f>>