Fix issue 47160: "`llvm::is_trivially_copyable` -- static assertion failure after DR 1734"

[strega-nil]

Summary

Includes an explanation of the difference between llvm::is_trivially_copyable and std::is_trivially_copyable, in the comment above.

Link to bug: https://bugs.llvm.org/show_bug.cgi?id=47160

This doesn't change any behaviour, but it removes an existing (wrong) compile-fail check, and fixes the build under VS2019 19.27.

[CaseyCarter]

I'm not going to "request changes" since I don't want you to feel like I'm asking for a novel on trivial special member functions. I'm just going to "comment" and race out of the review before someone asks me to do work ;)

llvm/include/llvm/Support/type_traits.h

132

The diff is missing context; I think you forgot to pass -U 99999 to git diff.

142

"llvm::is_trivially_copyable<T> for a class type T checks for the following:" would be more precise and avoid confusion about where T comes from.

145

If T x(declval<const T&>()) is well-formed, then std::is_destructible_v<T> && std::is_copy_constructible_v<T> is true. That fact implies neither that T has a copy constructor - that syntax could invoke a constructor template - nor that any copy constructor of T is trivial - that syntax could invoke a non-trivial copy constructor. Ditto the next three bullets, but replacing "copy" with "move" and "constructor/constructible" with "assignment operator/assignment" as appropriate.

The foregoing is why detail::trivial_helper exists. detail::trivial_helper is the dirt-simple union template:

c++
template<class T>
union trivial_helper {
    T t;
};

Which has some convenient properties thanks to the rules for implicitly-defined special member functions for unions. First, "A defaulted destructor for a class X is defined as deleted if ... X is a union-like class that has a variant member with a non-trivial destructor ..." (n4861 [class.dtor]/7). If is_destructible_v<trivial_helper<T>> is true, then it must be the case that T has a trivial destructor, so is_destructible_v<T> && is_destructible_v<trivial_helper<T>> == is_trivially_destructible_v<T>. We have implemented is_trivially_destructible.

Second, "A defaulted copy/move constructor for a class X is defined as deleted if X has ... a potentially constructed subobject type M (or array thereof) that cannot be copied/moved ... [or] a variant member whose corresponding constructor as selected by overload resolution is non-trivial [or] any potentially constructed subobject of a type with a destructor that is deleted or inaccessible [or] for the copy constructor, a non-static data member of rvalue reference type." (n4861 [class.copy.ctor]/10). Tells us that is_copy_constructible_v<trivial_helper<T>> is roughly is_trivially_copy_constructible_v<T> || !is_destructible_v<T>. Since we have access to is_destructible we have an implementation of is_trivially_copy_constructible. Similarly, is_move_constructible_v<trivial_helper<T>> == is_trivially_move_constructible_v<T> || !is_destructible_v<T> which we can use to implement is_trivially_move_constructible.

Okay, we're rocketing along here - what about copy/move assignments? "A defaulted copy/move assignment operator for class X is defined as deleted if X has ... a variant member with a non-trivial corresponding assignment operator and X is a union-like class, or ... a direct non-static data member of class type M (or array thereof) ... that cannot be copied/moved" (n4861 [class.copy.assign]/7). similarly to the foregoing is_copy_assignable_v<trivial_helper<T>> and is_move_assignable_v<trivial_helper<T>> imply is_trivially_copy_assignable_v<T> and is_trivially_move_assignable_v<T>.

There's an additional piece of information we need to be aware of: the only way to have trivial destruction or copy/move construction/assignment is to have a trivial destructor or trivial copy/move constructor/assignment operator. There are non-special-member-function signatures that could intercept the syntax for these operations, but those other function / function templates would necessarily be deleted or non-trivial.

Given the foregoing, it's tempting to define is_trivially_copyable_v<T> as:

