is_trivially_copyable_v misbehaves

[llvmbot]

Bugzilla Link	39050
Version	7.0
OS	Windows NT
Reporter	LLVM Bugzilla Contributor
CC	@DougGregor,@mclow,@zygoloid,@rjmccall

Extended Description

Tested on various c++17 compatible versions on godbolt.

Assuming cppreference is correct (https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable) and my interpretation of their text is also correct.

std::is_trivially_copyable_v doesn't follow the standard's definition.

Example 1, Trivial non-deleted destructor :
The deleted destructor should fail the check : https://godbolt.org/z/5BOO68

Example 2, at least one copy constructor, move constructor, copy assignment operator, or move assignment operator is non-deleted :
There are no copy/move ctors or assignement operators, should fail the check : https://godbolt.org/z/APcHua

I tested on gcc with the same results. I'm assuming I've missed something.

Does libc++ use is_trivially_copyable_v internally for vector resize?

[zygoloid]

This is DR1734, which Clang (and apparently GCC) does not yet implement.

[llvmbot]

Does a fix for DR1734 require an ABI flag to enable the fix? Or can the fix after its implemented always be active?

I'm asking because
a) My colleague Erich Keane recalls a conversation with Richard Smith that changing trivially copyable is a somewhat important ABI break
b) DR 2094 was done because the previous change caused a IA-64 ABI break. But I don't fully understand why that was concluded. I understand that trivial types impacts things like register passing. But I don't understand why a type is considered trivial by the ABI is impacted by whether a type is considered trivial by C++. Because I don't fully understand the details of that I don't understand whether this would also impact this fix.

And I would like to understand what's required to implement a fix for DR1734.

[zygoloid]

We can condition a fix behind -fclang-abi-compat so that people can opt into behavior that's compatible with old Clang versions. Before proceeding with this, we should gather some data as to how common it is for this language change to actually change the trivial-copyability of existing class types.

There are ABI rules that use triviality to drive record layout choices (64-bit iOS, WatchOS, wasm, Fuchsia), for which this would potentially have more significant impact. We would need to decide whether these ABI rules retain the old semantics (even though they involve asking whether a deleted function is trivial, which is a meaningless question).

[llvmbot]

We can condition a fix behind -fclang-abi-compat so that people can opt into behavior that's compatible with old Clang versions.

Would the flag only impact whether a type is considered trivial for ABI purposes or would that flag also change what the type-traits return?

Before proceeding with this, we should gather some data as to how common it is for this language change to actually change the trivial-copyability of existing class types.

Do you mean how often effected types exist at all or how often the traits is_trivial/is_trivial_copyable are used with those effected types?

There are ABI rules that use triviality to drive record layout choices (64-bit iOS, WatchOS, wasm, Fuchsia), for which this would potentially have more significant impact. We would need to decide whether these ABI rules retain the old semantics (even though they involve asking whether a deleted function is trivial, which is a meaningless question).

Do we have a choice? For Itanium trivial is defined as

it has a non-trivial copy constructor, move constructor, or destructor, or
all of its copy and move constructors are deleted.

https://itanium-cxx-abi.github.io/cxx-abi/abi.html#non-trivial . This is clearly different from the C++ definition. Is this different for the other ABIs? Don't they have their own definition of trivial and instead refer to the C++ definition of trivial?

[zygoloid]

We can condition a fix behind -fclang-abi-compat so that people can opt into behavior that's compatible with old Clang versions.

Would the flag only impact whether a type is considered trivial for ABI
purposes or would that flag also change what the type-traits return?

I think this should affect the type traits. We do something similar for whether we consider types 'empty', and that that also affects what the type traits return. Because of how the traits are used by the standard library, this choice is largely forced on us -- changing the value produced by one of these traits is a de facto ABI change.

Before proceeding with this, we should gather some data as to how common it is for this language change to actually change the trivial-copyability of existing class types.

