[clang] Do not create ExprWithCleanups while checking immediate invocation

[Fznamznon]

Summary

Immediate invocations do not need to run cleanups themselves and the
destructors need to run after each full-expression is processed.
Attempting to add ExprWithCleanups for each immediate invocation may cause
adding it in the wrong place and producing code with memory leaks.

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

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

[shafik]

clang/lib/Sema/SemaExpr.cpp

18177

Drive by nit clean-up while we are here.

clang/test/SemaCXX/consteval-cleanup.cpp

20

I am not too sure on this but perhaps showing P : P and A : A in the checks would be helpful

[Fznamznon]

Rebase, fix nit, expand the test

[ilya-biryukov]

As mentioned in the GH issue, I think this change looks fine.
But I would suggest waiting for feedback from @rsmith to ensure there isn't a reason for cleanups being removed that we are missing.

[rsmith]

A constant expression (including the immediate invocations generated for consteval functions) is a full-expression, so destructors should be run at the end of evaluating it, not as part of the enclosing expression. That's presumably why the code is calling MaybeCreateExprWithCleanups -- to associate cleanups for the immediate invocation with that ConstantExpr rather than with the outer context. I think the problem is that we don't have an ExpressionEvaluationContext wrapping the immediate invocation, so we don't save and restore the enclosing cleanup state -- and we can't really wrap it, because we don't find out that we're building an immediate invocation until after we've already done so. Probably the best way to handle that is to create the inner ExprWithCleanups for the constant expression, but leave the cleanups flags alone so that we also create an outer ExprWithCleanups. That'll mean we sometimes create an ExprWithCleanups that doesn't actually run any cleanups, but that's OK, just redundant. One thing we will need to be careful about is assigning any block / compound literal cleanups to the right ExprWithCleanups node. We can figure out where to put them by doing a traversal of the ConstantExpr's subexpression to see if it contains the BlockDecl / CompoundLiteralExpr being referenced by the cleanup.

[ilya-biryukov]

In D153294#4435767, @rsmith wrote:

A constant expression (including the immediate invocations generated for consteval functions) is a full-expression, so destructors should be run at the end of evaluating it, not as part of the enclosing expression. That's presumably why the code is calling MaybeCreateExprWithCleanups -- to associate cleanups for the immediate invocation with that ConstantExpr rather than with the outer context. I think the problem is that we don't have an ExpressionEvaluationContext wrapping the immediate invocation, so we don't save and restore the enclosing cleanup state -- and we can't really wrap it, because we don't find out that we're building an immediate invocation until after we've already done so. Probably the best way to handle that is to create the inner ExprWithCleanups for the constant expression, but leave the cleanups flags alone so that we also create an outer ExprWithCleanups. That'll mean we sometimes create an ExprWithCleanups that doesn't actually run any cleanups, but that's OK, just redundant. One thing we will need to be careful about is assigning any block / compound literal cleanups to the right ExprWithCleanups node. We can figure out where to put them by doing a traversal of the ConstantExpr's subexpression to see if it contains the BlockDecl / CompoundLiteralExpr being referenced by the cleanup.

Thanks! We don't have any tests that break with this change, so I assume this is not tested.
What would an example of ConstantExpression that has to run cleanups? I'm trying to come up with one, but since all types used inside immediate invocation must be literal types, whenever I try to plug a non-trivial destructor the code gets rejected.

[Fznamznon]

Thanks! We don't have any tests that break with this change, so I assume this is not tested.

I suppose it could also be that in most cases we have enough places in Sema adding ExprWithCleanups. While trying to figure out why we need ExprWithCleanups wrapping immediate invocations I came to https://reviews.llvm.org/D63960 . There is https://reviews.llvm.org/D63960#inline-613559 asking to add wrapping that with an example, I added this example to the test and I suppose it didn't break after change since there is still an ExprWithCleanups around, I suppose it was added in another place.
In fact, I can't come up with the test that this patch would break. Either ExprWithCleanups is redundant or added by different parts of Sema.

[ilya-biryukov]

In D153294#4437381, @Fznamznon wrote:

In fact, I can't come up with the test that this patch would break. Either ExprWithCleanups is redundant or added by different parts of Sema.

Same here, maybe @rsmith can come up with something that breaks.
The AST does seem off for this example:

struct A {
    int *p = new int(42);
    consteval int get() const {
        return *p;
    }
    constexpr ~A() {
        delete p;
    }
};

int k = A().get() + 1;

With the patch we get ExprWithCleanups under VarDecl:

`-VarDecl 0x56375b44f7b0 <line:12:1, col:21> col:5 k 'int' cinit
  `-ExprWithCleanups 0x56375b44fef0 <col:9, col:21> 'int'
    `-BinaryOperator 0x56375b44fed0 <col:9, col:21> 'int' '+'
      |-ConstantExpr 0x56375b44fe90 <col:9, col:17> 'int'
      | `-CXXMemberCallExpr 0x56375b44fe58 <col:9, col:17> 'int'
      |   `-MemberExpr 0x56375b44fe28 <col:9, col:13> '<bound member function type>' .get 0x56375b42f028
      |     `-ImplicitCastExpr 0x56375b44fe78 <col:9, col:11> 'const A' xvalue <NoOp>
      |       `-MaterializeTemporaryExpr 0x56375b44fe10 <col:9, col:11> 'A':'A' xvalue
      |         `-CXXBindTemporaryExpr 0x56375b44fdf0 <col:9, col:11> 'A':'A' (CXXTemporary 0x56375b44fdf0)
      |           `-CXXTemporaryObjectExpr 0x56375b44fdb8 <col:9, col:11> 'A':'A' 'void () noexcept(false)' zeroing
      `-IntegerLiteral 0x56375b44feb0 <col:21> 'int' 1

