Disallow UPCXX_SERIALIZED_FIELDS on bitfield members

[Dan Bonachea]

Example code:

#include <upcxx/upcxx.hpp>
#include <iostream>
#include <string>

struct T {
  std::string s;
  int64_t x   :3;
  int64_t y   :3;
  int64_t z   :2;

  UPCXX_SERIALIZED_FIELDS(s,x,y,z)
};

int main() {
  upcxx::init();

  upcxx::rpc(0,[](T t) { }, T()).wait();

  upcxx::finalize();
  return 0;
}

This code currently fails to compile, because our implementation of UPCXX_SERIALIZED_FIELDS makes a fundamental assumption that it can create (non-const) references to the named fields, however C++ [class.bit] prohibits non-const references to bitfields (and also pointers to bitfield members).

I suspect that it's impossible to portably implement UPCXX_SERIALIZED_FIELDS to handle bit field members (at least without additional introspection support we don't have in C++11). While I cannot prove that suspicion, our current solution approach definitely won't work for bitfields.

I suggest we update the spec to prohibit bitfield member arguments to UPCXX_SERIALIZED_FIELDS. Classes such as the one above will need to use one of the other serialization mechanisms.

CC: @Rob Egan

[Rob Egan]

I think that is perfectly okay. imo, bitfields only need to be used to compact data members into a standard type and so should composed into structs that are trivial to serialize only.

struct T_bitfields {
  int64_t x   :3;
  int64_t y   :3;
  int64_t z   :2;
};
struct T {
  std::string s;
  T_bitfields bfs;
};

A related question for the spec might also mention restrictions on unions and how they are treated.

union T_bitfield_union {
  T_bitfields bfs;
  void *ptr;
};

‌

[Paul Hargrove]

I am pretty sure (but short of 100%) based on my knowledge C, that something like the following has sufficiently well-defined semantics be used:

struct T {
  std::string s;
  union {
    struct {
      int64_t x   :3;
      int64_t y   :3;
      int64_t z   :2;
    } fields;
    int64_t xyz;
  } u;
  UPCXX_SERIALIZED_FIELDS(s, u.xyz)
};

If I am correct that this is well-defined, is it worth documenting this somewhere (other than here in the issue) as a viable work around? Of course this does significant "damage" to existing code using the x, y, z fields, but might be acceptable for classes being developed from scratch for UPC++.

[Dan Bonachea]

@Rob Egan said:

A related question for the spec might also mention restrictions on unions and how they are treated.

AFAIK unions should "work" with UPCXX_SERIALIZED_FIELDS, but they are probably not what you usually want. In particular, passing more than one union member to UPCXX_SERIALIZED_FIELDS will result in the overlapping reads for the data being serialized and (more importantly) overlapping writes of different types during deserialization, which could easily result in UB and data corruption. Unions are not "easy mode" serialization and should probably use one of the other more capable serialization mechanisms.

@Paul Hargrove said:

is it worth documenting this somewhere (other than here in the issue) as a viable work around?

IMO bitfield members should be specified to not qualify for "easy mode" serialization. This is just one more of four existing restrictions on "easy mode" serialization. IMO the suggested workaround should be the obvious one - use UPCXX_SERIALIZED_VALUES or Custom Serialization instead. It's a few more lines of typing, but is transparent to clients of the class.

[Amir Kamil]

Prohibit bit fields in UPCXX_SERIALIZED_FIELDS. Fixes #175.

→ <<cset 93047e9850e6>>