Page MenuHomePhabricator

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

Authored by strega-nil on Aug 17 2020, 9:41 PM.



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

Link to bug:

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

Diff Detail

Event Timeline

strega-nil created this revision.Aug 17 2020, 9:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 9:41 PM
strega-nil requested review of this revision.Aug 17 2020, 9:41 PM

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 ;)


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


"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.


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:

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:

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:

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:

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.


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 marked an inline comment as done.Aug 18 2020, 9:29 AM
strega-nil added inline comments.

Yeah, that's my thought :)

strega-nil marked an inline comment as done.

Just for you @CaseyCarter :) (thanks!)

Adding the two people who implemented the function.

Yeah I dunno what's happening here.

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).

Some history on that particular problem: back to 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).

  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)

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, llvm-10.0.1.src/include/llvm/ADT/SmallVector.h

  • llvm-10.0.1.src/include/llvm/ADT/SmallVector.h, 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");

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


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


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 .


I understand why you remove this one, but it also removes the only safe guard we had to prevent the ABI issue solved by 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 added inline comments.Feb 10 2021, 9:26 AM

@serge-sans-paille please correct me If I'm wrong, but 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: )

@danilaml I've submited 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.

Now that landed, we can remove the assert with some more confidence.

This revision is now accepted and ready to land.Feb 26 2021, 2:27 AM