Project

General

Profile

Bug #1701

Cannot have non-copyable types in constructor arguments

Added by Nils Deppe 3 months ago. Updated about 16 hours ago.

Status:
Merged
Priority:
Normal
Assignee:
Category:
-
Target version:
Start date:
09/30/2017
Due date:
% Done:

0%


Description

If I try to pass a non-copyable type to a chare constructor my only option is to move an lvalue reference, which is not good. The solution is again supporting C++11, though there may be a C++03 method of implementing a workaround.


Related issues

Related to Charm++ - Bug #1679: Do Not Require Default Constructors for Serializable Classes Merged 09/18/2017

History

#1 Updated by Phil Miller 2 months ago

I take it you're trying to pass an rvalue reference to such an object? It doesn't really make semantic sense for any entry method argument (passed to a constructor or otherwise) to be truly non-copyable, in that they're going to get serialized and desrialized, creating a copy in some form.

I think the code is happy to take a const &, though. I'll check that shortly.

#2 Updated by Nils Deppe 2 months ago

Looking now, this ticket could be made much more helpful on my part. The serialization is not the problem, it is the move into the impl_obj_void. Below is a modified def.h file where I have added the missing calls to std::move. The deserialized impl_noname_0 is normally passed as an lvalue reference, which is fine for copyable types, but not for non-copyable. Having the constructor take a non-const lvalue reference and moving that is a workaround, but this breaks using the class outside of a Charm++ context. So far this has only bitten us in unit tests, but as C++11 support is required this should be a relatively straightforward change to Charm++.

template <typename Metavariables>

void CkIndex_ConstGlobalCache<Metavariables>::_call_ConstGlobalCache_marshall1(
    void* impl_msg, void* impl_obj_void) {
  ConstGlobalCache<Metavariables>* impl_obj =
      static_cast<ConstGlobalCache<Metavariables>*>(impl_obj_void);
  CkMarshallMsg* impl_msg_typed = (CkMarshallMsg*)impl_msg;
  char* impl_buf = impl_msg_typed->msgBuf;
  /*Unmarshall pup'd fields: const tuples::TaggedTupleTypelist<typename
   * Metavariables::const_global_cache_tag_list > &impl_noname_0*/
  PUP::fromMem implP(impl_buf);
  tuples::TaggedTupleTypelist<
      typename Metavariables::const_global_cache_tag_list>
      impl_noname_0;
  implP | impl_noname_0;
  impl_buf += CK_ALIGN(implP.size(), 16);
  /*Unmarshall arrays:*/
  new (impl_obj_void) ConstGlobalCache<Metavariables>(std::move(impl_noname_0));
}
template <typename Metavariables>

int CkIndex_ConstGlobalCache<Metavariables>::
    _callmarshall_ConstGlobalCache_marshall1(char* impl_buf,
                                             void* impl_obj_void) {
  ConstGlobalCache<Metavariables>* impl_obj =
      static_cast<ConstGlobalCache<Metavariables>*>(impl_obj_void);
  /*Unmarshall pup'd fields: const tuples::TaggedTupleTypelist<typename
   * Metavariables::const_global_cache_tag_list > &impl_noname_0*/
  PUP::fromMem implP(impl_buf);
  tuples::TaggedTupleTypelist<
      typename Metavariables::const_global_cache_tag_list>
      impl_noname_0;
  implP | impl_noname_0;
  impl_buf += CK_ALIGN(implP.size(), 16);
  /*Unmarshall arrays:*/
  new (impl_obj_void) ConstGlobalCache<Metavariables>(std::move(impl_noname_0));
  return implP.size();
}

#3 Updated by Phil Miller 2 months ago

Indeed, CProxy_Foo's generated code takes its arguments by const& even when declared in the .ci file as taking them by value. So, a temporary object passed as an argument will do the right thing, and an actual lvalue can be moved or not, and still be consumed as const lvalue reference. As far as I can tell, this means there isn't an issue of code semantics that should work but doesn't, but there might be syntactic representations that have to be avoided. Do I have that right, or is there a case I haven't considered properly? Do you have a concrete example I could look at?

#4 Updated by Phil Miller 2 months ago

OK, our notes crossed paths as we were writing them. I'll look further in a bit.

#5 Updated by Phil Miller 2 months ago

OK, so I think what you're saying is that the generated code needs to move the temporary instances, so that the recipient can potentially take them by rvalue reference and consume them, rather than only being able to copy or hackishly moving from something that looks like an lvalue reference argument. Is that right? The difference between the current generated code and your example is the addition of std::move?

#6 Updated by Nils Deppe 2 months ago

Yep, that's exactly correct. The only difference between my modified code and the generated is the addition of std::move. Specifically,

new (impl_obj_void) ConstGlobalCache<Metavariables>(simpl_noname_0);
// was changed to
new (impl_obj_void) ConstGlobalCache<Metavariables>(std::move(impl_noname_0));

and the same for the second function.

#7 Updated by Eric Bohm 2 months ago

  • Assignee set to Eric Mikida

#8 Updated by Phil Miller 11 days ago

I think this is addressed by https://charm.cs.illinois.edu/gerrit/#/c/3263/ ?

#9 Updated by Phil Miller 5 days ago

  • Status changed from New to Implemented
  • Assignee changed from Eric Mikida to Phil Miller
  • Target version set to 6.9.0

Based on the linked patch. If that doesn't resolve this issue, we can re-adjust the status, target, and assignment.

#10 Updated by Phil Miller about 16 hours ago

  • Related to Bug #1679: Do Not Require Default Constructors for Serializable Classes added

#11 Updated by Phil Miller about 16 hours ago

  • Status changed from Implemented to Merged

As illustrated https://charm.cs.illinois.edu/gerrit/#/c/3263/7/src/ck-core/ckarray.h we now have chare classes that take rvalue references to their non-copyable constructor arguments.

Also available in: Atom PDF