Page MenuHomePhabricator

Fix lvm::is_trivially_copyable portability issues
ClosedPublic

Authored by serge-sans-paille on Jan 21 2019, 7:24 AM.

Details

Summary

llvm::is_trivially_copyable portability is verified at compile time using std::is_trivially_copyable as the reference implementation. Unfortunately, the latter is not available on all platforms, so introduce a proper configure check to detect if it is available on the target platform.

In a similar manner, std::is_copy_assignable is not fully supported for gcc4.9. Provide a portable (?) implementation instead.

Diff Detail

Repository
rL LLVM

Event Timeline

fedor.sergeev requested changes to this revision.Jan 21 2019, 12:42 PM

Sorry, now it fails on is_move_assignable:

] cat copyable_with_llvm.cpp
#include "llvm/Support/type_traits.h"
// code from tools/llvm-xray/xray-converter.cpp
template <typename AssociatedData> struct TrieNode {
  int FuncId;
  AssociatedData ExtraData;
};
struct StackIdData {
  // to get is_trivially_copyable instantiation from within the class
  int x [llvm::is_trivially_copyable<TrieNode<StackIdData> *>::value + 1];
};
] g++ -std=c++11 -I ../../llvm/include -I include copyable_with_llvm.cpp 
copyable_with_llvm.cpp: In instantiation of ‘struct TrieNode<StackIdData>’:
/gcc/include/c++/4.9.2/type_traits:1183:45:   required by substitution of ‘template<class _Tp1, class _Up1, class> static std::true_type std::__is_assignable_helper<_Tp, _Up>::__test(int) [with _Tp1 = llvm::detail::trivial_helper<TrieNode<StackIdData>*>&; _Up1 = llvm::detail::trivial_helper<TrieNode<StackIdData>*>&&; <template-parameter-1-3> = <missing>]’
/gcc/include/c++/4.9.2/type_traits:1192:30:   required from ‘class std::__is_assignable_helper<llvm::detail::trivial_helper<TrieNode<StackIdData>*>&, llvm::detail::trivial_helper<TrieNode<StackIdData>*>&&>’
/gcc/include/c++/4.9.2/type_traits:1197:12:   required from ‘struct std::is_assignable<llvm::detail::trivial_helper<TrieNode<StackIdData>*>&, llvm::detail::trivial_helper<TrieNode<StackIdData>*>&&>’
/gcc/include/c++/4.9.2/type_traits:1227:12:   required from ‘struct std::__is_move_assignable_impl<llvm::detail::trivial_helper<TrieNode<StackIdData>*>, true>’
/gcc/include/c++/4.9.2/type_traits:1233:12:   required from ‘struct std::is_move_assignable<llvm::detail::trivial_helper<TrieNode<StackIdData>*> >’
../../llvm/include/llvm/Support/type_traits.h:157:25:   required from ‘constexpr const bool llvm::is_trivially_copyable<TrieNode<StackIdData>*>::has_trivial_move_assign’
../../llvm/include/llvm/Support/type_traits.h:170:32:   required from ‘constexpr const bool llvm::is_trivially_copyable<TrieNode<StackIdData>*>::value’
copyable_with_llvm.cpp:9:64:   required from here
copyable_with_llvm.cpp:5:18: error: ‘TrieNode<AssociatedData>::ExtraData’ has incomplete type
   AssociatedData ExtraData;
                  ^