But per Richard's comments ConstantExpr is a full expression and the cleanups should be under it instead (see godbolt for what's happening today).
From playing around with it, I couldn't find anything where this affects either codegen or diagnostics.

I think that's because any temporaries produced must have constexpr or trivial destructors, so they don't produce any code. Additionally, there seems to be no way for those temporaries to escape the immediate invocation.

I would guess we are still not comfortable with moving ExprWithCleanups outside ConstantExpr, but let's wait for Richard's comments.

[ilya-biryukov]

While we wait for Richard's response... @Fznamznon what are your thoughts on wrapping all ConstantExpr that span immediate invocations with redundant ExprWithCleanups per Richard's suggestions?
I would be comfortable LGTMing such a change even if we choose to remove some of this redundancy later in a follow-ups.

This bug looks scary enough to me to warrant a fix asap.

[Fznamznon]

While we wait for Richard's response... @Fznamznon what are your thoughts on wrapping all ConstantExpr that span immediate invocations with redundant ExprWithCleanups per Richard's suggestions?
This bug looks scary enough to me to warrant a fix asap.

Agree. I was thinking of implementing Richard's suggestions in a separate review for some time. I'll do that.

[ilya-biryukov]

In D153294#4444065, @Fznamznon wrote:

While we wait for Richard's response... @Fznamznon what are your thoughts on wrapping all ConstantExpr that span immediate invocations with redundant ExprWithCleanups per Richard's suggestions?
This bug looks scary enough to me to warrant a fix asap.

Agree. I was thinking of implementing Richard's suggestions in a separate review for some time. I'll do that.

Many thanks!

[Fznamznon]

One thing we will need to be careful about is assigning any block / compound literal cleanups to the right ExprWithCleanups node. We can figure out where to put them by doing a traversal of the ConstantExpr's subexpression to see if it contains the BlockDecl / CompoundLiteralExpr being referenced by the cleanup

I'm having a slight trouble with understanding why this part is required and how to implement the test that would use this part. I've got a feeling that it will be adding a bit of a dead code because BlockDecl seems to be ObjC-only construct, so I don't think I can get it inside of an immediate invocation. Compound literals in C++ are never referenced in ExprCleanupObjects which clang normally uses to attach BlockDecl / CompoundLiteralExpr to an ExprWithCleanups, according to https://github.com/llvm/llvm-project/blob/00b12a94322dc7779f42d7f863957f8ea4304dad/clang/lib/Sema/SemaExpr.cpp#L7731 .
@rsmith could you please suggest the test case where it is required?

Also, simply adding a flag to MaybeCreateExprWithCleanup signaling that we're processing an immediate invocation to not discard cleanups also passes check-clang and fixes original memory leak problem. Maybe we should just do that?

[ilya-biryukov]

In D153294#4448342, @Fznamznon wrote:

I'm having a slight trouble with understanding why this part is required and how to implement the test ...

Also, simply adding a flag to MaybeCreateExprWithCleanup signaling that we're processing an immediate invocation to not discard cleanups also passes check-clang and fixes original memory leak problem. Maybe we should just do that?

I also struggle to see how any of the cleanup objects may end up being inside ExprWithCleanups under immediate invocation.
Any use of block literals inside constant expressions results in an error and compound literals are handled like temporaries in C++ and don't create cleanup objects (see the reference above).

I would maybe guard the code doing the processing immediate invocations with assert(LangOpts.CPlusPlus) to sanity-check that we don't end up calling this in C in some very distant future.
I believe we should make the proposed change (add potentially redundant ExprWithCleanups without touching the cleanups objects or flags) without blocking on Richard's approval.
It definitely fixes the somewhat common problem we have now and potentially leaves behind a few corner cases with blocks which are much less likely (or even impossible) to occur in practice.

[Fznamznon]

I believe we should make the proposed change (add potentially redundant ExprWithCleanups without touching the cleanups objects or flags) without blocking on Richard's approval.

I posted https://reviews.llvm.org/D153962 .

clang/docs/ReleaseNotes.rst

- CallExpr built for C error-recovery now is always type-dependent. Fixes a
   (`#50244 <https://github.com/llvm/llvm-project/issues/50244>_`).
 - Apply ``-fmacro-prefix-map`` to anonymous tags in template arguments
   (`#63219 <https://github.com/llvm/llvm-project/issues/63219>`_).
 - Clang now properly diagnoses format string mismatches involving scoped
   enumeration types. A scoped enumeration type is not promoted to an integer
   type by the default argument promotions, and thus this is UB. Clang's
   behavior now matches GCC's behavior in C++.
   (`#38717 <https://github.com/llvm/llvm-project/issues/38717>_`).
+- 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);
-
   ConstantExpr *Res = ConstantExpr::Create(
       getASTContext(), E.get(),
       ConstantExpr::getStorageKind(Decl->getReturnType().getTypePtr(),
                                    getASTContext()),
-      /*IsImmediateInvocation*/ true);
+      /*IsImmediateInvocation=*/true);
   if (Cached.hasValue())
     Res->MoveIntoResult(Cached, getASTContext());
   /// Value-dependent constant expressions should not be immediately
   /// evaluated until they are instantiated.
   if (!Res->isValueDependent())
     ExprEvalContexts.back().ImmediateInvocationCandidates.emplace_back(Res, 0);
   return Res;
 }

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 -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); }
+  ~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-NOT: ExprWithCleanups
+}
+
+struct B {
+  int *p = new int(38);
+  consteval int get() { return *p; }
+  constexpr ~B() { delete p; }
+};
+
+void bar() {
+  // CHECK: bar
+  // CHECK: ExprWithCleanups
+  // CHECK-NEXT: ConstantExpr
+  int k = B().get();
+}