c++
template <class T>
constexpr bool is_trivially_copyable_v = is_trivially_destructible_v<T> &&
    (is_trivially_copy_constructible_v<T> || !is_copy_constructible_v<T>) &&
    (is_trivially_move_constructible_v<T> || !is_move_constructible_v<T>) &&
    (is_trivially_copy_assignable_v<T> || !is_copy_assignable_v<T>) &&
    (is_trivially_move_assignable_v<T> || !is_move_assignable_v<T>) &&
    (is_trivially_copy_constructible_v<T> || is_trivially_move_constructible_v<T> ||
     is_trivially_copy_assignable_v<T> || is_trivially_move_assignable_v<T>);

where the is_trivially_meowable traits are implemented as developed above. This would be correct for a great many types, but there's a hidden premise: the preceding assumes that is_meow<T> && !is_trivially_meow<T> implies that T's meow (copy/move constructor/assignment operator) is not deleted. It is, however, possible to have a type with a deleted special member function for which the corresponding syntax invokes a non-special member function. Case in point, MSVC's std::pair looks a lot like:

c++
template <class F, class S>
struct pair {
  F first;
  S second;

  ~pair() = default;
  pair(const pair&) = default;
  pair(pair&&) = default;
  // ...

  pair& operator=(const volatile pair&) = delete;
  template <enable_if_t</*...*/, int> = 0>
  pair& operator=(const pair&) {
    // ...
  }

  template <enable_if_t</*...*/, int> = 0>
  pair& operator=(pair&&) {
    // ...
  }
};

If both F and S are trivially destructible, trivially move constructible, and trivially copy constructible (or not copy constructible) then pair<F, S> is trivially copyable regardless of is_copy_assignable and is_move_assignable.

In any case, the definition this file uses is actually:

c++
template <class T>
constexpr bool is_trivially_copyable_v = is_trivially_destructible_v<T> &&
    (is_trivially_copy_constructible_v<T> || !is_copy_constructible_v<T>) &&
    (is_trivially_move_constructible_v<T> || !is_move_constructible_v<T>) &&
    (is_trivially_copy_assignable_v<T> || !is_copy_assignable_v<T>) &&
    (is_trivially_move_assignable_v<T> || !is_move_assignable_v<T>);

which corresponds to the C++14 (pre-CWG 1734) formulation of trivial copyability and not the C++17 formulation. (Clang implements __is_trivially_copyable similarly, having not yet implemented CWG 1734.) The best fix would be to avoid this fillin trait completely on platforms that provide it in the STL - I think all supported compilers/standard libraries except for libstdc++ pre-5.0? - but a close second is to not expect the trait to agree with the STL.

161

Since C++20 (Specifically P0848R3) "all of T's _eligible_ copy constructors, move constructors, ..." would be more precise. I acknowledge this may be more precision than you are interested in =)

[strega-nil]

llvm/include/llvm/Support/type_traits.h

161

Yeah, that's my thought :)

[strega-nil]

Just for you @CaseyCarter :) (thanks!)

[strega-nil]

Adding the two people who implemented the function.

[danilaml]

Ping?

[strega-nil]

Yeah I dunno what's happening here.

[danilaml]

I've taken a liberty of adding more reviewers to hopefully help get this going.

@CaseyCarter Is there anything left to address in this review? This error shows up in some pre-merge checks and generally keeping local patches to fix this is annoying (although low-maintenance as this header is rarely updated).

[serge-sans-paille]

Some history on that particular problem: back to https://reviews.llvm.org/D54472 when we moved away from llvm::isPodLike<> in favor of llvm::is_trivially_copyable<>, the goal was to move to is_trivially_copyable once the minimal compiler requirements reaches a supported state.
So I'm not in favor of a diverging between the llvm and std implementation, but either

1. Fix the llvm:: implementation to match std::
2. Use std:: implementation if we know have a decent compiler requirement.

Note that a requirement (for both 1. and 2.) is that all supported compilers return the same value for is_trivially_copyable<T> otherwise it leads to ABI issues (different specialization etc).

[strega-nil]

In D86126#2405424, @serge-sans-paille wrote:

1. Fix the llvm:: implementation to match std::

