[clang] Add __builtin_start_object_lifetime builtin. #82776

hokein · 2024-02-23T15:27:54Z

This patch implements a clang builtin __builtin_start_object_lifetime, it has the same semantics as C++23's std::start_lifetime_as, but without the implicit-lifetime type restriction, it could be used for implementing std::start_lifetime_as in the future.

Due to the current clang lowering, the builtin reuses the existing __builtin_launder implementation:

it is a no-op for the most part;
with -fstrict-vtable-pointers flag, we update the vtpr assumption correctly (mark the load/store vptr with appropriate invariant group intrinsics) to prevent incorrect vptr load folding;
for now, it is non-constant, thus cannot be executed in constant evaluation;

CAVEAT:

this builtin may cause TBAA miscomplies without the -fno-strict-aliasing flag. These TBAA miscompiles are known issues and may need more LLVM IR support for the fix, fixing them is orthogonal to the implementation of the builtin.

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

llvmbot · 2024-02-23T15:28:22Z

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

This patch implements a clang builtin __builtin_start_object_lifetime, it has the same semantics as C++23's std::start_lifetime_as, but without the implicit-lifetime type restriction, it could be used for implementing std::start_lifetime_as in the future.

Due to the current clang lowering, the builtin reuses the existing __builtin_launder implementation:

it is a no-op for the most part;
with -fstrict-vtable-pointers flag, we update the vtpr assumption correctly (mark the load/store vptr with appropriate invariant group intrinsics) to prevent incorrect vptr load folding;
for now, it is non-constant, thus cannot be executed in constant evaluation;

CAVEAT:

this builtin may cause TBAA miscomplies without the -fno-strict-aliasing flag. These TBAA miscompiles are known issues and may need more LLVM IR support for the fix, fixing them is orthogonal to the implementation of the builtin.

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

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

6 Files Affected:

(modified) clang/include/clang/Basic/Builtins.td (+6)
(modified) clang/lib/CodeGen/CGBuiltin.cpp (+1)
(modified) clang/lib/Sema/SemaChecking.cpp (+2)
(modified) clang/test/CodeGen/builtins.c (+10)
(added) clang/test/CodeGenCXX/builtin-start-object-life-time.cpp (+49)
(modified) clang/test/SemaCXX/builtins.cpp (+32-1)

diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index df74026c5d2d50..70361afe69a980 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -896,6 +896,12 @@ def Launder : Builtin {
   let Prototype = "void*(void*)";
 }
 
+def StartObjectLifeTime : Builtin {
+  let Spellings = ["__builtin_start_object_lifetime"];
+  let Attributes = [NoThrow, CustomTypeChecking];
+  let Prototype = "void*(void*)";
+}
+
 def IsConstantEvaluated : LangBuiltin<"CXX_LANG"> {
   let Spellings = ["__builtin_is_constant_evaluated"];
   let Attributes = [NoThrow, Constexpr];
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index d454ccc1dd8613..7a98f734f32ada 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -4386,6 +4386,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
 
     return RValue::get(nullptr);
   }
+  case Builtin::BI__builtin_start_object_lifetime:
   case Builtin::BI__builtin_launder: {
     const Expr *Arg = E->getArg(0);
     QualType ArgTy = Arg->getType()->getPointeeType();
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 8e763384774444..f79cb7e0445260 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -37,6 +37,7 @@
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/UnresolvedSet.h"
 #include "clang/Basic/AddressSpaces.h"
+#include "clang/Basic/Builtins.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/IdentifierTable.h"
@@ -2386,6 +2387,7 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
     TheCall->setType(Context.IntTy);
     break;
   }
+  case Builtin::BI__builtin_start_object_lifetime:
   case Builtin::BI__builtin_launder:
     return SemaBuiltinLaunder(*this, TheCall);
   case Builtin::BI__sync_fetch_and_add:
diff --git a/clang/test/CodeGen/builtins.c b/clang/test/CodeGen/builtins.c
index 88282120283b8a..f46d6eb7632afc 100644
--- a/clang/test/CodeGen/builtins.c
+++ b/clang/test/CodeGen/builtins.c
@@ -143,6 +143,7 @@ int main(void) {
   P(signbit, (1.0));
 
   R(launder, (&N));
+  R(start_object_lifetime, (&N));
 
   return 0;
 }
