Make sure some types are indeed trivially_copyable per llvm::is_trivially_copyable

[serge-sans-paille]

Summary

Test a few types used as llvm::SmallVector parameter. It is important to ensure
we have a consistent behavior for these types to prevent ABI issues as the one
we met in https://bugs.llvm.org/show_bug.cgi?id=39427.

[danilaml]

I don't know whether these checks are enough (have trivial copy constructor =/=> trivially copyable AFAIK), but otherwise LGTM.

llvm/unittests/Support/TypeTraitsTest.cpp

37

I guess these shouldn't have the same problem as trivially_copyable.

[serge-sans-paille]

@danilaml This is certainly not complete, but that's much better than just removing an assert as proposed in https://reviews.llvm.org/D86126. Once that one is accepted, we can move forward and commit https://reviews.llvm.org/D86126

[danilaml]

In D96536#2582905, @serge-sans-paille wrote:

@danilaml This is certainly not complete, but that's much better than just removing an assert as proposed in https://reviews.llvm.org/D86126. Once that one is accepted, we can move forward and commit https://reviews.llvm.org/D86126

It's better, I just still not completely get why that assert was necessary in the first place. I.e. what exact scenario is it trying to prevent? If we just want all trivially_copyable uses to be consistent across compilers, then that assert won't help (at the very least since std:: is not consistent across compiler/std versions). If we just don't want to memcpy wrong types but can't use std:: version due to implementation issues on some old compilers, then assert is not helpful either. Although it can be changed to test llvm:: => std::, instead of <=>, so llvm version won't accidentally be more permissive than the std one.

[serge-sans-paille]

It's better, I just still not completely get why that assert was necessary in the first place. I.e. what exact scenario is it trying to prevent? If we just want all trivially_copyable uses to be consistent across compilers, then that assert won't help (at the very least since std:: is not consistent across compiler/std versions).

Yeah, that was the original attempt: make sure that llvm::is_trivially_copyable was consistent with std::is_trivially_copyable when compiler implement it (at that time, some compiler were not supporting it all). As you know, experience proved that this was a naive approach :-/

[danilaml]

In D96536#2585263, @serge-sans-paille wrote:

It's better, I just still not completely get why that assert was necessary in the first place. I.e. what exact scenario is it trying to prevent? If we just want all trivially_copyable uses to be consistent across compilers, then that assert won't help (at the very least since std:: is not consistent across compiler/std versions).

Yeah, that was the original attempt: make sure that llvm::is_trivially_copyable was consistent with std::is_trivially_copyable when compiler implement it (at that time, some compiler were not supporting it all). As you know, experience proved that this was a naive approach :-/

That's a bit different and seems like a non-issue. We can't guarantee consistency across compilers without compiling with several compilers. And assert by itself gives us nothing, like I've explained. We should only care that llvm:: => std:: at most.
I don't think that checks added in this patch solve this in any way.

Also not sure why the attempt to replace llvm:: with std:: has failed (probably some weird mix of obsolete STL with a newer patched GCC), although if it's the ABI we want, it doesn't matter.

[serge-sans-paille]

Also not sure why the attempt to replace llvm:: with std:: has failed (probably some weird mix of obsolete STL with a newer patched GCC), although if it's the ABI we want, it doesn't matter.

The root of this problem lies in llvm::OptionalStorage which is specialized based on the value of llvm::is_trivially_copyable. In order to guarantee that code compiled with, say, clang, can link with LLVM compiled with, say gcc, we need to guarantee that the same specialization happen for the same type. That's the problem described in the bug https://bugs.llvm.org/show_bug.cgi?id=39427 I quoted in the commit comment.

As you point out, we cannot rely on std::is_trivially_copyable to enforce that guarantee, because it's not reproducible across compiler (this is something I didn't know back in the day, and probably hold true at some point for a subset of compiler large enough not to trigger the assert I added).
This is why we're looking for decent way to ensure llvm::is_trivially_copyable behaves thr same across compilers. And this patch is a decent minimal approach at doing so.

[danilaml]

In D96536#2585609, @serge-sans-paille wrote:

Also not sure why the attempt to replace llvm:: with std:: has failed (probably some weird mix of obsolete STL with a newer patched GCC), although if it's the ABI we want, it doesn't matter.

The root of this problem lies in llvm::OptionalStorage which is specialized based on the value of llvm::is_trivially_copyable. In order to guarantee that code compiled with, say, clang, can link with LLVM compiled with, say gcc, we need to guarantee that the same specialization happen for the same type. That's the problem described in the bug https://bugs.llvm.org/show_bug.cgi?id=39427 I quoted in the commit comment.

As you point out, we cannot rely on std::is_trivially_copyable to enforce that guarantee, because it's not reproducible across compiler (this is something I didn't know back in the day, and probably hold true at some point for a subset of compiler large enough not to trigger the assert I added).
This is why we're looking for decent way to ensure llvm::is_trivially_copyable behaves thr same across compilers. And this patch is a decent minimal approach at doing so.

That would cause a linking issue. I was talking about error in https://reviews.llvm.org/D92591#2431801

These tests are enough to ensure that we get expected result for llvm::is_trivially_*_constructible for types we care about but they don't test is_trivially_copyable.

[arichardson]

llvm/unittests/Support/TypeTraitsTest.cpp

113

This causes a build failure on FreeBSD12.2:

