Page MenuHomePhabricator

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

Authored by serge-sans-paille on Feb 11 2021, 12:38 PM.

Details

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.

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Feb 11 2021, 12:38 PM
serge-sans-paille created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2021, 12:38 PM

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.

@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 accepted this revision.Feb 24 2021, 3:01 AM

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

This revision is now accepted and ready to land.Feb 24 2021, 3:01 AM

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 :-/

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.

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.

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 added inline comments.
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 (git@github.com:llvm/llvm-project.git llvmorg-10.0.1-0-gef32c611aa2)