Fix is_trivially_copyable check for MSVC and older stdlibc++

MSVC 2015 and 2017 implement std::is_trivially_copyable, but not
correctly. Hence, reimplement it using more low-level primitives.

For stdlibc++ versions below 5.0, we already have a workaround for the
missing support of std::is_trivially_copyable, but this is an unsound
approximation, because it is ignoring move constructor, move assignment
and copy assignment. Therefore, do not use this approximation for
asserting trivial copyability of a type.

Finally, add unittests for the new is_trivially_copyable
implementations.

R=mstarzinger@chromium.org
CC=loorongjie@gmail.com

Change-Id: I9ee56a65882e8c94b72c9a2d484edd27963a5d89
Reviewed-on: https://chromium-review.googlesource.com/941521Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51651}

src/allocation.h

@@ -43,8 +43,8 @@ T* NewArray(size_t size) {
   return result;
 }
 
-template <typename T,
-          typename = typename std::enable_if<IS_TRIVIALLY_COPYABLE(T)>::type>
+template <typename T, typename = typename std::enable_if<
+                          base::is_trivially_copyable<T>::value>::type>
 T* NewArray(size_t size, T default_val) {
   T* result = reinterpret_cast<T*>(NewArray<uint8_t>(sizeof(T) * size));
   for (size_t i = 0; i < size; ++i) result[i] = default_val;

src/arm/assembler-arm.h

@@ -162,8 +162,8 @@ class Register : public RegisterBase<Register, kRegAfterLast> {
   explicit constexpr Register(int code) : RegisterBase(code) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(Register) &&
-                  sizeof(Register) == sizeof(int),
+ASSERT_TRIVIALLY_COPYABLE(Register);
+static_assert(sizeof(Register) == sizeof(int),
               "Register can efficiently be passed by value");
 
 // r7: context register
@@ -218,8 +218,8 @@ class SwVfpRegister : public RegisterBase<SwVfpRegister, kSwVfpAfterLast> {
   explicit constexpr SwVfpRegister(int code) : RegisterBase(code) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(SwVfpRegister) &&
-                  sizeof(SwVfpRegister) == sizeof(int),
+ASSERT_TRIVIALLY_COPYABLE(SwVfpRegister);
+static_assert(sizeof(SwVfpRegister) == sizeof(int),
               "SwVfpRegister can efficiently be passed by value");
 
 typedef SwVfpRegister FloatRegister;
@@ -256,8 +256,8 @@ class DwVfpRegister : public RegisterBase<DwVfpRegister, kDoubleAfterLast> {
   explicit constexpr DwVfpRegister(int code) : RegisterBase(code) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(DwVfpRegister) &&
-                  sizeof(DwVfpRegister) == sizeof(int),
+ASSERT_TRIVIALLY_COPYABLE(DwVfpRegister);
+static_assert(sizeof(DwVfpRegister) == sizeof(int),
               "DwVfpRegister can efficiently be passed by value");
 
 typedef DwVfpRegister DoubleRegister;

src/arm64/assembler-arm64.h

@@ -237,8 +237,7 @@ class CPURegister : public RegisterBase<CPURegister, kRegAfterLast> {
   }
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(CPURegister),
-              "CPURegister can efficiently be passed by value");
+ASSERT_TRIVIALLY_COPYABLE(CPURegister);
 
 class Register : public CPURegister {
  public:
@@ -292,8 +291,7 @@ class Register : public CPURegister {
   constexpr explicit Register(const CPURegister& r) : CPURegister(r) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(Register),
-              "Register can efficiently be passed by value");
+ASSERT_TRIVIALLY_COPYABLE(Register);
 
 constexpr bool kPadArguments = true;
 constexpr bool kSimpleFPAliasing = true;
@@ -430,8 +428,7 @@ class VRegister : public CPURegister {
   }
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(VRegister),
-              "VRegister can efficiently be passed by value");
+ASSERT_TRIVIALLY_COPYABLE(VRegister);
 
 // No*Reg is used to indicate an unused argument, or an error case. Note that
 // these all compare equal (using the Is() method). The Register and VRegister

src/base/macros.h

@@ -196,12 +196,67 @@ V8_INLINE Dest bit_cast(Source const& source) {
 // TODO(all) Replace all uses of this macro with static_assert, remove macro.
 #define STATIC_ASSERT(test) static_assert(test, #test)
 
-// TODO(rongjie) Remove this workaround once we require gcc >= 5.0
-#if __GNUG__ && __GNUC__ < 5
-#define IS_TRIVIALLY_COPYABLE(T) \
-  (__has_trivial_copy(T) && __has_trivial_destructor(T))
+namespace v8 {
+namespace base {
+
+template <typename T>
+struct is_trivially_copyable {
+#if V8_CC_MSVC
+  // Unfortunately, MSVC 2015 is broken in that std::is_trivially_copyable can
+  // be false even though it should be true according to the standard.
+  // (status at 2018-02-26, observed on the msvc waterfall bot).
+  // Interestingly, the lower-level primitives used below are working as
+  // intended, so we reimplement this according to the standard.
+  // See also https://developercommunity.visualstudio.com/content/problem/
+  //          170883/msvc-type-traits-stdis-trivial-is-bugged.html.
+  static constexpr bool value =
+      // Copy constructor is trivial or deleted.
+      (std::is_trivially_copy_constructible<T>::value ||
+       !std::is_copy_constructible<T>::value) &&
+      // Copy assignment operator is trivial or deleted.
+      (std::is_trivially_copy_assignable<T>::value ||
+       !std::is_copy_assignable<T>::value) &&
+      // Move constructor is trivial or deleted.
+      (std::is_trivially_move_constructible<T>::value ||
+       !std::is_move_constructible<T>::value) &&
+      // Move assignment operator is trivial or deleted.
+      (std::is_trivially_move_assignable<T>::value ||
+       !std::is_move_assignable<T>::value) &&
+      // One of the above is non-deleted.
+      (std::is_trivially_copy_constructible<T>::value ||
+       std::is_trivially_copy_assignable<T>::value ||
+       std::is_trivially_move_constructible<T>::value ||
+       std::is_trivially_move_assignable<T>::value) &&
+      // Trivial non-deleted destructor.
+      std::is_trivially_destructible<T>::value;
+
+#elif defined(__GNUC__) && __GNUC__ < 5
+  // WARNING:
+  // On older libstdc++ versions, there is no way to correctly implement
+  // is_trivially_copyable. The workaround below is an approximation (neither
+  // over- nor underapproximation). E.g. it wrongly returns true if the move
+  // constructor is non-trivial, and it wrongly returns false if the copy
+  // constructor is deleted, but copy assignment is trivial.
+  // TODO(rongjie) Remove this workaround once we require gcc >= 5.0
+  static constexpr bool value =
+      __has_trivial_copy(T) && __has_trivial_destructor(T);
+
 #else
-#define IS_TRIVIALLY_COPYABLE(T) std::is_trivially_copyable<T>::value
+  static constexpr bool value = std::is_trivially_copyable<T>::value;
+#endif
+};
+#if defined(__GNUC__) && __GNUC__ < 5
+// On older libstdc++ versions, base::is_trivially_copyable<T>::value is only an
+// approximation (see above), so make ASSERT_{NOT_,}TRIVIALLY_COPYABLE a noop.
+#define ASSERT_TRIVIALLY_COPYABLE(T) static_assert(true, "check disabled")
+#define ASSERT_NOT_TRIVIALLY_COPYABLE(T) static_assert(true, "check disabled")
+#else
+#define ASSERT_TRIVIALLY_COPYABLE(T)                         \
+  static_assert(::v8::base::is_trivially_copyable<T>::value, \
+                #T "should be trivially copyable")
+#define ASSERT_NOT_TRIVIALLY_COPYABLE(T)                      \
+  static_assert(!::v8::base::is_trivially_copyable<T>::value, \
+                #T "should not be trivially copyable")
 #endif
 
 // The USE(x, ...) template is used to silence C++ compiler warnings
@@ -213,10 +268,13 @@ struct Use {
 };
 #define USE(...)                                                   \
   do {                                                             \
-    ::Use unused_tmp_array_for_use_macro[]{__VA_ARGS__}; \
+    ::v8::base::Use unused_tmp_array_for_use_macro[]{__VA_ARGS__}; \
     (void)unused_tmp_array_for_use_macro;                          \
   } while (false)
 
+}  // namespace base
+}  // namespace v8
+
 // Define our own macros for writing 64-bit constants.  This is less fragile
 // than defining __STDC_CONSTANT_MACROS before including <stdint.h>, and it
 // works on compilers that don't have it (like MSVC).

src/boxed-float.h

@@ -51,8 +51,7 @@ class Float32 {
       : bit_pattern_(bit_pattern) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(Float32),
-              "Float32 should be trivially copyable");
+ASSERT_TRIVIALLY_COPYABLE(Float32);
 
 // Safety wrapper for a 64-bit floating-point value to make sure we don't lose
 // the exact bit pattern during deoptimization when passing this value.
@@ -91,8 +90,7 @@ class Float64 {
       : bit_pattern_(bit_pattern) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(Float64),
-              "Float64 should be trivially copyable");
+ASSERT_TRIVIALLY_COPYABLE(Float64);
 
 }  // namespace internal
 }  // namespace v8

src/handles.cc

@@ -16,12 +16,9 @@ namespace internal {
 // Handles should be trivially copyable so that they can be efficiently passed
 // by value. If they are not trivially copyable, they cannot be passed in
 // registers.
-static_assert(IS_TRIVIALLY_COPYABLE(HandleBase),
-              "HandleBase should be trivially copyable");
-static_assert(IS_TRIVIALLY_COPYABLE(Handle<Object>),
-              "Handle<Object> should be trivially copyable");
-static_assert(IS_TRIVIALLY_COPYABLE(MaybeHandle<Object>),
-              "MaybeHandle<Object> should be trivially copyable");
+ASSERT_TRIVIALLY_COPYABLE(HandleBase);
+ASSERT_TRIVIALLY_COPYABLE(Handle<Object>);
+ASSERT_TRIVIALLY_COPYABLE(MaybeHandle<Object>);
 
 #ifdef DEBUG
 bool HandleBase::IsDereferenceAllowed(DereferenceCheckMode mode) const {

src/ia32/assembler-ia32.h

@@ -103,8 +103,8 @@ class Register : public RegisterBase<Register, kRegAfterLast> {
   explicit constexpr Register(int code) : RegisterBase(code) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(Register) &&
-                  sizeof(Register) == sizeof(int),
+ASSERT_TRIVIALLY_COPYABLE(Register);
+static_assert(sizeof(Register) == sizeof(int),
               "Register can efficiently be passed by value");
 
 #define DEFINE_REGISTER(R) \
@@ -422,10 +422,9 @@ class Operand {
   // TODO(clemensh): Get rid of this friendship, or make Operand immutable.
   friend class Assembler;
 };
+ASSERT_TRIVIALLY_COPYABLE(Operand);
 static_assert(sizeof(Operand) <= 2 * kPointerSize,
               "Operand must be small enough to pass it by value");
-static_assert(IS_TRIVIALLY_COPYABLE(Operand),
-              "Operand must be trivially copyable to pass it by value");
 
 // -----------------------------------------------------------------------------
 // A Displacement describes the 32bit immediate field of an instruction which

src/ppc/assembler-ppc.h

@@ -287,8 +287,8 @@ class Register : public RegisterBase<Register, kRegAfterLast> {
   explicit constexpr Register(int code) : RegisterBase(code) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(Register) &&
-                  sizeof(Register) == sizeof(int),
+ASSERT_TRIVIALLY_COPYABLE(Register);
+static_assert(sizeof(Register) == sizeof(int),
               "Register can efficiently be passed by value");
 
 #define DEFINE_REGISTER(R) \
@@ -329,8 +329,8 @@ class DoubleRegister : public RegisterBase<DoubleRegister, kDoubleAfterLast> {
   explicit constexpr DoubleRegister(int code) : RegisterBase(code) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(DoubleRegister) &&
-                  sizeof(DoubleRegister) == sizeof(int),
+ASSERT_TRIVIALLY_COPYABLE(DoubleRegister);
+static_assert(sizeof(DoubleRegister) == sizeof(int),
               "DoubleRegister can efficiently be passed by value");
 
 typedef DoubleRegister FloatRegister;

src/s390/assembler-s390.h

@@ -261,8 +261,8 @@ class Register : public RegisterBase<Register, kRegAfterLast> {
   explicit constexpr Register(int code) : RegisterBase(code) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(Register) &&
-                  sizeof(Register) == sizeof(int),
+ASSERT_TRIVIALLY_COPYABLE(Register);
+static_assert(sizeof(Register) == sizeof(int),
               "Register can efficiently be passed by value");
 
 #define DEFINE_REGISTER(R) \
@@ -303,8 +303,8 @@ class DoubleRegister : public RegisterBase<DoubleRegister, kDoubleAfterLast> {
   explicit constexpr DoubleRegister(int code) : RegisterBase(code) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(DoubleRegister) &&
-                  sizeof(DoubleRegister) == sizeof(int),
+ASSERT_TRIVIALLY_COPYABLE(DoubleRegister);
+static_assert(sizeof(DoubleRegister) == sizeof(int),
               "DoubleRegister can efficiently be passed by value");
 
 typedef DoubleRegister FloatRegister;

src/wasm/baseline/liftoff-assembler.h

@@ -104,8 +104,7 @@ class LiftoffAssembler : public TurboAssembler {
     };
   };
 
-  static_assert(IS_TRIVIALLY_COPYABLE(VarState),
-                "VarState should be trivially copyable");
+  ASSERT_TRIVIALLY_COPYABLE(VarState);
 
   struct CacheState {
     // Allow default construction, move construction, and move assignment.

src/wasm/baseline/liftoff-register.h

@@ -171,8 +171,7 @@ class LiftoffRegister {
 
   explicit constexpr LiftoffRegister(storage_t code) : code_(code) {}
 };
-static_assert(IS_TRIVIALLY_COPYABLE(LiftoffRegister),
-              "LiftoffRegister can efficiently be passed by value");
+ASSERT_TRIVIALLY_COPYABLE(LiftoffRegister);
 
 inline std::ostream& operator<<(std::ostream& os, LiftoffRegister reg) {
   if (reg.is_pair()) {
@@ -298,8 +297,7 @@ class LiftoffRegList {
   // Unchecked constructor. Only use for valid bits.
   explicit constexpr LiftoffRegList(storage_t bits) : regs_(bits) {}
 };
-static_assert(IS_TRIVIALLY_COPYABLE(LiftoffRegList),
-              "LiftoffRegList can be passed by value");
+ASSERT_TRIVIALLY_COPYABLE(LiftoffRegList);
 
 static constexpr LiftoffRegList kGpCacheRegList =
     LiftoffRegList::FromBits<LiftoffRegList::kGpMask>();

src/wasm/function-body-decoder-impl.h

@@ -1139,8 +1139,7 @@ class WasmFullDecoder : public WasmDecoder<validate> {
 
   // All Value types should be trivially copyable for performance. We push, pop,
   // and store them in local variables.
-  static_assert(IS_TRIVIALLY_COPYABLE(Value),
-                "all Value<...> types should be trivially copyable");
+  ASSERT_TRIVIALLY_COPYABLE(Value);
 
  public:
   template <typename... InterfaceArgs>

src/x64/assembler-x64.h

@@ -103,8 +103,8 @@ class Register : public RegisterBase<Register, kRegAfterLast> {
   explicit constexpr Register(int code) : RegisterBase(code) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(Register) &&
-                  sizeof(Register) == sizeof(int),
+ASSERT_TRIVIALLY_COPYABLE(Register);
+static_assert(sizeof(Register) == sizeof(int),
               "Register can efficiently be passed by value");
 
 #define DECLARE_REGISTER(R) \
@@ -204,8 +204,8 @@ class XMMRegister : public RegisterBase<XMMRegister, kDoubleAfterLast> {
   explicit constexpr XMMRegister(int code) : RegisterBase(code) {}
 };
 
-static_assert(IS_TRIVIALLY_COPYABLE(XMMRegister) &&
-                  sizeof(XMMRegister) == sizeof(int),
+ASSERT_TRIVIALLY_COPYABLE(XMMRegister);
+static_assert(sizeof(XMMRegister) == sizeof(int),
               "XMMRegister can efficiently be passed by value");
 
 typedef XMMRegister FloatRegister;
@@ -380,19 +380,9 @@ class Operand {
  private:
   const Data data_;
 };
+ASSERT_TRIVIALLY_COPYABLE(Operand);
 static_assert(sizeof(Operand) <= 2 * kPointerSize,
               "Operand must be small enough to pass it by value");
-// Unfortunately, MSVC 2015 is broken in that both is_trivially_destructible and
-// is_trivially_copy_constructible are true, but is_trivially_copyable is false.
-// (status at 2018-02-26, observed on the msvc waterfall bot).
-#if V8_CC_MSVC
-static_assert(std::is_trivially_copy_constructible<Operand>::value &&
-                  std::is_trivially_destructible<Operand>::value,
-              "Operand must be trivially copyable to pass it by value");
-#else
-static_assert(IS_TRIVIALLY_COPYABLE(Operand),
-              "Operand must be trivially copyable to pass it by value");
-#endif
 
 #define ASSEMBLER_INSTRUCTION_LIST(V) \
   V(add)                              \

test/unittests/base/macros-unittest.cc

@@ -21,5 +21,43 @@ TEST(AlignedAddressTest, AlignedAddress) {
             AlignedAddress(reinterpret_cast<void*>(0xFFFFF), 0x100000));
 }
 
+struct TriviallyCopyable {
+  const int i;
+};
+ASSERT_TRIVIALLY_COPYABLE(TriviallyCopyable);
+
+struct NonTrivialDestructor {
+  ~NonTrivialDestructor() {}
+};
+ASSERT_NOT_TRIVIALLY_COPYABLE(NonTrivialDestructor);
+
+struct NonCopyable {
+  NonCopyable(const NonCopyable&&) = delete;
+  NonCopyable(const NonCopyable&) = delete;
+  NonCopyable& operator=(const NonCopyable&) = delete;
+  NonCopyable& operator=(const NonCopyable&&) = delete;
+};
+ASSERT_NOT_TRIVIALLY_COPYABLE(NonCopyable);
+
+struct NonTrivialCopyConstructor {
+  NonTrivialCopyConstructor(const NonTrivialCopyConstructor&) {}
+};
+ASSERT_NOT_TRIVIALLY_COPYABLE(NonTrivialCopyConstructor);
+
+struct NonTrivialMoveConstructor {
+  NonTrivialMoveConstructor(const NonTrivialMoveConstructor&) {}
+};
+ASSERT_NOT_TRIVIALLY_COPYABLE(NonTrivialMoveConstructor);
+
+struct NonTrivialCopyAssignment {
+  NonTrivialCopyAssignment(const NonTrivialCopyAssignment&) {}
+};
+ASSERT_NOT_TRIVIALLY_COPYABLE(NonTrivialCopyAssignment);
+
+struct NonTrivialMoveAssignment {
+  NonTrivialMoveAssignment(const NonTrivialMoveAssignment&) {}
+};
+ASSERT_NOT_TRIVIALLY_COPYABLE(NonTrivialMoveAssignment);
+
 }  // namespace base
 }  // namespace v8