[clang] Do not discard cleanups while processing immediate invocation

[Fznamznon]

Summary

Since an immediate invocation is a full expression itself - it requires
an additional ExprWithCleanups node, but it can participate to a bigger
full expression which actually requires cleanups to be run after.

Thanks @ilya-biryukov for helping reducing the reproducer and confirming
that the analysis is correct.

Fixes https://github.com/llvm/llvm-project/issues/60709

[Fznamznon]

Rebase

[ilya-biryukov]

clang/lib/Sema/SemaExprCXX.cpp

7374 ↗

(On Diff #535419)

Because we may potentially pass some cleanups here (which are outside ConstantExpr) and won't discard them later, we end up adding the unrelated blocks to this cleanup expression.
Here's an example. If we start with this code:

cpp
  struct P {
    consteval P() {}
  };

  struct A {
    A(int v) { this->data = new int(v); }
    const int& get() const {
      return *this->data;
    }
    ~A() { delete data; }
  private:
    int *data;
  };

  void foo() {
    A a(1);
  for (; ^{ ptr = &a.get(); }(), P(), false;);
  }

And run clang++ -std=c++20 -fblocks -Xclang=-ast-dump, we'll get:
(notice same Block in two cleanups)

`-ForStmt 0x5592888b1318 <line:17:3, col:46>
  |-<<<NULL>>>
  |-<<<NULL>>>
  |-ExprWithCleanups 0x5592888b12f0 <col:10, col:39> 'bool'
  | |-cleanup Block 0x5592888ad728
  | `-BinaryOperator 0x5592888b12d0 <col:10, col:39> 'bool' ','
  |   |-BinaryOperator 0x5592888b12a0 <col:10, col:36> 'P':'P' ','
  |   | |-CallExpr 0x5592888adb48 <col:10, col:31> 'void'
  |   | | `-BlockExpr 0x5592888adb30 <col:10, col:29> 'void (^)()'
  |   | |   `-BlockDecl 0x5592888ad728 <col:10, col:29> col:10
  |   | |     |-capture Var 0x5592888ad318 'a' 'A':'A'
  |   | |     `-CompoundStmt 0x5592888ad930 <col:11, col:29>
  |   | `-CXXFunctionalCastExpr 0x5592888b1278 <col:34, col:36> 'P':'P' functional cast to P <NoOp>
  |   |   `-ConstantExpr 0x5592888b1108 <col:34, col:36> 'P':'P'
  |   |     |-value: Struct
  |   |     `-ExprWithCleanups 0x5592888b10e8 <col:34, col:36> 'P':'P'
  |   |       |-cleanup Block 0x5592888ad728
  |   |       `-CXXTemporaryObjectExpr 0x5592888b10b8 <col:34, col:36> 'P':'P' 'void ()'
  |   `-CXXBoolLiteralExpr 0x5592888b12c0 <col:39> 'bool' false

I didn't check if this particular error can lead to codegen bug, but even having a misplaced cleanup in the AST is already a red flag.

I think in case of immediate invocations we could simply create ExprWithCleanups and not do anything else:

• CleanupVarDeclMarking will be handled when containing evaluation context is popped from the stack.
• ExprCleanupObjects may only contain blocks at this point (as it's C++ and we have assert for this). One can't use blocks inside a constant expression (Clang will produce an error) so in correct code any cleanups we see inside the current context must handled by the containing evaluation context and not this ExprWithCleanups.

[Fznamznon]

Rebase, don't use MaybeCreateExprWithCleanups

[Fznamznon]

Rebase

[Fznamznon]

clang/lib/Sema/SemaExprCXX.cpp

7374 ↗

(On Diff #535419)

I see. Thank you. I updated it to only create ExprWithCleanups.

[ilya-biryukov]

LGTM with two NITs. Thanks a lot for fixing this!

clang/lib/Sema/SemaExpr.cpp

18187

NIT: maybe be a bit more specific about the types of cleanups?

// Note that ExprWithCleanups created here must always have empty cleanup objects:
// - compound literals do not create cleanup objects in C++ and immediate invocations are C++-only.
// - blocks are not allowed inside constant expressions and compiler will issue an error if they appear there.
// Hence, in correct code any cleanup objects created inside current evaluation context must be outside
// the immediate invocation.

clang/test/SemaCXX/consteval-cleanup.cpp

2

Maybe also add a test with -fblocks that checks that only one ExprWithCleanups has the block cleanup action attached?

[Fznamznon]

Update the comment, add blocks test.

[ilya-biryukov]

LGTM once again with one NIT about the comment line placement.
Thanks!

clang/lib/Sema/SemaExpr.cpp

18198–18199

NIT: put this on a separate line.

[rsmith]

LGTM, thank you for doing this!

clang/lib/Sema/SemaExpr.cpp

18201

This argument makes sense to me, thanks for documenting why this is correct!

clang/docs/ReleaseNotes.rst

- Fixed a failing assertion when implicitly defining a function within a GNU
   statement expression that appears outside of a function block scope. The
   assertion was benign outside of asserts builds and would only fire in C.
   (`#48579 <https://github.com/llvm/llvm-project/issues/48579>`_).
 - Fixed a failing assertion when applying an attribute to an anonymous union.
   The assertion was benign outside of asserts builds and would only fire in C++.
   (`#48512 <https://github.com/llvm/llvm-project/issues/48512>`_).
 - Fixed a failing assertion when parsing incomplete destructor.
   (`#63503 <https://github.com/llvm/llvm-project/issues/63503>`_)
+- Fix missing destructor calls and therefore memory leaks in generated code
+  when an immediate invocation appears as a part of an expression that produces
+  temporaries.
+  (`#60709 <https://github.com/llvm/llvm-project/issues/60709>`_).
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/lib/Sema/SemaExpr.cpp

ExprResult Sema::CheckForImmediateInvocation(ExprResult E, FunctionDecl *Decl) {
 
   if (!E.get()->isValueDependent() &&
       ExprEvalContexts.back().InImmediateEscalatingFunctionContext &&
       !CheckConstantExpressionAndKeepResult()) {
     MarkExpressionAsImmediateEscalating(E.get());
     return E;
   }
 
-  E = MaybeCreateExprWithCleanups(E);
+  if (Cleanup.exprNeedsCleanups()) {
+    // Since an immediate invocation is a full expression itself - it requires
+    // an additional ExprWithCleanups node, but it can participate to a bigger
+    // full expression which actually requires cleanups to be run after so
+    // create ExprWithCleanups without using MaybeCreateExprWithCleanups as it
+    // may discard cleanups for outer expression too early.
+
+    // Note that ExprWithCleanups created here must always have empty cleanup
+    // objects:
+    // - compound literals do not create cleanup objects in C++ and immediate
+    // invocations are C++-only.
+    // - blocks are not allowed inside constant expressions and compiler will
+    // issue an error if they appear there. Hence, in correct code any cleanup
+    // objects created inside current evaluation context must be outside the
+    // immediate invocation.
+    E = ExprWithCleanups::Create(getASTContext(), E.get(),
+                                 Cleanup.cleanupsHaveSideEffects(), {});
+  }
 
   ConstantExpr *Res = ConstantExpr::Create(
       getASTContext(), E.get(),
       ConstantExpr::getStorageKind(Decl->getReturnType().getTypePtr(),
                                    getASTContext()),
       /*IsImmediateInvocation*/ true);
   if (Cached.hasValue())
     Res->MoveIntoResult(Cached, getASTContext());

clang/test/CodeGenCXX/consteval-cleanup.cpp

+// RUN: %clang_cc1 -std=c++20 -Wno-unused-value -triple x86_64-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+struct P {
+  consteval P() {}
+};
+
+struct A {
+  A(int v) { this->data = new int(v); }
+  ~A() { delete data; }
+private:
+  int *data;
+};
+
+void foo() {
+  for (;A(1), P(), false;);
+  // CHECK: foo
+  // CHECK: for.cond:
+  // CHECK: call void @_ZN1AC1Ei
+  // CHECK: call void @_ZN1AD1Ev
+  // CHECK: for.body
+}

clang/test/SemaCXX/consteval-cleanup.cpp

+// RUN: %clang_cc1 -fblocks -Wno-unused-value -std=c++20 -ast-dump -verify %s -ast-dump | FileCheck %s
+
+// expected-no-diagnostics
+
+struct P {
+  consteval P() {}
+};
+
+struct A {
+  A(int v) { this->data = new int(v); }
+  const int& get() const {
+    return *this->data;
+  }
+  ~A() { delete data; }
+private:
+  int *data;
+};
+
+void foo() {
+  for (;A(1), P(), false;);
+  // CHECK: foo
+  // CHECK: ExprWithCleanups
+  // CHECK-NEXT: BinaryOperator {{.*}} 'bool' ','
+  // CHECK-NEXT: BinaryOperator {{.*}} 'P':'P' ','
+  // CHECK-NEXT: CXXFunctionalCastExpr {{.*}} 'A':'A'
+  // CHECK-NEXT: CXXBindTemporaryExpr {{.*}} 'A':'A'
+  // CHECK-NEXT: CXXConstructExpr {{.*}} 'A':'A'
+  // CHECK: ConstantExpr {{.*}} 'P':'P'
+  // CHECK-NEXT: value:
+  // CHECK-NEXT: ExprWithCleanups
+}
+
+void foobar() {
+  A a(1);
+  for (; ^{ auto ptr = &a.get(); }(), P(), false;);
+  // CHECK: ExprWithCleanups
+  // CHECK-NEXT: cleanup Block
+  // CHECK-NEXT: BinaryOperator {{.*}} 'bool' ','
+  // CHECK-NEXT: BinaryOperator {{.*}} 'P':'P' ','
+  // CHECK-NEXT: CallExpr
+  // CHECK-NEXT: BlockExpr
+  // CHECK: ConstantExpr {{.*}} 'P':'P'
+  // CHECK-NEXT: value:
+  // CHECK-NEXT: ExprWithCleanups
+  // CHECK-NOT:  cleanup Block
+}
+
+struct B {
+  int *p = new int(38);
+  consteval int get() { return *p; }
+  constexpr ~B() { delete p; }
+};
+
+void bar() {
+  // CHECK: bar
+  // CHECK: ExprWithCleanups
+  // CHECK: ConstantExpr
+  // CHECK-NEXT: value:
+  // CHECK-NEXT: ExprWithCleanups
+  int k = B().get();
+}