[Clang] No longer require complete types with __builtin_launder #91070

MitalAshok · 2024-05-04T16:35:53Z

Incomplete types are assumed to need the llvm.launder.invariant.group intrinsic

Fixes #90949

llvmbot · 2024-05-04T16:36:23Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Mital Ashok (MitalAshok)

Changes

Incomplete types are assumed to need the llvm.launder.invariant.group intrinsic

Fixes #90949

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

5 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+2)
(modified) clang/lib/CodeGen/CGBuiltin.cpp (+3-2)
(modified) clang/lib/Sema/SemaChecking.cpp (+1-19)
(modified) clang/test/CodeGenCXX/builtin-launder.cpp (+56)
(modified) clang/test/SemaCXX/builtins.cpp (+11-3)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 54b58b1ae99fbd..e22a80a6f281c9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -552,6 +552,8 @@ Bug Fixes in This Version
 - Clang will no longer emit a duplicate -Wunused-value warning for an expression
   `(A, B)` which evaluates to glvalue `B` that can be converted to non ODR-use. (#GH45783)
 
+- `__builtin_launder` no longer requires a pointer to a complete type. (#GH90949)
+
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 8e31652f4dabef..6a544f97cac5e2 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -2487,8 +2487,9 @@ TypeRequiresBuiltinLaunderImp(const ASTContext &Ctx, QualType Ty,
   if (!Seen.insert(Record).second)
     return false;
 
-  assert(Record->hasDefinition() &&
-         "Incomplete types should already be diagnosed");
+  // Assume incomplete types need to be laundered
+  if (!Record->hasDefinition())
+    return true;
 
   if (Record->isDynamicClass())
     return true;
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 3179d542b1f926..c2eb5b51975c1a 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2164,15 +2164,7 @@ static ExprResult BuiltinLaunder(Sema &S, CallExpr *TheCall) {
   //  * The type of the argument if it's not an array or function type,
   //  Otherwise,
   //  * The decayed argument type.
-  QualType ParamTy = [&]() {
-    QualType ArgTy = TheCall->getArg(0)->getType();
-    if (const ArrayType *Ty = ArgTy->getAsArrayTypeUnsafe())
-      return S.Context.getPointerType(Ty->getElementType());
-    if (ArgTy->isFunctionType()) {
-      return S.Context.getPointerType(ArgTy);
-    }
-    return ArgTy;
-  }();
+  QualType ParamTy = S.Context.getAdjustedParameterType(TheCall->getArg(0)->getType());
 
   TheCall->setType(ParamTy);
 
@@ -2191,16 +2183,6 @@ static ExprResult BuiltinLaunder(Sema &S, CallExpr *TheCall) {
     return ExprError();
   }
 
-  // We either have an incomplete class type, or we have a class template
-  // whose instantiation has not been forced. Example:
-  //
-  //   template <class T> struct Foo { T value; };
-  //   Foo<int> *p = nullptr;
-  //   auto *d = __builtin_launder(p);
-  if (S.RequireCompleteType(TheCall->getBeginLoc(), ParamTy->getPointeeType(),
-                            diag::err_incomplete_type))
-    return ExprError();
-
   assert(ParamTy->getPointeeType()->isObjectType() &&
          "Unhandled non-object pointer case");
 
diff --git a/clang/test/CodeGenCXX/builtin-launder.cpp b/clang/test/CodeGenCXX/builtin-launder.cpp
index 06a93d1c441d29..8cfbc3101e30d3 100644
--- a/clang/test/CodeGenCXX/builtin-launder.cpp
+++ b/clang/test/CodeGenCXX/builtin-launder.cpp
@@ -53,10 +53,66 @@ extern "C" void test_builtin_launder_virtual_base(TestVirtualBase *p) {
   TestVirtualBase *d = __builtin_launder(p);
 }
 
+struct IncompleteNeedsLaunder;
+
+// CHECK-LABEL: define{{.*}} void @test_builtin_launder_incomplete_later_needs_launder
+extern "C" void test_builtin_launder_incomplete_later_needs_launder(IncompleteNeedsLaunder *p) {
+  // CHECK-STRICT-NOT: ret void
+  // CHECK-STRICT: @llvm.launder.invariant.group
+
+  // CHECK-NONSTRICT-NOT: @llvm.launder.invariant.group
+
+  // CHECK: ret void
+  IncompleteNeedsLaunder *d = __builtin_launder(p);
+}
+
+struct IncompleteNeedsLaunder {
+  virtual void foo() {}
+};
+
+// CHECK-LABEL: define{{.*}} void @test_builtin_launder_completed_needs_launder
+extern "C" void test_builtin_launder_completed_needs_launder(IncompleteNeedsLaunder *p) {
+  // CHECK-STRICT-NOT: ret void
+  // CHECK-STRICT: @llvm.launder.invariant.group
+
+  // CHECK-NONSTRICT-NOT: @llvm.launder.invariant.group
+
+  // CHECK: ret void
+  IncompleteNeedsLaunder *d = __builtin_launder(p);
+}
+
+struct IncompleteDoesntNeedLaunder;
+
+// CHECK-LABEL: define{{.*}} void @test_builtin_launder_incomplete_later_doesnt_needs_launder
+extern "C" void test_builtin_launder_incomplete_later_doesnt_needs_launder(IncompleteDoesntNeedLaunder *p) {
+  // CHECK-STRICT-NOT: ret void
+  // CHECK-STRICT: @llvm.launder.invariant.group
+
+  // CHECK-NONSTRICT-NOT: @llvm.launder.invariant.group
+
+  // CHECK: ret void
+  IncompleteDoesntNeedLaunder *d = __builtin_launder(p);
+}
+
 //===----------------------------------------------------------------------===//
 //                            Negative Cases
 //===----------------------------------------------------------------------===//
 
+struct IncompleteDoesntNeedLaunder {};
+
+// CHECK-LABEL: define{{.*}} void @test_builtin_launder_completed_doesnt_need_launder
+extern "C" void test_builtin_launder_completed_doesnt_need_launder(IncompleteDoesntNeedLaunder *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
+  IncompleteDoesntNeedLaunder *d = __builtin_launder(p);
+}
+
 // CHECK-LABEL: define{{.*}} void @test_builtin_launder_ommitted_one
 extern "C" void test_builtin_launder_ommitted_one(int *p) {
   // CHECK: entry
diff --git a/clang/test/SemaCXX/builtins.cpp b/clang/test/SemaCXX/builtins.cpp
index 080b4476c7eec1..bf587c3aa0ccb9 100644
--- a/clang/test/SemaCXX/builtins.cpp
+++ b/clang/test/SemaCXX/builtins.cpp
@@ -144,16 +144,24 @@ void f() {
   static_assert(test_in_constexpr(i), "");
 }
 
-struct Incomplete; // expected-note {{forward declaration}}
+struct Incomplete;
 struct IncompleteMember {
   Incomplete &i;
 };
 void test_incomplete(Incomplete *i, IncompleteMember *im) {
-  // expected-error@+1 {{incomplete type 'Incomplete' where a complete type is required}}
-  __builtin_launder(i);
+  __builtin_launder(i); // OK
   __builtin_launder(&i); // OK
   __builtin_launder(im); // OK
 }
+extern Incomplete incomplete;
+extern IncompleteMember incomplete_member;
+static_assert(test_constexpr_launder(&incomplete) == &incomplete, "");
+static_assert(test_constexpr_launder(&incomplete_member) == &incomplete_member, "");
+template<typename> struct X { static_assert(false, ""); };
+extern X<void> x;
+static_assert(__builtin_launder(__builtin_addressof(x)) == __builtin_addressof(x), "");
+static_assert((test_constexpr_launder)(__builtin_addressof(x)) == __builtin_addressof(x), "");
+template<> struct X<void> {};
 
 void test_noexcept(int *i) {
   static_assert(noexcept(__builtin_launder(i)), "");

github-actions · 2024-05-04T16:38:43Z

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

Incomplete types are assumed to need the llvm.launder.invariant.group intrinsic Fixes llvm#90949

cor3ntin

Should we try to force an instantiation in codegen for complete-but-not-yet-instantiated classes?

erichkeane · 2024-05-10T12:23:53Z

This patch doesn't really make sense to me. Why would we want __builtin_launder to work on incomplete types?

cor3ntin · 2024-05-10T12:32:11Z

The committee says so

Because launder is just adding a fence on a pointer to T, we need not know what T is.

I don't know whether there are scenarios where calling launder on an incomplete type would be useful (as presumably you would new an object right before which would require completeness) but it's also harmless.

erichkeane · 2024-05-10T12:34:20Z

The committee says so

Because launder is just adding a fence on a pointer to T, we need not know what T is.

I don't know whether there are scenarios where calling launder on an incomplete type would be useful (as presumably you would new an object right before which would require completeness) but it's also harmless.

Thanks! That wasn't clear to me from the bug report/commit message.

erichkeane · 2024-05-10T12:40:14Z

Should we try to force an instantiation in codegen for complete-but-not-yet-instantiated classes?

I think we have to here. I believe we can do this by changing the RequireCompleteType to pass a null diagnoser.

See this overload: https://clang.llvm.org/doxygen/classclang_1_1Sema.html#ac457a5af05f2ffc2cf6286f398d9201d (RequireCompleteType() [1/5])

MitalAshok · 2024-05-10T12:46:12Z

Here's one scenario where it could make a difference in codegen https://godbolt.org/z/dxvjzsWKr:

// Header
struct X;
extern X x;
void f(X&);
inline void g() {
    f(x);
}
inline void h() {
    f(*__builtin_launder(__builtin_addressof(x)));
}

// Source
struct X {
    virtual void mem() { __builtin_printf("X::mem()\n"); }
};
void f(X& x) {
    x.mem();
}
void(*p[2])() = { g, h };  // Force inline functions to be generated

Even though Clang currently doesn't do this optimization, with -fstrict-vtable-pointers, g() is allowed to to devirtualise the call.

Also, we can't "complete" the type at all. See https://github.com/llvm/llvm-project/pull/91070/files#diff-a8ef8d65dd30e14c0c43287a86c22b072ad29d04e85c2807602de9bcf2a26af1R160-R164, where the template is not allowed to be instantiated by launder.

Maybe we could delay the code-gen to the end of the TU to see if it's completed by then? I do think it's an incredibly rare situation for some incomplete type to be std::laundered and then later completed in the same TU.

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

MitalAshok force-pushed the incomplete-launder branch from 21d9f27 to fe8c0dc Compare May 4, 2024 16:39

[Clang] No longer require complete types with __builtin_launder

11ad517

Incomplete types are assumed to need the llvm.launder.invariant.group intrinsic Fixes llvm#90949

MitalAshok force-pushed the incomplete-launder branch from fe8c0dc to 11ad517 Compare May 4, 2024 17:44

MitalAshok requested a review from tbaederr as a code owner May 4, 2024 17:44

cor3ntin reviewed May 5, 2024

View reviewed changes

cor3ntin requested review from AaronBallman and erichkeane May 10, 2024 06:54

shafik requested a review from zygoloid May 25, 2024 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] No longer require complete types with __builtin_launder #91070

[Clang] No longer require complete types with __builtin_launder #91070

MitalAshok commented May 4, 2024

llvmbot commented May 4, 2024 •

edited

github-actions bot commented May 4, 2024 •

edited

cor3ntin left a comment

erichkeane commented May 10, 2024

cor3ntin commented May 10, 2024

erichkeane commented May 10, 2024

erichkeane commented May 10, 2024

MitalAshok commented May 10, 2024

[Clang] No longer require complete types with __builtin_launder #91070

Are you sure you want to change the base?

[Clang] No longer require complete types with __builtin_launder #91070

Conversation

MitalAshok commented May 4, 2024

llvmbot commented May 4, 2024 • edited

github-actions bot commented May 4, 2024 • edited

cor3ntin left a comment

Choose a reason for hiding this comment

erichkeane commented May 10, 2024

cor3ntin commented May 10, 2024

erichkeane commented May 10, 2024

erichkeane commented May 10, 2024

MitalAshok commented May 10, 2024

llvmbot commented May 4, 2024 •

edited

github-actions bot commented May 4, 2024 •

edited