MoveAssignment operators and self-assignment

As of release 2022.3.0 our major resource types now have move assignment operators, like these:

      atomic_domain &operator=(atomic_domain &&that) {
        UPCXXI_ASSERT_MASTER();
        // only allow assignment moves onto "dead" object
        UPCXX_ASSERT(!this->is_active(),
                     "Move assignment is only allowed on an inactive atomic_domain");
        this->ad_gex_handle = that.ad_gex_handle;
        this->atomic_gex_ops = that.atomic_gex_ops;
        this->parent_tm_ = that.parent_tm_;
        // revert `that` to non-constructed state
        that.atomic_gex_ops = 0;
        that.ad_gex_handle = 0;
        that.parent_tm_ = nullptr;
        return *this;
      }

team& team::operator=(team &&that) {
  UPCXXI_ASSERT_INIT();
  UPCXXI_ASSERT_MASTER();
  UPCXX_ASSERT(&that != &world(),
               "team world() cannot be passed to move constructor or assignment");
  UPCXX_ASSERT(&that != &local_team(),
               "team local_team() cannot be passed to move constructor or assignment");
  UPCXX_ASSERT(
    !this->is_active(),
    "team move assignment operator requires receiver to be inactive"
  );

  backend::team_base::operator=(std::move(that));
  id_ = that.id_;
  coll_counter_ = that.coll_counter_;
  n_ = that.n_;
  me_ = that.me_;

  that.invalidate(detail::internal_only{});

  if (id_ != tombstone) {
    detail::registry[id_] = this;
  }
  return *this;
}

Both of the move assignment operators above will crash in debug mode if an active object is involved in self-move-assignment (due to the inactivity precondition), e.g.:

  upcxx::team tm0 = upcxx::world().split(0, 0);
  UPCXX_ASSERT_ALWAYS(tm0.is_active());
  tm0 = std::move(tm0);  // crashes in debug mode
  UPCXX_ASSERT_ALWAYS(tm0.is_active()); // crashes in opt mode
  tm0.destroy();

and in opt mode will silently corrupt data state (leading to a failure of the second assertion and otherwise memory leaks due to the skipped destroy()). Similar observations apply to memory kinds types.

While this is admittedly a strange thing for a user to do, my understanding is that our behavior is not kosher by C++ norms. Here's a good blog article on the topic.

I'm thinking we should probably add a dynamic check for self-assignment in these move assignment operators that makes them a no-op after the first few assertions (details TBD). For these heavyweight objects moves should be relatively rare, so the added cost of one branch should be justified to provide less surprising behavior. I'm less sure about types like future, but based on my quick audit its move assignment operator already happens to work correctly for self-assign.

Thoughts?

Comments (3)