[Clang] [CodeGen] Perform derived-to-base conversion on explicit object parameter in lambda

[Sirraide]

Consider this code:

template <typename... Ts>
struct Overloaded : Ts... { using Ts::operator()...; };

template <typename... Ts>
Overloaded(Ts...) -> Overloaded<Ts...>;

void f() {
  int x;
  Overloaded o {
    [&](this auto& self) {
      return &x;
    }
  };
  o();
}

To access x in the lambda, we need to perform derived-to-base conversion on self (since the type of self is not the lambda type, but rather Overloaded<(lambda type)>). We were previously missing this step, causing us to attempt to load the entire lambda (as the base class, it would end up being the ‘field’ with index 0 here), which would then assert later on in codegen.

This fixes #87210 and fixes #89541.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: None (Sirraide)

Changes

Consider this code:

template <typename... Ts>
struct Overloaded : Ts... { using Ts::operator()...; };

template <typename... Ts>
Overloaded(Ts...) -> Overloaded<Ts...>;

void f() {
  int x;
  Overloaded o {
    [&](this auto& self) {
      return &x;
    }
  };
  o();
}

To access x in the lambda, we need to perform derived-to-base conversion on self (since the type of self is not the lambda type, but rather Overloaded<(lambda type)>). We were previously missing this step, causing us to attempt to load the entire lambda (as the base class, it would end up being the ‘field’ with index 0 here), which would then assert later on in codegen.

This fixes #87210 and fixes #89541.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+3)
• (modified) clang/lib/CodeGen/CGExpr.cpp (+23)
• (modified) clang/test/CodeGenCXX/cxx2b-deducing-this.cpp (+63)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index d1f7293a842bb6..34aad4abf39619 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -562,6 +562,9 @@ Bug Fixes to C++ Support
 - Fixed a crash when trying to evaluate a user-defined ``static_assert`` message whose ``size()``
   function returns a large or negative value. Fixes (#GH89407).
 - Fixed a use-after-free bug in parsing of type constraints with default arguments that involve lambdas. (#GH67235)
+- Fixed a crash when trying to emit captures in a lambda call operator with an explicit object
+  parameter that is called on a derived type of the lambda.
+  Fixes (#GH87210), (GH89541).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 931cb391342ea2..33795d7d4d1921 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -4684,6 +4684,29 @@ LValue CodeGenFunction::EmitLValueForLambdaField(const FieldDecl *Field,
     else
       LambdaLV = MakeAddrLValue(AddrOfExplicitObject,
                                 D->getType().getNonReferenceType());
+
+    // Make sure we have an lvalue to the lambda itself and not a derived class.
+    auto *ThisTy = D->getType().getNonReferenceType()->getAsCXXRecordDecl();
+    auto *LambdaTy = cast<CXXRecordDecl>(Field->getParent());
+    if (ThisTy != LambdaTy) {
+      CXXBasePaths Paths(/*FindAmbiguities=*/false, /*RecordPaths=*/true,
+                         /*DetectVirtual=*/false);
+
+      [[maybe_unused]] bool Derived = ThisTy->isDerivedFrom(LambdaTy, Paths);
+      assert(Derived && "Type not derived from lambda type?");
+
+      const CXXBasePath *Path = &Paths.front();
+      CXXCastPath BasePathArray;
+      for (unsigned I = 0, E = Path->size(); I != E; ++I)
+        BasePathArray.push_back(
+            const_cast<CXXBaseSpecifier *>((*Path)[I].Base));
+
+      Address Base = GetAddressOfBaseClass(
+          LambdaLV.getAddress(*this), ThisTy, BasePathArray.begin(),
+          BasePathArray.end(), /*NullCheckValue=*/false, SourceLocation());
+
+      LambdaLV = MakeAddrLValue(Base, QualType{LambdaTy->getTypeForDecl(), 0});
+    }
   } else {
     QualType LambdaTagType = getContext().getTagDeclType(Field->getParent());
     LambdaLV = MakeNaturalAlignAddrLValue(ThisValue, LambdaTagType);
diff --git a/clang/test/CodeGenCXX/cxx2b-deducing-this.cpp b/clang/test/CodeGenCXX/cxx2b-deducing-this.cpp
index b755e80db35a12..649fe2afbf4e91 100644
--- a/clang/test/CodeGenCXX/cxx2b-deducing-this.cpp
+++ b/clang/test/CodeGenCXX/cxx2b-deducing-this.cpp
@@ -182,3 +182,66 @@ auto dothing(int num)
   fun();
 }
 }
+
+namespace GH87210 {
+template <typename... Ts>
+struct Overloaded : Ts... {
+  using Ts::operator()...;
+};
+
+template <typename... Ts>
+Overloaded(Ts...) -> Overloaded<Ts...>;
+
+// CHECK-LABEL: define dso_local void @_ZN7GH872101fEv()
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[X:%.*]] = alloca i32
+// CHECK-NEXT:    [[Over:%.*]] = alloca %"{{.*}}Overloaded"
+// CHECK:         call noundef ptr @"_ZZN7GH872101fEvENH3$_0clINS_10OverloadedIJS0_EEEEEDaRT_"(ptr {{.*}} [[Over]])
+void f() {
+  int x;
+  Overloaded o {
+    // CHECK: define internal noundef ptr @"_ZZN7GH872101fEvENH3$_0clINS_10OverloadedIJS0_EEEEEDaRT_"(ptr {{.*}} [[Self:%.*]])
+    // CHECK-NEXT:  entry:
+    // CHECK-NEXT:    [[SelfAddr:%.*]] = alloca ptr
+    // CHECK-NEXT:    store ptr [[Self]], ptr [[SelfAddr]]
+    // CHECK-NEXT:    [[SelfPtr:%.*]] = load ptr, ptr [[SelfAddr]]
+    // CHECK-NEXT:    [[XRef:%.*]] = getelementptr inbounds %{{.*}}, ptr [[SelfPtr]], i32 0, i32 0
+    // CHECK-NEXT:    [[X:%.*]] = load ptr, ptr [[XRef]]
+    // CHECK-NEXT:    ret ptr [[X]]
+    [&](this auto& self) {
+      return &x;
+    }
+  };
+  o();
+}
+
+void g() {
+  int x;
+  Overloaded o {
+    [=](this auto& self) {
+      return x;
+    }
+  };
+  o();
+}
+}
+
+namespace GH89541 {
+// Same as above; just check that this doesn't crash.
+int one = 1;
+auto factory(int& x = one) {
+  return [&](this auto self) {
+    x;
+  };
+};
+
+using Base = decltype(factory());
+struct Derived : Base {
+  Derived() : Base(factory()) {}
+};
+
+void f() {
+  Derived d;
+  d();
+}
+}

