[clang] Implement a bitwise_copyable builtin type trait. #86512

hokein · 2024-03-25T14:58:19Z

This patch implements a __is_bitwise_cloneable builtin in clang.

The builtin is used as a guard to check a type can be safely bitwise copied by memcpy. It's functionally similar to __is_trivially_copyable, but covers a wider range of types (e.g. classes with virtual functions). The compiler guarantees that after copy, the destination object has the same object representations as the source object. And it is up to user to guarantee that program semantic constraints are satisfied.

Context: https://discourse.llvm.org/t/extension-for-creating-objects-via-memcpy

llvmbot · 2024-03-25T14:58:51Z

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

This patch implements a __is_bitwise_copyable builtin in clang.

The bitwise copyable types act as the trivially copyable types, but they support a wider range of types (e.g. classes with virtual methods) -- their underlying types can be safely copied by memcpy or memmove, the clang compiler guarantees that both source and destination objects have the same object representations after the copy operation, and the lifetime of the destination object implicitly starts.

A particular use case of this builtin is to clone an object via memcopy (without running the constructor):

Message* clone(const Message* src, char* buffer, int size) {
  if constexpr __is_bitwise_copyable(Message) {
    // bitwise copy to buffer, and implicitly create objects at the buffer
    __builtin_memcpy(buffer, src, size);
    return std::launder(reinterpret_cast&lt;Message*&gt;(buffer));
  }
  // Fallback the operator new, which calls the constructor to start the lifetime.
  return new(buffer) Message(src);
}

Note that the definition of bitwise copyable is not tied to the Rule Of Five, so users of this builtin must guarantee that program semantic constraints are satisfied, e.g. no double resource deallocations.

Context: https://discourse.llvm.org/t/extension-for-creating-objects-via-memcpy

Full diff: https://github.com/llvm/llvm-project/pull/86512.diff

5 Files Affected:

(modified) clang/include/clang/AST/Type.h (+12)
(modified) clang/include/clang/Basic/TokenKinds.def (+1)
(modified) clang/lib/AST/Type.cpp (+23)
(modified) clang/lib/Sema/SemaExprCXX.cpp (+4)
(added) clang/test/SemaCXX/builtin-is-bitwise-copyable.cpp (+26)

diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 1942b0e67f65a3..9f819185d954cd 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -917,6 +917,18 @@ class QualType {
   /// Return true if this is a trivially copyable type (C++0x [basic.types]p9)
   bool isTriviallyCopyableType(const ASTContext &Context) const;
 
+  /// Return true if this is a bitwise copyable type.
+  ///
+  /// This is an extension in clang: bitwise copyable types act as trivially
+  /// copyable types, underlying bytes of bitwise copyable type can be safely
+  /// copied by memcpy or memmove. Clang guarantees that both source and
+  /// destination objects have the same **object** representations after the
+  /// copy, and the lifetime of the destination object implicitly starts.
+  ///
+  /// bitwise copyable types cover a wider range of types, e.g. classes with
+  /// virtual methods.
+  bool isBitwiseCopyableType(const ASTContext &Context) const;
+
   /// Return true if this is a trivially copyable type
   bool isTriviallyCopyConstructibleType(const ASTContext &Context) const;
 
diff --git a/clang/include/clang/Basic/TokenKinds.def b/clang/include/clang/Basic/TokenKinds.def
index 3a96f8a4d22bd1..bd0f15c8b56d9e 100644
--- a/clang/include/clang/Basic/TokenKinds.def
+++ b/clang/include/clang/Basic/TokenKinds.def
@@ -526,6 +526,7 @@ TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"
 
 // Clang-only C++ Type Traits
+TYPE_TRAIT_1(__is_bitwise_copyable, IsBitwiseCopyable, KEYCXX)
 TYPE_TRAIT_1(__is_trivially_relocatable, IsTriviallyRelocatable, KEYCXX)
 TYPE_TRAIT_1(__is_trivially_equality_comparable, IsTriviallyEqualityComparable, KEYCXX)
 TYPE_TRAIT_1(__is_bounded_array, IsBoundedArray, KEYCXX)
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 22666184c56ccf..0164c72f0d5cf2 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2667,6 +2667,29 @@ bool QualType::isTriviallyCopyableType(const ASTContext &Context) const {
                                      /*IsCopyConstructible=*/false);
 }
 