Do you mean how often effected types exist at all or how often the traits
is_trivial/is_trivial_copyable are used with those effected types?

I was thinking the former; use of the traits is probably much less interesting than implicit uses involved in ABI decisions. But really anything that gives us a sense of how careful we need to be would be useful.

There are ABI rules that use triviality to drive record layout choices (64-bit iOS, WatchOS, wasm, Fuchsia), for which this would potentially have more significant impact. We would need to decide whether these ABI rules retain the old semantics (even though they involve asking whether a deleted function is trivial, which is a meaningless question).

Do we have a choice? For Itanium trivial is defined as

it has a non-trivial copy constructor, move constructor, or destructor, or
all of its copy and move constructors are deleted.

https://itanium-cxx-abi.github.io/cxx-abi/abi.html#non-trivial . This is
clearly different from the C++ definition.

This is intended to be the same as the C++ rule for when you can pass in registers. (The ABI rule intends for you to treat deleted functions as not being non-trivial; it should probably explicitly say so.)

But that's not what I was talking about, since it's not defined in terms of whether a class is trivially-copyable. The part of the ABI I was referring to is "POD for the purpose of layout", for which various different targets use different rules, and the four that I listed above base their rule on Clang's broken notion of "trivially-copyable".

[llvmbot]

As per the report in bug #​49002, please can we ensure that this code builds with any fix:

#include <type_traits>

class Bar {
public:
int A;
// User-declared move assignment operator, defined as deleted
Bar& operator=(Bar&&) = delete;
};

static_assert(!std::is_trivially_copyable::value, "Bar is trivially copyable");

Thanks, Andrew R

[llvmbot]

mentioned in issue llvm/llvm-bugzilla-archive#49002

[philnik777]

Looking at the implementation of is_trivally_copyable it seems like this is a clang bug (specifically the __is_trivially_copyable implementation), not a libc++ bug.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[LYP951018]

I'm hacking on this (also DR1496, they are very similar) these days. Initial thoughts:

• implementing this would fix std::is_trivially_copyable_v is false using clang for class with const class field. #59624 automatically, because deleted move ctor is not eligible. But this breaks the PS4 ABI, so we must introduce a new ABI flag. PS4 ABI won'be affected.

• There's no direct way to determine whether a class has an eligible special member currently. I'm going to use HasTrivialSpecialMembers, DeclaredNonTrivialSpecialMembers, needsImplicitXXX and DefaultedXXXIsDeleted together to do that, like

    bool HasEligibleCopyCtor() { 
      return
        (HasTrivialSpecialMembers & SMF_CopyConstructor) || 
       (DeclaredNonTrivialSpecialMembers & SMF_CopyConstructor) || 
       (needsImplicitCopyConstructor() && !DefaultedCopyConstructorIsDeleted); 
   }

• It seems that HasTrivialSpecialMembers's semantic has been changed to HasEligibleTrivialSpecialMembers. So, if I make deleted functions not eligible anymore, __has_trivial_copy's result will change for deleted copy ctor. Is that acceptable? related issue: behavior for __has_trivial_{copy,assign} matches gcc's documentation but not gcc's behavior #18559. This will be hidden behind the compat flag.

• A related issue I found: https://reviews.llvm.org/D86126

I appreciate your feedback and suggestions. ;)

UPDATE:

• Richard said

  There are ABI rules that use triviality to drive record layout choices (64-bit iOS, WatchOS, wasm, Fuchsia), for which this would potentially have more significant impact. We would need to decide whether these ABI rules retain the old semantics (even though they involve asking whether a deleted function is trivial, which is a meaningless question).

I need to investigate on that further. (UPDATE: see comments below)

UPDATE 2:
I think if we want to maintain the ABI for Fuchsia etc. , we need something like HasCXX11TrivialSpecialMembers which is just like IsCXX11StandardLayout It seems difficult to maintain the ABI of iOS64/Fuchsia/... without introducing a compat ABI version ...

[Endilll]

CC @AaronBallman