Page MenuHomePhabricator

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

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

Details

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.

Diff Detail

Unit TestsFailed

TimeTest
700 mslinux > LLVM.Transforms/OpenMP::parallel_deletion.ll
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt -S -attributor -openmpopt < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/OpenMP/parallel_deletion.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/OpenMP/parallel_deletion.ll
80 mswindows > LLVM.DebugInfo/X86::addr-tu-to-non-tu.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llc.exe -filetype=obj -O0 -generate-type-units -split-dwarf-file=x.dwo < C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\DebugInfo\X86\addr-tu-to-non-tu.ll | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\llvm-dwarfdump.exe -debug-info -debug-types - | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe --implicit-check-not=Unit --implicit-check-not=contents --implicit-check-not=declaration C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\DebugInfo\X86\addr-tu-to-non-tu.ll
530 mswindows > LLVM.Transforms/OpenMP::parallel_deletion.ll
Script: -- : 'RUN: at line 2'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\opt.exe -S -attributor -openmpopt < C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\Transforms\OpenMP\parallel_deletion.ll | c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16c2-1\llvm-project\premerge-checks\llvm\test\Transforms\OpenMP\parallel_deletion.ll

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

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 marked an inline comment as done.Aug 18 2020, 9:29 AM
strega-nil added inline comments.
llvm/include/llvm/Support/type_traits.h
161

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

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

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 .

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 added inline comments.Feb 10 2021, 9:26 AM
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 )

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

Now that https://reviews.llvm.org/D96536 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