copyable_with_llvm.cpp:7:8: error: forward declaration of ‘struct StackIdData’
 struct StackIdData {
        ^

And yes, the same failure on the actual llvm build on xray-converter.cpp.

This revision now requires changes to proceed.Jan 21 2019, 12:42 PM
fedor.sergeev added inline comments.Jan 21 2019, 1:29 PM
include/llvm/Support/type_traits.h
125 ↗(On Diff #182801)

Somehow this declval<F> thing does not look right. Doesnt it have F&& type?
Besides, the result of assignment might as well be a reference, which will reject forming a pointer on its decltype.

serge-sans-paille added a subscriber: sylvestre.ledru.

A few comments missing but I'd like to know if this now works correctly on gcc 4.9, @fedor.sergeev , @Anastasia and @sylvestre.ledru . I successfully passed test on a dedicated setup so I guess it's ok...

it does *not* work on my shorter reproducer:

] g++ -std=c++11 -I ../../llvm/include -I include ../../llvm/copyable_with_llvm.cpp 
../../llvm/copyable_with_llvm.cpp: In instantiation of ‘struct TrieNode<StackIdData>’:
../../llvm/include/llvm/Support/type_traits.h:118:34:   required by substitution of ‘template<class T> using IsCopyAssignableImpl = decltype ((declval<T&>)()=(declval<const T&>)()) [with T = llvm::detail::trivial_helper<TrieNode<StackIdData>*>]’
../../llvm/include/llvm/Support/type_traits.h:114:8:   required from ‘struct llvm::detail::IsDetected<llvm::detail::IsCopyAssignableImpl, llvm::detail::trivial_helper<TrieNode<StackIdData>*> >’
../../llvm/include/llvm/Support/type_traits.h:149:8:   required from ‘struct llvm::is_copy_assignable<llvm::detail::trivial_helper<TrieNode<StackIdData>*> >’
../../llvm/include/llvm/Support/type_traits.h:174:25:   required from ‘constexpr const bool llvm::is_trivially_copyable<TrieNode<StackIdData>*>::has_trivial_copy_assign’
../../llvm/include/llvm/Support/type_traits.h:195:32:   required from ‘constexpr const bool llvm::is_trivially_copyable<TrieNode<StackIdData>*>::value’
../../llvm/copyable_with_llvm.cpp:12:64:   required from here
../../llvm/copyable_with_llvm.cpp:7:18: error: ‘TrieNode<AssociatedData>::ExtraData’ has incomplete type
   AssociatedData ExtraData;
                  ^
../../llvm/copyable_with_llvm.cpp:10:8: error: forward declaration of ‘struct StackIdData’
 struct StackIdData {
        ^

Started full build, will report as soon as it is ready.

Started full build, will report as soon as it is ready.

On xray-converter.cpp fails the same way as on my shorter reproducer.

fedor.sergeev accepted this revision.Jan 22 2019, 12:59 AM

Both the small reproducer and full LLVM-only builds/tests passed with my gcc 4.9.
LGTM

This revision is now accepted and ready to land.Jan 22 2019, 12:59 AM
xbolva00 added inline comments.
cmake/config-ix.cmake
328 ↗(On Diff #182850)

Typo

Slightly cleaner implementation, added comments and tested with polly.

serge-sans-paille marked an inline comment as done.Jan 22 2019, 2:31 AM
TWeaver added a subscriber: TWeaver.EditedJan 22 2019, 3:44 AM

Hi there,

we're seeing build failures on our internal CI as a result of the issue this patch aims to fix.

hope I don't seem too pushy but, are there any plans to land this in the next few hours?

we'd like to see our CI back up and running as soon as possible to avoid any further headaches.

Thanks in advance
Tom W

Yeah, I do a final test round and I'll push that one. It's a very touchy piece of code, so I don't want to make all builders fail again...

This revision was automatically updated to reflect the committed changes.

We've been having issues with std::is_trivially_copyable too, and it seems not even this patch solves the problems for us.

I'm running an Ubuntu 14.04 machine with clang 3.6.0 and libstdc++-4.8-dev and now I get the following when I compile on trunk:

In file included from ../utils/TableGen/CodeGenSchedule.cpp:14:
In file included from ../utils/TableGen/CodeGenSchedule.h:18:
In file included from ../include/llvm/ADT/DenseMap.h:16:
In file included from ../include/llvm/ADT/DenseMapInfo.h:16:
In file included from ../include/llvm/ADT/ArrayRef.h:12:
In file included from ../include/llvm/ADT/Hashing.h:48:
In file included from ../include/llvm/Support/Host.h:16:
In file included from ../include/llvm/ADT/StringMap.h:16:
In file included from ../include/llvm/ADT/StringRef.h:12:
In file included from ../include/llvm/ADT/STLExtras.h:19:
In file included from ../include/llvm/ADT/Optional.h:21:
../include/llvm/Support/type_traits.h:127:57: error: object of type 'llvm::detail::trivial_helper<std::pair<void *, unsigned long> >' cannot

  be assigned because its copy assignment operator is implicitly deleted
static auto get(F*) -> decltype(std::declval<T &>() = std::declval<const T &>(), std::true_type{});
                                                    ^

../include/llvm/Support/type_traits.h:162:7: note: in instantiation of template class

'llvm::is_copy_assignable<llvm::detail::trivial_helper<std::pair<void *, unsigned long> > >' requested here
is_copy_assignable<detail::trivial_helper<T>>::value;
^

../include/llvm/ADT/SmallVector.h:184:30: note: in instantiation of template class 'llvm::is_trivially_copyable<std::pair<void *, unsigned

long> >' requested here

template <typename T, bool = is_trivially_copyable<T>::value>

^

../include/llvm/ADT/SmallVector.h:321:32: note: in instantiation of default argument for 'SmallVectorTemplateBase<std::pair<void *, unsigned

long> >' required here

class SmallVectorImpl : public SmallVectorTemplateBase<T> {

^~~~~~~~~~~~~~~~~~~~~~~~~~

../include/llvm/ADT/SmallVector.h:845:28: note: in instantiation of template class 'llvm::SmallVectorImpl<std::pair<void *, unsigned long> >'

requested here

class SmallVector : public SmallVectorImpl<T>, SmallVectorStorage<T, N> {

^

../include/llvm/Support/Allocator.h:373:45: note: in instantiation of template class 'llvm::SmallVector<std::pair<void *, unsigned long>, 0>'

    requested here
SmallVector<std::pair<void *, size_t>, 0> CustomSizedSlabs;
                                          ^

../include/llvm/Support/type_traits.h:96:7: note: copy assignment operator of 'trivial_helper<std::pair<void *, unsigned long> >' is

  implicitly deleted because variant field 't' has a non-trivial copy assignment operator
T t;
  ^

Just saw build-bots failed the same way

jmorse added a subscriber: jmorse.Jan 22 2019, 6:12 AM

seems like the best thing to do here is revert the original commit that broke the bots in the first place?

and thanks for the patch either way, nice to know we're not in this alone.

TWeaver added a comment.EditedJan 22 2019, 6:25 AM

Slight fix for r351820 seems to have done the trick, many thanks!

@uabelho top-of-tree should be fine now.

@uabelho top-of-tree should be fine now.

Yep, thanks!