Use C++ protection features to enforce abstraction boundaries

[Dan Bonachea]

UPC++ exposes a number of public classes with deliberately unspecified members used for the implementation that should never be touched by users.

A prominent example is the global_ptr<T> class template, which currently includes:

  public: //private!
    #if UPCXX_MANY_KINDS
      std::int32_t device_;
    #else
      static constexpr std::int32_t device_ = -1;
    #endif
    intrank_t rank_;
    T* raw_ptr_;

As public fields these look very inviting to end users who don't fully read/comprehend our documentation and just access the fields they see (especially in rich IDEs which will show these as public fields, without the comment). This has already happened with several external users and leads to problems when we change the internal fields or they use them incorrectly (because they are undocumented).

C++ has features to prevent exactly this problem. All such fields should really be private or at least protected, or (as a last resort) appear in a detail namespace which we very loudly document as "off-limits".

[Dan Bonachea]

This was discussed in the 2019-12-04 Pagoda meeting.

With regards to the global_ptr fields in particular, John explained that these fields are currently declared public because it simplifies usage internal to the runtime. The consensus was the current "trailing underscore" naming convention is insufficient to universally convey to our end users that these fields are private and unsupported (as evidenced by multiple independent support threads on the forum). It was noted that the behavior of the raw_ptr_ field values is especially surprising to users who don't understand our deliberately undocumented shared memory mappings.

@john bachan promised to investigate ways to make these fields less "attractive" to end users with smart editors (who usually won't see any comments in the header), and more obviously "danger, these are unsupported and probably don't behave the way you expect".

Possibilities include:

1. Renaming the fields with a more obviously "dangerous" naming convention eg global_ptr::danger_private_raw_ptr
2. Making the fields private and introducing inline accessor methods in the detail namespace for use by the runtime

[Dan Bonachea]

Bulk roll-over of unresolved issues to next milestone

[Dan Bonachea]

Mass roll-over of open issues to next release milestone

[Amir Kamil]

Now that PR #326 has been merged, here are the remaining public members I’ve been able to find:

global_ptr<const T, KindSet>::check()
persona::backend_state_
persona::cuda_state_
persona::undischarged_n_
persona::active()
persona_scope::the_default_dummy_
persona_scope::next_
persona_scope::persona_xor_default_
persona_scope::next_unique_
persona_scope::tls
serialization<T>::existence
serialization<T>::is_serializable
serialization_traits<T>::static_ubound
serialization_traits<T>::static_ubound_t

future1::kind_type
future1::results_type
future1::impl_type
future1::clref_results_refs_or_vals_type
future1::rref_results_refs_or_vals_type
future1::result_return_select_type
future1::future1(impl_type)
future1::then_lazy()

Of these, I think global_ptr::check() and future1::then_lazy() would look the most enticing to users. I’ll look into protecting them (likely with UPCXX_INTERNAL_ONLY for global_ptr::check() and friend declarations for future1::then_lazy()). I’ll also see how much work it would be to protect the persona and persona_scope members.

[Dan Bonachea]

global_ptr::check() is harmless and provides a sanity check that I actually deliberately added that in the public namespace. I view this as an implementation feature that we could optionally document in docs/implementation-defined.md, although possibly not something for the formal spec.

The persona and persona_scope internals are alot more dangerous and ideally would be protected somehow (but not if it comes at the cost of additional calls).

[Amir Kamil]

OK, I’ll leave global_ptr::check() alone.

I will likely look at future1::then_lazy() as part of PR #328, since that moves future1 into the detail namespace.

I’ll look at persona and persona_scope in a separate PR.

I don’t think I’ll touch serialization or serialization_traits.

[Dan Bonachea]

PR 326 and PR 328 have now been merged, which resolves the worst offenders using the agreed-upon enforcement mechanisms.