+bool QualType::isBitwiseCopyableType(const ASTContext & Context) const {
+  QualType CanonicalType = getCanonicalType();
+  if (CanonicalType->isIncompleteType() || CanonicalType->isDependentType())
+    return false;
+  // Trivially copyable types are bitwise copyable, e.g. scalar types.
+  if (CanonicalType.isTriviallyCopyableType(Context))
+    return true;
+
+  if (CanonicalType->isArrayType())
+    return Context.getBaseElementType(CanonicalType)
+        .isBitwiseCopyableType(Context);
+
+  if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {
+    for (auto *const Field : RD->fields()) {
+      QualType T = Context.getBaseElementType(Field->getType());
+      if (!T.isBitwiseCopyableType(Context))
+        return false;
+    }
+    return true;
+  }
+  return false;
+}
+
 bool QualType::isTriviallyCopyConstructibleType(
     const ASTContext &Context) const {
   return isTriviallyCopyableTypeImpl(*this, Context,
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index c34a40fa7c81ac..dc20ec47f5b574 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -5056,6 +5056,8 @@ static bool CheckUnaryTypeTraitTypeCompleteness(Sema &S, TypeTrait UTT,
   case UTT_IsStandardLayout:
   case UTT_IsPOD:
   case UTT_IsLiteral:
+  // Clang extension:
+  case UTT_IsBitwiseCopyable:
   // By analogy, is_trivially_relocatable and is_trivially_equality_comparable
   // impose the same constraints.
   case UTT_IsTriviallyRelocatable:
@@ -5547,6 +5549,8 @@ static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT,
     return C.hasUniqueObjectRepresentations(T);
   case UTT_IsTriviallyRelocatable:
     return T.isTriviallyRelocatableType(C);
+  case UTT_IsBitwiseCopyable:
+    return T.isBitwiseCopyableType(C);
   case UTT_IsReferenceable:
     return T.isReferenceable();
   case UTT_CanPassInRegs:
diff --git a/clang/test/SemaCXX/builtin-is-bitwise-copyable.cpp b/clang/test/SemaCXX/builtin-is-bitwise-copyable.cpp
new file mode 100644
index 00000000000000..68e5dd21aa47d8
--- /dev/null
+++ b/clang/test/SemaCXX/builtin-is-bitwise-copyable.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+
+// Scalar types are bitwise copyable.
+static_assert(__is_bitwise_copyable(int));
+static_assert(__is_bitwise_copyable(int*));
+// array
+static_assert(__is_bitwise_copyable(int[10]));
+
+
+struct Forward; // expected-note 2{{forward declaration of 'Forward'}}
+static_assert(!__is_bitwise_copyable(Forward)); // expected-error {{incomplete type 'Forward' used in type trait expression}}
+
+struct Foo { int a; };
+static_assert(__is_bitwise_copyable(Foo));
+
+struct DynamicClass { virtual int Foo(); };
+static_assert(__is_bitwise_copyable(DynamicClass));
+
+template <typename T>
+void TemplateFunction() {
+  static_assert(__is_bitwise_copyable(T)); // expected-error {{incomplete type 'Forward' used in type trait expression}}
+}
+void CallTemplateFunc() {
+  TemplateFunction<Forward>(); // expected-note {{in instantiation of function template specialization}}
+  TemplateFunction<Foo>();
+}

github-actions · 2024-03-25T15:01:47Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2024-03-25T15:01:47Z

✅ With the latest revision this PR passed the C/C++ code formatter.

sam-mccall

I'm in favour of this, but we should have someone less-attached to it sign off on at least the basic design.

One question this doesn't explicitly address: after memcpying, does user code need to bless the bits somehow to start the new object's lifetime?

I think the answer should probably be yes, even if blessing is a no-op for now. #82776 seems suitable.

__is_trivially_copyable doesn't require this, but the types it applies to are pretty limited in terms of lifetime concerns, so I think it's OK to draw a distinction.

One argument for this is with no "bless" barrier, I don't understand exactly where the line is drawn for strict-aliasing purposes:

if we can memcpy an object A into a buffer B
then surely we can copy it ourselves byte-by-byte
and we can then immediately copy it back from B to A, if the destructor is trivial
now we get to treat it as a different type
Here A's bytes didn't change - is the write to B important? Do we have to write the bytes sequentially for this transmutation of A to work, or can we reuse a single byte variable?

This all seems weird and like we're reinventing implicit-lifetime types. That's by necessity a weird feature, but the weirdness is not needed here.

sam-mccall · 2024-03-27T13:28:02Z

clang/lib/AST/Type.cpp

@@ -2667,6 +2667,29 @@ bool QualType::isTriviallyCopyableType(const ASTContext &Context) const {
                                     /*IsCopyConstructible=*/false);
 }

+bool QualType::isBitwiseCopyableType(const ASTContext & Context) const {
+  QualType CanonicalType = getCanonicalType();
+  if (CanonicalType->isIncompleteType() || CanonicalType->isDependentType())


should the answer here be dependent if the type is dependent?
similarly should incomplete be an error?

what do other traits do?

Other traits (see isTriviallyCopyableTypeImpl) do the similar things for incomplete and dependent types, we follow what they did in this type trait.

sam-mccall · 2024-03-27T13:28:24Z

clang/lib/AST/Type.cpp

+
+  if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {
+    for (auto *const Field : RD->fields()) {
+      QualType T = Context.getBaseElementType(Field->getType());


we're missing base classes and such

sam-mccall · 2024-03-27T13:28:39Z

clang/lib/AST/Type.cpp

+    return Context.getBaseElementType(CanonicalType)
+        .isBitwiseCopyableType(Context);
+
+  if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {


this seems to exclude very few types, like reference members. Is this the right approach?

Yeah, for reference members, its type is non-scalar type, thus this is excluded for now (reference types are also not trivially copyable).

The current approach, is to traverse the entire class on every query, which has some cost.
A more efficient alternative is to use an extra bit in the class definition data, we propagate this bit during the construction of this class.

sam-mccall · 2024-03-27T13:29:19Z

clang/include/clang/Basic/TokenKinds.def

@@ -526,6 +526,7 @@ TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"

 // Clang-only C++ Type Traits
+TYPE_TRAIT_1(__is_bitwise_copyable, IsBitwiseCopyable, KEYCXX)


why c++ only?

switched to KEYALL

sam-mccall · 2024-03-27T14:42:09Z

clang/include/clang/Basic/TokenKinds.def

@@ -526,6 +526,7 @@ TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"

 // Clang-only C++ Type Traits
+TYPE_TRAIT_1(__is_bitwise_copyable, IsBitwiseCopyable, KEYCXX)


naming thought #1 (sorry):

if we're going to use "bit" here, you might consider e.g. __is_bit_copyable rather than "bitwise", for consistency with std::bit_cast which is closely related to this concept.

(In a vacuum I do like "bitwise" better, though "bytewise_copyable" seems more accurate: the copy (noun) is bit-for-bit but the copy (verb) is byte-by-byte)

sam-mccall · 2024-03-27T14:44:11Z

clang/include/clang/Basic/TokenKinds.def

@@ -526,6 +526,7 @@ TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"

 // Clang-only C++ Type Traits
+TYPE_TRAIT_1(__is_bitwise_copyable, IsBitwiseCopyable, KEYCXX)


naming thought #2:

I'm a little concerned that all other type traits that mention "copy" use it to mean "is equivalent to the language-level copy", not "the copy is physically possible".

For that reason I would consider e.g. __is_bitwise_cloneable.

OK, renamed to __is_bitwise_cloneable.

sam-mccall · 2024-03-27T14:56:28Z

clang/include/clang/Basic/TokenKinds.def

@@ -526,6 +526,7 @@ TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"

 // Clang-only C++ Type Traits
+TYPE_TRAIT_1(__is_bitwise_copyable, IsBitwiseCopyable, KEYCXX)


this new trait should be documented in docs/LanguageExtensions.

The documentation needs to explicitly mention whether or not the user code needs to perform a "start lifetime" operation to incarnate the resulting bytes.

Done, added one, please take a look.

The LanguageExtensions.rst is a huge doc with many sections, I added a new item to the Builtin Functions ¶ bucket. Happy to re-arrange it to a more suitable place.

sam-mccall · 2024-03-27T14:58:27Z

clang/lib/Sema/SemaExprCXX.cpp

@@ -5056,6 +5056,8 @@ static bool CheckUnaryTypeTraitTypeCompleteness(Sema &S, TypeTrait UTT,
  case UTT_IsStandardLayout:
  case UTT_IsPOD:
  case UTT_IsLiteral:
+  // Clang extension:


AFAIK these are all clang extensions (the __ versions), so I'm not sure this comment is clear/helpful

sam-mccall · 2024-03-27T15:35:57Z

clang/test/SemaCXX/builtin-is-bitwise-copyable.cpp

+void CallTemplateFunc() {
+  TemplateFunction<Forward>(); // expected-note {{in instantiation of function template specialization}}
+  TemplateFunction<Foo>();
+}


if there are some class types not intended to be bitwise copyable (like struct { int &x; }), we should have negative test cases too

Added some negative test cases (int&). Note that struct Foo { int &x; }; is considered bitwise cloneable (as it is trivially copyable).

cor3ntin · 2024-04-04T05:53:50Z

clang/include/clang/AST/Type.h

+  /// copied by memcpy or memmove. Clang guarantees that both source and
+  /// destination objects have the same **object** representations after the
+  /// copy, and the lifetime of the destination object implicitly starts.
+  ///
+  /// bitwise copyable types cover a wider range of types, e.g. classes with
+  /// virtual methods.


I don't understand that comment. We do not check the type has implicit lifetime anywhere? And because it's a query, no copy is made there so there is no guarantee to be made.

I made some adjustments to this comment, hopefully it is clearer.

cor3ntin · 2024-04-04T05:56:21Z

clang/include/clang/AST/Type.h

+  /// destination objects have the same **object** representations after the
+  /// copy, and the lifetime of the destination object implicitly starts.
+  ///
+  /// bitwise copyable types cover a wider range of types, e.g. classes with


wider range than what?

than the trivially-copyable types. I think it may be better to remove it, to avoid this confusion.

cor3ntin · 2024-04-04T05:56:49Z

clang/include/clang/Basic/TokenKinds.def

@@ -526,6 +526,7 @@ TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, KEYCXX)
 #include "clang/Basic/TransformTypeTraits.def"

 // Clang-only C++ Type Traits
+TYPE_TRAIT_1(__is_bitwise_copyable, IsBitwiseCopyable, KEYCXX)


hokein · 2024-04-29T07:59:37Z

Friendly ping, it is ready for the second round of review.

ilya-biryukov · 2024-05-14T16:19:42Z

A few comments from me to move this review forward.
The major question I have is about the tests and how reference members should affect the results of that trait.

ilya-biryukov · 2024-05-13T14:18:14Z

clang/lib/AST/Type.cpp

+        return false;
+
+    for (auto *const Field : RD->fields()) {
+      QualType T = Context.getBaseElementType(Field->getType());


Why do we need to call getBaseElementType here?
Why not simply call the isBitwiseCloneableType(Field->getType())?

Switched to isBitwiseCloneableType(Field->getType()).

ilya-biryukov · 2024-05-13T14:30:56Z

clang/docs/LanguageExtensions.rst

+**Description**:
+
+It is common for library owners to perform memcpy/memmove on types that aren't
+trivally copyable for performance reason. However, according to the C++ standard,


NIT: I think it would be useful to explain the differences between this and is_trivially_copyable.

Something like:

This trait is similar to `std::is_trivially_copyable`, but additionally allows to have user-defined constructors, virtual functions and virtual bases. It is up to the user code to guarantee that a bitwise copy results in non-broken object and that the lifetime of an object is properly started.

clang/test/SemaCXX/builtin-is-bitwise-cloneable.cpp

ilya-biryukov · 2024-05-14T16:16:59Z

clang/test/SemaCXX/builtin-is-bitwise-cloneable.cpp

+
+struct Bar { int& b; }; // trivially copyable
+static_assert(__is_trivially_copyable(Bar));
+static_assert(__is_bitwise_cloneable(Bar));


I don't quite get the rule here.
If reference members make the class non-bitwise-cloneable, why isn't that assertion firing?
If they don't, why is the next assertion a problem (the two classes only differ by presence of constructor, which we choose to ignore in bitwise-cloneability).

This is a tricky bit.

The question arises: should we consider all trivially copyable types to be bitwise cloneable? My inclination is to say yes. Since memcpy is safe on all trivially copyable types, it's probably a good idea to ensure that bitwise_clonable covers as many types as possible.

I think it makes a lot of sense to make this a strict superset of trivially copyable types.
But why isn't next example bitwise-cloneable then?

struct Bar2 { Bar2(const Bar2&); int& b; }; static_assert(!__is_trivially_copyable(Bar2)); static_assert(!__is_bitwise_cloneable(Bar2));

Bar2 is not trivially copyable because of the presence of a user-defined constructor, we chose to drop that requirement for the new trait. Why isn't the type bitwise-cloneable?

Because of the reference member int & b.

Reference types are currently excluded in bitwise-cloneable types (references are just alias, and they cannot be reset once initialized).

But the reference member in Bar does not stop it from being bitwise-cloneable.
We should make up our mind about it: do reference members stop the type from being bitwise-cloneable or not?

ilya-biryukov · 2024-05-14T16:30:05Z

clang/lib/AST/Type.cpp

+
+  if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {
+    for (auto Base : RD->bases())
+      if (!Base.getType().isBitwiseCloneableType(Context))


I wanted to note that because we aren't caching results of the computations for classes, the running time may end up expontential for some examples.

A synthetic example would be:

struct F1 { /*...*/ }; struct F2 : F1 {} struct F3 : F1, F2 {} struct F4 : F2, F3 {} ...

I don't think this would matter much in practice, but at least leaving a FIXME for the future would be useful.

Good point, added a FIXME (we could use an extra bit in the class definition class to cache the result).

hokein

thanks for the review.

hokein · 2024-05-15T12:47:55Z

clang/test/SemaCXX/builtin-is-bitwise-cloneable.cpp

+
+struct Bar { int& b; }; // trivially copyable
+static_assert(__is_trivially_copyable(Bar));
+static_assert(__is_bitwise_cloneable(Bar));


This is a tricky bit.

The question arises: should we consider all trivially copyable types to be bitwise cloneable? My inclination is to say yes. Since memcpy is safe on all trivially copyable types, it's probably a good idea to ensure that bitwise_clonable covers as many types as possible.

clang/test/SemaCXX/builtin-is-bitwise-cloneable.cpp

hokein · 2024-05-16T07:56:37Z

clang/lib/AST/Type.cpp

+
+  if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {
+    for (auto Base : RD->bases())
+      if (!Base.getType().isBitwiseCloneableType(Context))


Good point, added a FIXME (we could use an extra bit in the class definition class to cache the result).

hokein · 2024-05-16T08:10:07Z

clang/lib/AST/Type.cpp

+        return false;
+
+    for (auto *const Field : RD->fields()) {
+      QualType T = Context.getBaseElementType(Field->getType());


Switched to isBitwiseCloneableType(Field->getType()).

hokein · 2024-05-16T08:14:00Z

clang/docs/LanguageExtensions.rst

+**Description**:
+
+It is common for library owners to perform memcpy/memmove on types that aren't
+trivally copyable for performance reason. However, according to the C++ standard,


ilya-biryukov

Could we rewrite the description and documentation to capture what the type trait is doing now?
We have a lot of references to the is_trivially_copyable, which is almost fully irrelevant to the current implementation.

Also, is there a simple way to make this work with the corresponding sanitizer?
If not or it requires too much work, we could definitely return false here. Just making sure we explored this.

ilya-biryukov · 2024-05-28T15:28:41Z

clang/docs/LanguageExtensions.rst

+
+After the copy, the lifetime of the new object isn't started yet (unless the
+type is trivially copyable). Users must ensure its lifetime is started to avoid
+undefined behavior.


Users must ensure its lifetime is started

What should the intended action taken by the user be? Is this advice actionable?
I guess that if the type is implicit-lifetime-type, users could use start_lifetime_as (although it's not implemented in Clang).
AFAIK, there is no way to start a lifetime of other objects (e.g. containing virtual members).

I suggest to either mention the start_lifetime_as or simply say that it's still undefined behavior to use this object, i.e.

Note that after the copy the lifetime of the new object isn't started and using it is undefined behavior from the C++ Standard's perspective. In C++23 `std::start_lifetime_as` can start the lifetime of implicit-lifetime types. There is no standard way to start a lifetime of other objects, e.g. that have a vtable.

What should the intended action taken by the user be? Is this advice actionable?
I guess that if the type is implicit-lifetime-type, users could use start_lifetime_as (although it's not implemented in Clang).
AFAIK, there is no way to start a lifetime of other objects (e.g. containing virtual members).

For implicit-lifetime types, users don't need to start the lifetime explicitly, the memcpy implicitly starts the lifetime of the object, per the standard; for other types, yeah, this is no standard way to do it (for our case, we plan to do the trick with __builtin_launder, but I would not mention it in the public doc).

How about this?

For implicit-lifetime types, the lifetime of the new object is implicitly started after the memcpy. For other types (e.g., classes with virtual methods), the lifetime isn't started, and using the object results in undefined behavior according to the C++ Standard, and there is no standard way to start the lifetime.

ilya-biryukov · 2024-05-28T16:05:10Z

clang/include/clang/AST/Type.h

@@ -1120,6 +1120,14 @@ class QualType {
  /// Return true if this is a trivially copyable type (C++0x [basic.types]p9)
  bool isTriviallyCopyableType(const ASTContext &Context) const;

+  /// Return true if the type is safe to bitwise copy by memcpy.
+  ///
+  /// This is an extension in clang: bitwise clonable types act as trivially


We should update the documentation to talk about the things that we are actually checking now, i.e. that memcpy will succeed and have a valid object representation, even if the lifetime is not formally started.
Mentioning when it's not the case would also be helpful:

memcpy fails, e.g. sanitizers that add padding with poison.

(potentially) types with ObjC lifetimes.

Done, updated the documentation accordingly, please take a second look.

ilya-biryukov · 2024-05-28T16:12:01Z

clang/lib/AST/Type.cpp

@@ -2749,6 +2749,17 @@ bool QualType::isTriviallyCopyableType(const ASTContext &Context) const {
                                     /*IsCopyConstructible=*/false);
 }

+bool QualType::isBitwiseCloneableType(const ASTContext & Context) const {
+  if (const auto *RD = getCanonicalType()->getAsCXXRecordDecl()) {


Should we also disallow the objects that return true for hasNonTrivialObjCLifetime?
That's the part of the is_trivially_copyable that seems relevant. I don't have a good understanding of ObjC, so I'd rather lean on the safe side here. (I.e. maybe there are low-level ways to update the reference counters after memcpy, I don't really know. But assuming there aren't, I'd rather lean on the side of returning false for this trait)

Agree, for the safety reason, we should disable it for NonTrivialObjCLifetime. Done.

hokein · 2024-05-29T13:53:50Z

Could we rewrite the description and documentation to capture what the type trait is doing now? We have a lot of references to the is_trivially_copyable, which is almost fully irrelevant to the current implementation.

Thanks, I updated the PR description and documentation in the code, please take a look again.
I think it is worth to mentioning the is_trivially_copyable, they're conceptually relevant (users use is_trivially_copyable as a safe guard for memcpy).

Also, is there a simple way to make this work with the corresponding sanitizer? If not or it requires too much work, we could definitely return false here. Just making sure we explored this.

This doesn't scale well. There are a few sanitizers (asan, msan etc), and each sanitizer has their own version of memcpy. It is non-trivial to check all sanitizers to ensure all of them work properly. And it doesn't solve types with non-trivial ObjectiveC lifetime.

hokein · 2024-05-29T14:00:14Z

Add @eugenis as a reviewer, could you take a look on the sanitizer bit?

eugenis

mayInsertExtraPadding logic looks fine to me

ilya-biryukov

LGTM with a few NITs to fix before submitting.

ilya-biryukov · 2024-06-03T13:46:50Z

clang/docs/LanguageExtensions.rst

+
+Objects of bitwise cloneable types can be bitwise copied by memcpy/memmove. The
+Clang compiler warrants that this behavior is well defined, and won't be
+broken by compiler optimizations.


NIT: by compiler optimizations and sanitizers

ilya-biryukov · 2024-06-03T13:49:14Z

clang/include/clang/AST/Type.h

+  ///  - Types with Objective-C lifetimes, where specific runtime
+  ///    semantics may not be preserved during a bitwise copy.
+  ///
+  // FIXME: each call will trigger a full computation, cache the result.


NIT: move this FIXME into the implementation. I don't think it's relevant to the public API contract of this method.

ilya-biryukov · 2024-06-03T13:53:24Z

clang/lib/AST/Type.cpp

+  if (CanonicalType->isIncompleteType())
+    return false;
+
+  if (const auto *RD = CanonicalType->getAsRecordDecl()) { // struct/union/class


NIT: rewrite to reduce nesting, see LLVM Code Style.

const auto *RD = CanonicalType->getAsRecordDecl(); if (!RD) return true; // ... Rest of the code.

ilya-biryukov · 2024-06-03T13:59:53Z

Also, we need to fix formatting errors and make sure the CI is green.
E.g. the newly added test fails (needs an update to capture some notes).

This patch implements a `__is_bitwise_copyable` builtin in clang. The bitwise copyable types act as the trivially copyable types, but they support a wider range of types (e.g. classes with virtual methods) -- their underlying types can be safely copied by `memcopy` or `memmove`, the clang compiler guarantees that both source and destination objects have the same *object* representations after the copy operation, and the lifetime of the destination object implicitly starts. A particular use case of this builtin is to clone an object via memcopy (without running the constructor): ``` Message* clone(const Message* src, char* buffer, int size) { if constexpr __is_bitwise_copyable(Message) { // bitwise copy to buffer __builtin_memcpy(buffer, src, size); // using __builtin_launder to prevent miscompile for -fstrict-vtable-pointers. return __builtin_launder(reinterpret_cast<Message*>(buffer)); } // Fallback the operator new, which calls the constructor to start the lifetime. return new(buffer) Message(src); } ``` Note that the definition of bitwise copyable is not tied to the Rule Of Five, so users of this builtin must guarantee that program semantic constraints are satisfied, e.g. no double resource deallocations. Context: https://discourse.llvm.org/t/extension-for-creating-objects-via-memcpy

frederick-vs-ja · 2024-06-06T06:08:54Z

IIUC this intrinsic is unrelated to assignment operators. Do you have any plan for __is_bitwise_assignable, which should determine whether the assignment operator equivalently copies the value representation, possibly modulo the vptrs?

struct Cat {}; // bitwise copy assignable
struct Leopard : Cat { // is_trivially_assignable_v returns true but not bitwise copy assignable
    int spots_;
    Leopard& operator=(Leopard&) = delete;
    using Cat::operator=; // We can detect this by metaprogramming as operator= overloads have wrong types.
};
struct LeopardHouse { // is_trivially_assignable_v returns true but not bitwise copy assignable
    Leopard _;
    // Non-bitwise-assignability *can't* be detected by metaprogramming since operator= overloads have right types.
};

hokein requested review from sam-mccall and cor3ntin March 25, 2024 14:58

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 25, 2024

sam-mccall reviewed Mar 27, 2024

View reviewed changes

sam-mccall mentioned this pull request Mar 27, 2024

[clang] Add __builtin_start_object_lifetime builtin. #82776

Open

cor3ntin reviewed Apr 4, 2024

View reviewed changes

hokein force-pushed the bitwise-copyable-trait branch from 481627e to f64fe43 Compare April 16, 2024 11:47

ilya-biryukov self-requested a review May 14, 2024 16:20

ilya-biryukov reviewed May 14, 2024

View reviewed changes

hokein force-pushed the bitwise-copyable-trait branch from f64fe43 to d7f9e01 Compare May 16, 2024 08:17

hokein commented May 16, 2024

View reviewed changes

hokein force-pushed the bitwise-copyable-trait branch from d7f9e01 to 49747cd Compare May 28, 2024 13:06

ilya-biryukov requested changes May 28, 2024

View reviewed changes

hokein force-pushed the bitwise-copyable-trait branch from 49747cd to 011d6bb Compare May 29, 2024 13:45

hokein requested a review from eugenis May 29, 2024 14:00

eugenis reviewed May 30, 2024

View reviewed changes

ilya-biryukov approved these changes Jun 3, 2024

View reviewed changes

hokein force-pushed the bitwise-copyable-trait branch from 011d6bb to 0eec963 Compare June 5, 2024 11:27

hokein force-pushed the bitwise-copyable-trait branch from 0eec963 to 3ce8779 Compare June 5, 2024 11:31

hokein merged commit f6c1e65 into llvm:main Jun 6, 2024
8 checks passed

[clang] Implement a bitwise_copyable builtin type trait. #86512

[clang] Implement a bitwise_copyable builtin type trait. #86512

Conversation

hokein commented Mar 25, 2024 • edited Loading

llvmbot commented Mar 25, 2024

github-actions bot commented Mar 25, 2024

github-actions bot commented Mar 25, 2024 • edited Loading

sam-mccall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hokein commented Apr 29, 2024

ilya-biryukov commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-biryukov May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hokein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-biryukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hokein commented May 29, 2024

hokein commented May 29, 2024

eugenis left a comment

Choose a reason for hiding this comment

ilya-biryukov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-biryukov commented Jun 3, 2024

frederick-vs-ja commented Jun 6, 2024

hokein commented Mar 25, 2024 •

edited

Loading

github-actions bot commented Mar 25, 2024 •

edited

Loading

ilya-biryukov May 14, 2024 •

edited

Loading