[1196/1577] Building CXX object unittests/Support/CMakeFiles/SupportTests.dir/TypeTraitsTest.cpp.o
FAILED: unittests/Support/CMakeFiles/SupportTests.dir/TypeTraitsTest.cpp.o
/usr/local/bin/clang++11 -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iunittests/Support -I/exports/users/alr48/sources/upstream-llvm-project/llvm/unittests/Support -Iinclude -I/exports/users/alr48/sources/upstream-llvm-project/llvm/include -I/exports/users/alr48/sources/upstream-llvm-project/llvm/utils/unittest/googletest/include -I/exports/users/alr48/sources/upstream-llvm-project/llvm/utils/unittest/googlemock/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -O3   -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -UNDEBUG -Wno-suggest-override -std=c++14 -MD -MT unittests/Support/CMakeFiles/SupportTests.dir/TypeTraitsTest.cpp.o -MF unittests/Support/CMakeFiles/SupportTests.dir/TypeTraitsTest.cpp.o.d -o unittests/Support/CMakeFiles/SupportTests.dir/TypeTraitsTest.cpp.o -c /exports/users/alr48/sources/upstream-llvm-project/llvm/unittests/Support/TypeTraitsTest.cpp
/exports/users/alr48/sources/upstream-llvm-project/llvm/unittests/Support/TypeTraitsTest.cpp:28:3: error: static_assert failed due to requirement 'llvm::is_trivially_copy_constructible<std::__1::pair<int, bool>>::value == true' "Mismatch in expected trivial copy construction!"
  static_assert(llvm::is_trivially_copy_constructible<T>::value ==
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/exports/users/alr48/sources/upstream-llvm-project/llvm/unittests/Support/TypeTraitsTest.cpp:113:3: note: in instantiation of function template specialization '(anonymous namespace)::triviality::TrivialityTester<std::__1::pair<int, bool>, true, true>' requested here
  TrivialityTester<std::pair<int, bool>, true, true>();
  ^
/exports/users/alr48/sources/upstream-llvm-project/llvm/unittests/Support/TypeTraitsTest.cpp:31:3: error: static_assert failed due to requirement 'llvm::is_trivially_move_constructible<std::__1::pair<int, bool>>::value == true' "Mismatch in expected trivial move construction!"
  static_assert(llvm::is_trivially_move_constructible<T>::value ==
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/exports/users/alr48/sources/upstream-llvm-project/llvm/unittests/Support/TypeTraitsTest.cpp:37:3: error: static_assert failed due to requirement 'std::is_trivially_copy_constructible<std::__1::pair<int, bool>>::value == true' "Mismatch in expected trivial copy construction!"
  static_assert(std::is_trivially_copy_constructible<T>::value ==
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/exports/users/alr48/sources/upstream-llvm-project/llvm/unittests/Support/TypeTraitsTest.cpp:40:3: error: static_assert failed due to requirement 'std::is_trivially_move_constructible<std::__1::pair<int, bool>>::value == true' "Mismatch in expected trivial move construction!"
  static_assert(std::is_trivially_move_constructible<T>::value ==
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4 errors generated.

I'm not 100% what version of libc++ is being used as the system libc++, but clang is 10.0.1 so it's probably libc++ from that same git revision ([e-mail]:llvm/llvm-project.git llvmorg-10.0.1-0-gef32c611aa2)

llvm/unittests/Support/TypeTraitsTest.cpp

 //===- TypeTraitsTest.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/PointerIntPair.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/type_traits.h"
 #include "gtest/gtest.h"
 
 namespace {
 
 // Compile-time tests using static assert.
 namespace triviality {
 
 // Helper for compile time checking trivially copy constructible and trivially
 // move constructible type traits.
 template <typename T, bool IsTriviallyCopyConstructible,
           bool IsTriviallyMoveConstructible>
 void TrivialityTester() {
   static_assert(llvm::is_trivially_copy_constructible<T>::value ==
                     IsTriviallyCopyConstructible,
                 "Mismatch in expected trivial copy construction!");
   static_assert(llvm::is_trivially_move_constructible<T>::value ==
                     IsTriviallyMoveConstructible,
                 "Mismatch in expected trivial move construction!");
 
 #if defined(_LIBCPP_VERSION) || defined(_MSC_VER)
   // On compilers with support for the standard traits, make sure they agree.
   static_assert(std::is_trivially_copy_constructible<T>::value ==
                     IsTriviallyCopyConstructible,
                 "Mismatch in expected trivial copy construction!");
   static_assert(std::is_trivially_move_constructible<T>::value ==
                     IsTriviallyMoveConstructible,
                 "Mismatch in expected trivial move construction!");
 #endif
 }
 
TEST(Triviality, Tester) {
   TrivialityTester<Z &, true, true>();
   TrivialityTester<A &, true, true>();
   TrivialityTester<B &, true, true>();
   TrivialityTester<Z &&, false, true>();
   TrivialityTester<A &&, false, true>();
   TrivialityTester<B &&, false, true>();
 }
 
+// Test that the following ADT behave as expected wrt. trivially copyable trait
+//
+// NB: It is important that this trait behaves the same for (at least) these
+// types for all supported compilers to prevent ABI issue when llvm is compiled
+// with compiler A and an other project using llvm is compiled with compiler B.
+
+TEST(Triviality, ADT) {
+
+  TrivialityTester<llvm::SmallVector<int>, false, false>();
+  TrivialityTester<llvm::SmallString<8>, false, false>();
+
+  TrivialityTester<std::function<int()>, false, false>();
+  TrivialityTester<std::pair<int, bool>, true, true>();
+  TrivialityTester<llvm::unique_function<int()>, false, false>();
+  TrivialityTester<llvm::StringRef, true, true>();
+  TrivialityTester<llvm::ArrayRef<int>, true, true>();
+  TrivialityTester<llvm::PointerIntPair<int *, 2>, true, true>();
+  TrivialityTester<llvm::Optional<int>, true, true>();
+}
+
 } // namespace triviality
 
 } // end anonymous namespace