unfortunately, this is impossible. There is fundamentally no way to check for std::is_trivially_copyable without special compiler magic, and unfortunately, due to this DR, it's unlikely you'd ever be able to switch to the std:: implementation, at least until all compilers you support have this DR as a baseline - given that I don't think gcc or clang have even implemented it yet, this seems unlikely. (at least, this is my understanding... no guarantees on correctness here)

[danilaml]

ping?

[JVD]

I also was seeing this problem, when trying to build LLVM with Visual Studio 2019 Community Edition version 16.8.4,
on Windows 10 Pro x86_64 build 19042.746 update 20H2 , fully up-to-date as of 2021/01/15 .

In my case, the problem was fixed by this patch:

$ diff -wB0U3 llvm-10.0.1.src/include/llvm/ADT/SmallVector.h,10.0.0.1.src llvm-10.0.1.src/include/llvm/ADT/SmallVector.h

• llvm-10.0.1.src/include/llvm/ADT/SmallVector.h,10.0.0.1.src 2021-01-15 20:10:44.407349600 +0000

+++ llvm-10.0.1.src/include/llvm/ADT/SmallVector.h 2021-01-15 18:38:01.985069700 +0000
@@ -229,8 +229,8 @@
};

// Define this out-of-line to dissuade the C++ compiler from inlining it.
-template <typename T, bool TriviallyCopyable>
-void SmallVectorTemplateBase<T, TriviallyCopyable>::grow(size_t MinSize) {
+template <typename T, bool TRV = is_trivially_copyable<T>::value>
+void SmallVectorTemplateBase<T,TRV>::grow(size_t MinSize) {

if (MinSize > UINT32_MAX)
  report_bad_alloc_error("SmallVector capacity overflow during allocation");

[JVD]

With the above patch applied, and your patch, ie. to comment out or remove from Support/type_traits.h :

\#ifdef HAVE_STD_IS_TRIVIALLY_COPYABLE

static_assert(value == std::is_trivially_copyable<T>::value,
              "inconsistent behavior between llvm:: and std:: implementation of is_trivially_copyable");

\#endif

Then all 384 LLVM projects, including the ones I want (llc.exe, lli.exe, opt.exe) are built OK for (Release, x64) in Visual Studio 2019 .

[serge-sans-paille]

llvm/include/llvm/Support/type_traits.h

182

I understand why you remove this one, but it also removes the only safe guard we had to prevent the ABI issue solved by https://reviews.llvm.org/D54472. I'm fine with removing it, but only if we add a test case that makes sure that we have a consistent behavior for llvm::is_trivially_copyable for most of the ADT containers.

[danilaml]

llvm/include/llvm/Support/type_traits.h

182

@serge-sans-paille please correct me If I'm wrong, but https://reviews.llvm.org/D54472 is solved by using llvm's own is_trivially_copyable because older (now unsupported) compilers didn't implement the std:: version properly. static_assert wasn't a safeguard against ABI issues, it was there to ensure that LLVM's own implementation behaved the same as std:: one when it's available and that's all. But when does this assert matter for ABI (when llvm:: version is used everywhere)?

Currently it is impossible to build LLVM on any modern standard-conforming compiler and this fix missed not only 11.0 release but 12.0 as well, which is a shame.

Maybe when Clang will fail to build itself there will be enough will to finally fix it. (there was at least one attempt in the meantime: https://reviews.llvm.org/D92543 )

[serge-sans-paille]

@danilaml I've submited https://reviews.llvm.org/D96536 that provides some replacement checks for the previous std::is_trivially_copiable <> llvm::is_trivially_copyable check. Once that one (or a similar one / a revised version) is in, I'm 100% in favor of landing that one.

[serge-sans-paille]

Now that https://reviews.llvm.org/D96536 landed, we can remove the assert with some more confidence.

[strega-nil]

This is no longer necessary, as llvm::is_trivially_copyable no longer exists; closing the bugs.

llvm/include/llvm/Support/type_traits.h

 };
 
 template <typename T>
 struct is_move_assignable {
   template<class F>
     static auto get(F*) -> decltype(std::declval<F &>() = std::declval<F &&>(), std::true_type{});
     static std::false_type get(...);
     static constexpr bool value = decltype(get((T*)nullptr))::value;
 };
 
-
-// An implementation of `std::is_trivially_copyable` since STL version
+// A close implementation of `std::is_trivially_copyable` since STL version
 // is not equally supported by all compilers, especially GCC 4.9.
 // Uniform implementation of this trait is important for ABI compatibility
 // as it has an impact on SmallVector's ABI (among others).
+// This type trait is not exactly in line with `std::is_trivially_copyable`
+// as we cannot test for exactly what `std::is_trivially_copyable` tests for.
+//
+// `llvm::is_trivially_copyable` checks for the following:
+//   * if `T x(declval<const T&>())` is well formed,
+//     then all of T's copy constructors must be trivial,
+//     and there must be at least one non-deleted copy constructor.
+//   * if `declval<T&>() = declval<const T&>()` is well formed,
+//     then all of T's copy assignment operators must be trivial,
+//     and there must be at least one non-deleted copy assignment operator.
+//   * if `T x(declval<T&&>())` is well formed,
+//     then all of T's move constructors must be trivial,
+//     and there must be at least one non-deleted move constructor
+//   * if `declval<T&>() = declval<T&&>()` is well formed,
+//     then all of T's move assignment operators must be trivial,
+//     and there must be at least one non-deleted move assignment operator.
+//   * One of the above clauses must be satisfied; in other words,
+//     it must be either copy constructible, or copy assignable, or
+//     move constructible, or move assignable.
+//   * T must be trivially destructible
+//
+// whereas `std::is_trivially_copyable` checks for:
+//   all of T's copy constructors, move constructors,
+//   copy assignment operators, and move assignment operators are
+//   trivial or deleted, and at least one of these is non-deleted.
+//
+// in addition to the trivially destructible requirement. As an example,
+// see the following class:
+//
+//   struct foo {
+//     foo(const foo&) = default;
+//     foo& operator=(const volatile foo&) = delete;
+//     template <typename T>
+//     foo& operator=(const T&) { return *this; }
+//
+//     foo(foo&&) = delete;
+//     foo& operator=(foo&&) = delete;
+//     ~foo() = default;
+//   };
+//
+// `llvm::is_trivially_copyable<foo>()` is false,
+// since `declval<T&>() = declval<const T&>()` compiles
+// (it finds the template assignment operator),
+// but `foo` has no non-deleted copy assignment operator
+// (a template is never a copy assignment operator).
+//
+// `std::is_trivially_copyable<foo>()` is true,
+// since there are no non-deleted copy assignment operators,
+// and there is a non-deleted trivial copy constructor.
+//
+// This difference is important, since:
+//   * `std::is_trivially_copyable<pair<int, int>>`, but
+//   * `!llvm::is_trivially_copyable<pair<int, int>>`
 template <typename T>
 class is_trivially_copyable {
 
   // copy constructors
   static constexpr bool has_trivial_copy_constructor =
       std::is_copy_constructible<detail::trivial_helper<T>>::value;
   static constexpr bool has_deleted_copy_constructor =
       !std::is_copy_constructible<T>::value;
class is_trivially_copyable {
   public:
 
   static constexpr bool value =
       has_trivial_destructor &&
       (has_deleted_move_assign || has_trivial_move_assign) &&
       (has_deleted_move_constructor || has_trivial_move_constructor) &&
       (has_deleted_copy_assign || has_trivial_copy_assign) &&
       (has_deleted_copy_constructor || has_trivial_copy_constructor);
-
-#ifdef HAVE_STD_IS_TRIVIALLY_COPYABLE
-  static_assert(value == std::is_trivially_copyable<T>::value,
-                "inconsistent behavior between llvm:: and std:: implementation of is_trivially_copyable");
-#endif
 };
 template <typename T>
 class is_trivially_copyable<T*> : public std::true_type {
 };
 
 
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_TYPE_TRAITS_H