[Sirraide]

Ok, I’ve added a check for CWG 2881 to sema, but there’s at least 2 things I still want to take a look at:

• Consider emitting the upcast once in the function prologue instead of every time a variable is accessed.
  (We’re currently doing the latter, but based on feedback, it seems fine to just let the optimiser clean this up, and it’s simpler to do it this way)
• Check for invalid explicit object parameters when the lambda is instantiated rather than when it is called. (see below)

[Sirraide]

I’m also not sure if the dr28xx.cpp file is meant for open issues, but I’ve put the tests for this there for now.

[Endilll]

@Sirraide See example of

llvm-project/clang/test/CXX/drs/dr25xx.cpp

Line 148 in 662ef86

namespace cwg2565 { // cwg2565: 16 open 2023-06-07

In this case, the date should be 2024-04-19.

[Sirraide]

@Sirraide See example of

llvm-project/clang/test/CXX/drs/dr25xx.cpp

Line 148 in 662ef86

namespace cwg2565 { // cwg2565: 16 open 2023-06-07

In this case, the date should be 2024-04-19.

Ah, thanks.

[Sirraide]

@cor3ntin There already was a FIXME to consider moving this check to where the call operator is first instantiated; should I look into that as well or is it fine to leave this here? Asking because I’m assuming there’s a reason why it wasn’t done that way.

[cor3ntin]

@cor3ntin There already was a FIXME to consider moving this check to where the call operator is first instantiated; should I look into that as well or is it fine to leave this here? Asking because I’m assuming there’s a reason why it wasn’t done that way.

If you can do it easily, you should!
I don't remember the details sadly, It's been over a year!

[Sirraide]

Ok, I’ve looked into this a bit more, and it seems that checking for this at instantiation time ends up being more complicated and produces more diagnostics than just checking it at the call site; I’ve added a comment about that.

Other than that, is there anything else that we should look into here?

[cor3ntin]

You should run make_cxx_drs, otherwise I think this looks in good shape.

I think we should diag earlier (because if the lambda is not use we should still signal it's broken) but doing that as a follow up PR seems reasonable.

Less redundant diags is better but if we can't avoid it, i think it's a reasonable compromise

[Sirraide]

You should run make_cxx_drs, otherwise I think this looks in good shape.

Will do.

I think we should diag earlier (because if the lambda is not use we should still signal it's broken) but doing that as a follow up PR seems reasonable.

Less redundant diags is better but if we can't avoid it, i think it's a reasonable compromise

Alright; I’ll leave it as-is for this pr then.

[cor3ntin]

I think this makes sense. Thanks!

@Endilll just updated the list of DRs to avoid unrelated changes, so by fixing the conflict, only changes to cwg2881 should be in your PR

[Sirraide]

Endilll just updated the list of DRs to avoid unrelated changes, so by fixing the conflict, only changes to cwg2881 should be in your PR

Should I just run make_cxx_dr_status again then and commit the result?

[Sirraide]

Should I just run make_cxx_dr_status again then and commit the result?

I ended up figuring it out after talking to @Endilll about it.

Also, the only CI failure earlier was something unrelated (CoverageMapping/mcdc-system-headers.cpp).

[Sirraide]

It seems that a parameter to getAddress() that wasn’t being used has been removed (#92465), which caused builds to break after I merged this. My bad; I’ve already pushed a fix for this: #93086. I didn’t wait for CI this time because it was passing before (except for an unrelated issue), and I figured it wouldn’t be a problem because that was 2 days ago (it usually isn’t a problem in my experience at least), but the timing here was a bit unfortunate this time...