@@ -511,6 +512,15 @@ void test_builtin_launder(int *p) {
   int *d = __builtin_launder(p);
 }
 
+/// It should be a NOP in C since there are no vtables.
+// CHECK-LABEL: define{{.*}} void @test_builtin_start_object_lifetime
+void test_builtin_start_object_lifetime(int *p) {
+  // CHECK: [[TMP:%.*]] = load ptr,
+  // CHECK-NOT: @llvm.launder
+  // CHECK: store ptr [[TMP]],
+  int *d = __builtin_start_object_lifetime(p);
+}
+
 // __warn_memset_zero_len should be NOP, see https://sourceware.org/bugzilla/show_bug.cgi?id=25399
 // CHECK-LABEL: define{{.*}} void @test___warn_memset_zero_len
 void test___warn_memset_zero_len(void) {
diff --git a/clang/test/CodeGenCXX/builtin-start-object-life-time.cpp b/clang/test/CodeGenCXX/builtin-start-object-life-time.cpp
new file mode 100644
index 00000000000000..58012f52cc0ef5
--- /dev/null
+++ b/clang/test/CodeGenCXX/builtin-start-object-life-time.cpp
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -emit-llvm -fstrict-vtable-pointers -o - %s \
+// RUN: | FileCheck --check-prefixes=CHECK,CHECK-STRICT %s
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -emit-llvm -o - %s \
+// RUN: | FileCheck --check-prefixes=CHECK,CHECK-NONSTRICT %s
+
+struct TestVirtualFn {
+  virtual void foo();
+};
+// CHECK-LABEL: define{{.*}} void @test_dynamic_class
+extern "C" void test_dynamic_class(TestVirtualFn *p) {
+  // CHECK: store ptr %p, ptr %p.addr
+  // CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr %p.addr
+
+  // CHECK-NONSTRICT-NEXT: store ptr [[TMP0]], ptr %d
+
+  // CHECK-STRICT-NEXT: [[TMP2:%.*]] = call ptr @llvm.launder.invariant.group.p0(ptr [[TMP0]])
+  // CHECK-STRICT-NEXT: store ptr [[TMP2]], ptr %d
+
+  // CHECK-NEXT: ret void
+  TestVirtualFn *d = __builtin_start_object_lifetime(p);
+}
+
+// CHECK-LABEL: define{{.*}} void @test_scalar_pointer
+extern "C" void test_scalar_pointer(int *p) {
+  // CHECK: entry
+  // CHECK-NEXT: %p.addr = alloca ptr
+  // CHECK-NEXT: %d = alloca ptr
+  // CHECK-NEXT: store ptr %p, ptr %p.addr, align 8
+  // CHECK-NEXT: [[TMP:%.*]] = load ptr, ptr %p.addr
+  // CHECK-NEXT: store ptr [[TMP]], ptr %d
+  // CHECK-NEXT: ret void
+  int *d = __builtin_start_object_lifetime(p);
+}
+
+struct TestNoInvariant {
+  int x;
+};
+// CHECK-LABEL: define{{.*}} void @test_non_dynamic_class
+extern "C" void test_non_dynamic_class(TestNoInvariant *p) {
+  // CHECK: entry
+  // CHECK-NOT: llvm.launder.invariant.group
+  // CHECK-NEXT: %p.addr = alloca ptr, align 8
+  // CHECK-NEXT: %d = alloca ptr
+  // CHECK-NEXT: store ptr %p, ptr %p.addr
+  // CHECK-NEXT: [[TMP:%.*]] = load ptr, ptr %p.addr
+  // CHECK-NEXT: store ptr [[TMP]], ptr %d
+  // CHECK-NEXT: ret void
+  TestNoInvariant *d = __builtin_start_object_lifetime(p);
+}
diff --git a/clang/test/SemaCXX/builtins.cpp b/clang/test/SemaCXX/builtins.cpp
index 567094c94c171b..4334b5bbf63663 100644
--- a/clang/test/SemaCXX/builtins.cpp
+++ b/clang/test/SemaCXX/builtins.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify -std=c++11 -fcxx-exceptions
+// RUN: %clang_cc1 %s -fsyntax-only -verify -DCXX11 -std=c++11 -fcxx-exceptions
 // RUN: %clang_cc1 %s -fsyntax-only -verify -std=c++1z -fcxx-exceptions
 typedef const struct __CFString * CFStringRef;
 #define CFSTR __builtin___CFStringMakeConstantString
@@ -156,6 +156,37 @@ void test_noexcept(int *i) {
 #undef TEST_TYPE
 } // end namespace test_launder
 
+namespace test_start_object_lifetime {
+// The builtin is non-constant.
+constexpr int test_non_constexpr(int i) { // expected-error {{constexpr function never produces a constant expression}}
+  __builtin_start_object_lifetime(&i); // expected-note {{subexpression not valid in a constant expression}}
+#ifdef CXX11
+  // expected-warning@-2 {{use of this statement in a constexpr function is a C++14 extension}}
+#endif
+  return 0;
+}
+
+struct Incomplete; // expected-note {{forward declaration}}
+void test_incomplete(Incomplete *i) {
+   // Requires a complete type
+   __builtin_start_object_lifetime(i); // expected-error {{incomplete type 'Incomplete' where a complete type is required}}
+}
+
+// The builtin is type-generic.
+#define TEST_TYPE(Ptr, Type) \
+  static_assert(__is_same(decltype(__builtin_launder(Ptr)), Type), "expected same type")
+void test_type_generic() {
+  char * p;
+  int * i;
+  TEST_TYPE(p, char*);
+  TEST_TYPE(i, int*);
+}
+// The builtin is noexcept.
+void test_noexcept(int *i) {
+  static_assert(noexcept(__builtin_start_object_lifetime(i)), "");
+}
+}
+
 template<typename T> void test_builtin_complex(T v, double d) {
   (void)__builtin_complex(v, d); // expected-error {{different types}} expected-error {{not a real floating}}
   (void)__builtin_complex(d, v); // expected-error {{different types}} expected-error {{not a real floating}}

erichkeane

This would also need a release note I believe. I don't have the codegen expertise to review this with high confidence, but it looks right to me.

erichkeane · 2024-02-23T16:02:02Z

clang/lib/CodeGen/CGBuiltin.cpp

@@ -4386,6 +4386,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,

    return RValue::get(nullptr);
  }
+  case Builtin::BI__builtin_start_object_lifetime:
  case Builtin::BI__builtin_launder: {


Should the TypeRequiresBuiltinLaunder function be generalized, at least in name?

I think the name is fine, we're in the CodeGen, and the BuiltinLaunder in TypeRequiresBuiltinLaunder means the LLVM IR llvm.launder intrinsics.

frederick-vs-ja · 2024-02-23T18:27:34Z

with -fstrict-vtable-pointers flag, we update the vtpr assumption correctly (mark the load/store vptr with appropriate invariant group intrinsics) to prevent incorrect vptr load folding;

I guess we want a variant of this intrinsic which doesn't affect analyzation for devirtualization. Such an intrinsic should be sufficient for C++23 std::start_lifetime_as(_array).

for now, it is non-constant, thus cannot be executed in constant evaluation;

It seems intended that std::start_lifetime_as(_array) is not usable in constant evaluation, as the indeterminism of implicit object creation doesn't seem compatible with constant evaluation.

sam-mccall

Thanks for working on this! I'm far from an expert on the LLVM side so we'll need someone to weigh in.

We need to specify specify the behavior somewhere. I think we should add a description to docs/LanguageExtensions.html, which describes various other __builtin_*.

I'm not sure defining this in terms of "like start_lifetime_as but..." is ideal because:

implicit object creation recurses into only implicit-lifetimes subobjects, where this will recurse into everything. (Whether this is the same or different depends on your persepctive.
Unlike start_lifetime_as, it's clearly more useful to say "this starts an object's lifetime" than "this may start an object's lifetime as needed to avoid UB", I think this clarifies when you can/must call destructors. In this sense it's more like placement-new than start-lifetime-as.

sam-mccall · 2024-02-23T20:08:23Z

clang/include/clang/Basic/Builtins.td

@@ -896,6 +896,12 @@ def Launder : Builtin {
  let Prototype = "void*(void*)";
 }

+def StartObjectLifeTime : Builtin {
+  let Spellings = ["__builtin_start_object_lifetime"];


nit, I'd consider __builtin_start_lifetime both for brevity and to parallel std::start_lifetime_as.
I don't think it's really ambiguous what we're starting the lifetime of.

IMO if the intent of this intrinsic is to handle polymorphic classes (which is not covered by std::start_lifetime_as), its name should diverge from "plain" start_lifetime.

I don't have strong opinion on the name. Since this builtin doesn't strictly align with std::start_lifetime_as, I think it's better to have some divergence in the name.

sam-mccall · 2024-02-23T21:04:27Z

clang/lib/CodeGen/CGBuiltin.cpp

@@ -4386,6 +4386,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,

    return RValue::get(nullptr);
  }
+  case Builtin::BI__builtin_start_object_lifetime:


is there something to be commented here about why what we do here is sufficient?
It seems surprising to my very-untrained eye that start_lifetime is the same as launder, and we don't need any extra TBAA metadata or so...

Added a comment for it, please take a look.

clang/include/clang/Basic/Builtins.td

cor3ntin · 2024-03-04T07:13:02Z

I am a bit concerned that this does not actually have the desired semantics at all, but @zygoloid seemed to be "happy" with it. I will admit I struggle understanding the motivation of adding a builtin that does...less than it should.

Similarly, do you have plans for the array version of start_lifetime_as?

sam-mccall · 2024-03-27T18:38:49Z

This has been dormant in part while thinking about the memcpy half, I think.
Something like #86512 solves that well but likely needs this change too.

I am a bit concerned that this does not actually have the desired semantics at all, but @zygoloid seemed to be "happy" with it. I will admit I struggle understanding the motivation of adding a builtin that does...less than it should.

This does something both useful and correct with -fno-strict-aliasing. I think we're far from alone in building in this mode (in part precisely because of existing TBAA soundness questions!)[1].

I think it gets us incrementally closer to fully supporting std::start_lifetime_as: if there are specific miscompiles this would produce with strict-aliasing enabled, we can now observe them and use them to validate e.g. the llvm.tbaa.fence proposal. (It looks like what we need here but I also don't understand the LLVM change deeply).

Similarly, do you have plans for the array version of start_lifetime_as?

I think we need a similar parallel version of this, e.g.: __start_dynamic_array_lifetime(T*, size_t) -> T*.
This seems like a purely mechanical extension that could be done in this patch or subsequently. Preferences?

[1] I hope this is the year we can put some work into strict-aliasing and turn it on. By coincidence, our best estimate of the overall performance win is roughly the same as applying this optimization to one widely-used library...

clang/include/clang/Basic/Builtins.td

clang/test/SemaCXX/builtins.cpp

frederick-vs-ja · 2024-03-28T00:16:49Z

This does something both useful and correct with -fno-strict-aliasing.

I'm not sure whether an instrinsic is even needed with -fno-strict-aliasing. IIUC std::start_lifetime_as mainly tells the compiler that the types of data in the storage can be changed (indeterminately) with the object representations unchanged. This behaves like some fence for TBAA.

we can now observe them and use them to validate e.g. the llvm.tbaa.fence proposal.

Yeah. I guess we need to implement std::start_lifetime_as(_array) with llvm.tbaa.fence.

This patch implements a clang built `__builtin_start_object_lifetime`, it has the same semantics as C++23's `std::start_lifetime_as`, but without the implicit-lifetime type restriction, it can be used for implementing `std::start_lifetime_as` in the future. Due to the current clang lowering, the builtin reuses the existing `__builtin_launder` implementation: - it is a no-op for the most part; - with `-fstrict-vtable-pointers` flag, we update the vtpr assumption correctly (mark the load/store vptr with appropriate invariant group intrinsics) to prevent incorrect vptr load folding; - for now, the builtin is non-constant, cannot be executed in constant evaluation; CAVEAT: - this builtin may cause TBAA miscomplies without the `-fno-strict-alias` flag. These TBAA miscompiles are known issues and may need more LLVM IR support for the fix, fixing them is orthogonal to the implementaton of the builtin. Context: https://discourse.llvm.org/t/extension-for-creating-objects-via-memcpy/76961

- add doc for the builtin in LanguageExtensions.rst - adjust the existing diagnostics for the new builtin - add release note

hokein · 2024-04-29T08:55:14Z

Friendly ping.

cor3ntin · 2024-04-29T10:41:58Z

@sam-mccall Are you referring to https://reviews.llvm.org/D147020 or something more recent?

ilya-biryukov · 2024-05-16T11:27:27Z

I second @cor3ntin's concern here. We are adding a builtin that is named very similarly to the one from C++23 standard, but not actually having the semantics that is desired.

I believe we do need the effect that __builtin_launder provides to allow memcpy for types that have vtables, which was original motivation for this change and #86512. However, we could get those guarantees by using __builtin_launder directly.
Looking at the code, the __builtin_launder currently has a sole effect of preventing optimizations that would load an incorrect vtable when -fstrict-vtable-pointers is passed and same storage is reused for different types.

I feel it's better to just go with std::launder on our end at this point and instead focus on preventing misoptimizations from TBAA to get a proper implementation of start_lifetime_as. It carries a risk of hitting a misoptimization in the future on our end, but seems very unlikely unless we start using TBAA or LLVM starts optimizing much more aggressively based on C++ notion of object lifetimes, neither of which should go unnoticed.

@hokein WDYT?

hokein requested review from zygoloid, sam-mccall and cor3ntin February 23, 2024 15:27

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Feb 23, 2024

cor3ntin requested a review from erichkeane February 23, 2024 15:58

erichkeane reviewed Feb 23, 2024

View reviewed changes

sam-mccall reviewed Feb 23, 2024

View reviewed changes

zygoloid reviewed Feb 23, 2024

View reviewed changes

clang/include/clang/Basic/Builtins.td Outdated Show resolved Hide resolved

sam-mccall mentioned this pull request Mar 27, 2024

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

Merged

sam-mccall reviewed Mar 27, 2024

View reviewed changes

clang/include/clang/Basic/Builtins.td Outdated Show resolved Hide resolved

clang/test/SemaCXX/builtins.cpp Outdated Show resolved Hide resolved

hokein force-pushed the start_lifetime branch 2 times, most recently from 60f2e26 to 68b26fe Compare April 2, 2024 11:17

hokein added 2 commits April 29, 2024 09:52

- address review comments

4f1caac

- add doc for the builtin in LanguageExtensions.rst - adjust the existing diagnostics for the new builtin - add release note

hokein force-pushed the start_lifetime branch from 68b26fe to 4f1caac Compare April 29, 2024 08:19

cor3ntin mentioned this pull request Sep 4, 2024

[libc++] P2590R2: Explicit lifetime management #107206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Add __builtin_start_object_lifetime builtin. #82776

[clang] Add __builtin_start_object_lifetime builtin. #82776

hokein commented Feb 23, 2024

llvmbot commented Feb 23, 2024 •

edited

Loading

erichkeane left a comment

erichkeane Feb 23, 2024

hokein Apr 1, 2024

frederick-vs-ja commented Feb 23, 2024

sam-mccall left a comment

sam-mccall Feb 23, 2024

frederick-vs-ja Feb 25, 2024

hokein Apr 2, 2024

sam-mccall Feb 23, 2024

hokein Apr 1, 2024

cor3ntin commented Mar 4, 2024 •

edited

Loading

sam-mccall commented Mar 27, 2024

frederick-vs-ja commented Mar 28, 2024

hokein commented Apr 29, 2024

cor3ntin commented Apr 29, 2024

ilya-biryukov commented May 16, 2024

[clang] Add __builtin_start_object_lifetime builtin. #82776

Are you sure you want to change the base?

[clang] Add __builtin_start_object_lifetime builtin. #82776

Conversation

hokein commented Feb 23, 2024

llvmbot commented Feb 23, 2024 • edited Loading

erichkeane left a comment

Choose a reason for hiding this comment

erichkeane Feb 23, 2024

Choose a reason for hiding this comment

hokein Apr 1, 2024

Choose a reason for hiding this comment

frederick-vs-ja commented Feb 23, 2024

sam-mccall left a comment

Choose a reason for hiding this comment

sam-mccall Feb 23, 2024

Choose a reason for hiding this comment

frederick-vs-ja Feb 25, 2024

Choose a reason for hiding this comment

hokein Apr 2, 2024

Choose a reason for hiding this comment

sam-mccall Feb 23, 2024

Choose a reason for hiding this comment

hokein Apr 1, 2024

Choose a reason for hiding this comment

cor3ntin commented Mar 4, 2024 • edited Loading

sam-mccall commented Mar 27, 2024

frederick-vs-ja commented Mar 28, 2024

hokein commented Apr 29, 2024

cor3ntin commented Apr 29, 2024

ilya-biryukov commented May 16, 2024

llvmbot commented Feb 23, 2024 •

edited

Loading

cor3ntin commented Mar 4, 2024 •

edited

Loading