[Sema] Push a LambdaScopeInfo before calling SubstDefaultArgument

[ahatanak]

Summary

This is needed to fix https://github.com/llvm/llvm-project/issues/60155.

An assertion in tryCaptureVariable fails because SubstDefaultArgument pushes the context for the lambda's function operator, but the LambdaScopeInfo isn't being pushed to the stack.

It appears that the assertion started to fail in 4409a83c293537e22da046b54e9f69454cdd3dca, which delays instantiation of default arguments. By the time the default arguments are instantiated, which happens after inherited::TransformLambdaExpr is called, the lambda scope has already been popped.

[erichkeane]

@cor3ntin : Mind taking a look here? You're my lambda expert these days :)

[cor3ntin]

In D143109#4097775, @erichkeane wrote:

@cor3ntin : Mind taking a look here? You're my lambda expert these days :)

I did something similar to check the requires clause of a lambda and it does technically fix the issue, i don't see a more elegant solution, but it does sound a bit wrong... (it allocates a dummy function scope that is then kept around forever)
Maybe we can add a fixme comment here and add a "remove lambda scope" with an raii object for these two cases in a separate PR?

The issue is not surprising though, there are weird order of operations in how parameters and functions and their context are handled.

[ahatanak]

Doesn't Sema::FunctionScopeRAII pop the lambda scope when it goes out of scope?

https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/Sema.h#L5042

I also tried instantiating the default arguments after the call to BuildLambdaExpr using LSICopy (see https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/TreeTransform.h#L13477), which is the copy of the lambda scope, but I opted for the approach in this patch as using a dummy scope seemed sufficient.

[cor3ntin]

In D143109#4097850, @ahatanak wrote:

Doesn't Sema::FunctionScopeRAII pop the lambda scope when it goes out of scope?

No, sadly lambdas are handled differently

https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/Sema.h#L5042

I also tried instantiating the default arguments after the call to BuildLambdaExpr using LSICopy (see https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/TreeTransform.h#L13477), which is the copy of the lambda scope, but I opted for the approach in this patch as using a dummy scope seemed sufficient.

I think the patch is good enough too, just questioning whether we want to add a "FIXME" comment for later (It would be good to try to clean up all of that, one day - if someone has time)

[ahatanak]

I'm still don't understand what the problem is about cleaning up the lambda scope.

PushLambdaScope creates a new LambdaScopeInfo and pushes onto FunctionScopes.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/Sema.cpp#L2137

When Sema::FunctionScopeRAII goes out of scope, its destructor calls PopFunctionScopeInfo.
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/Sema.h#L5048

PopFunctionScopeInfo then pops the FunctionScopeInfo at the top of the stack, which is the LambdaScopeInfo that was pushed earlier, and PoppedFunctionScopePtr makes sure the popped scope is deleted.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/Sema.cpp#L2243

[tahonermann]

I'm still don't understand what the problem is about cleaning up the lambda scope.

Me either. It looks to me like the pushed lambda scope will get properly cleaned up.

Incidentally, it looks like that LSI copy (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/TreeTransform.h#L13469-L13478) has not been needed since commit bf5fe2dbba0899bee4323f5eaa075acc43a18e2e (see https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L15435-L15438).

clang/lib/Sema/SemaTemplateInstantiate.cpp

1352–1356

If I'm understanding the issue correctly, the problem is that the lambda scope was created within the call to inherited::TransformLambdaExpr(E) above, but then popped upon return (and the copy created to outlive the call to Sema::ActOnFinishFunctionBody() destructed). A better solution then would be to modify TreeTransform::TransformLambdaExpr() to include this loop and dispatch transformation of the default argument via getDerived().transformLambdaDefaultArgument() (or similar). That seems like a straight forward refactor that might be useful elsewhere.

clang/test/SemaCXX/lambda-default-arg.cpp

19

Another example that I think is worth adding:

template <class T> bool test2(float a = 1e-5f) {
  return [=](int b = sizeof(a)) -> bool {
    return b;
  }();
}

bool b2 = test2<int>();

[tahonermann]

Incidentally, it looks like that LSI copy (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/TreeTransform.h#L13469-L13478) has not been needed since commit bf5fe2dbba0899bee4323f5eaa075acc43a18e2e (see https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L15435-L15438).

I tried this and the testsuite informed me, in no uncertain terms that, no no, that copy is very much still needed. I haven't explored further.

[cor3ntin]

My question is, why do we need to mess up with scopes in that way outside of parsing (there are only a couple places where we do that at the moment, and they are dummy scopes which only exist to balance some push and pop, afaict they serve no other purpose).
As i said, I have no objection to this at all, it clearly fixes the issue and we should land it! But the fact we have to do this in the first place *may* be a sign that there is a deeper issue, one that we clearly should not try to address as part of this.

[ahatanak]

Instead of pushing an empty lambda scope, switch to the enclosing context if the variable is used in a default argument expression of a lambda call operator.

[ahatanak]

Fix indentation.

[ahatanak]

I agree that pushing an empty scope without initializing any of its members seems wrong.

Instead of doing that, the updated patch changes the decl context instead if we are trying to capture a variable in a default argument expression of a lambda call operator. I think that's okay since the type of the DeclRefExpr of the variable appearing in the default argument expression shouldn't depend on whether it's captured by the lambda, if I understand the standard rules correctly (note that when clang crashes, tryCaptureVariable is being called by getCapturedDeclRefType).

[tahonermann]

Thanks for the update, @ahatanak. Please note that myself and Corentin are both preoccupied with the WG21 meeting this week, so it might be a while before either of us is able to take a good look at your update.

[ahatanak]

@tahonermann @cor3ntin have you had a chance to take a look at the updated patch?

[tahonermann]

clang/lib/Sema/SemaExpr.cpp

19085–19092 ↗

(On Diff #495619)

I'm struggling to understand the change. If I'm following correctly, it looks like we're trying to capture the variable used in the default argument. If so, that seems wrong; I think we shouldn't even be trying to capture a variable for such usage.

I jumped into a debugger to poke around a bit. Perhaps the right change is to detect the use in a default argument in the code below so that the call to getCapturedDeclRefType() can be skipped for a non-ODR use.

/localdisk2/thonerma/llvm-project/clang/lib/Sema/SemaExpr.cpp:
 3265 ExprResult Sema::BuildDeclarationNameExpr(
 3266     const CXXScopeSpec &SS, const DeclarationNameInfo &NameInfo, NamedDecl *D,
 3267     NamedDecl *FoundD, const TemplateArgumentListInfo *TemplateArgs,
 3268     bool AcceptInvalidDecl) {
.....
 3326   switch (D->getKind()) {
.....
 3389   case Decl::Var:
 3390   case Decl::VarTemplateSpecialization:
 3391   case Decl::VarTemplatePartialSpecialization:
 3392   case Decl::Decomposition:
 3393   case Decl::OMPCapturedExpr:
 3394     // In C, "extern void blah;" is valid and is an r-value.
 3395     if (!getLangOpts().CPlusPlus && !type.hasQualifiers() &&
 3396         type->isVoidType()) {
 3397       valueKind = VK_PRValue;
 3398       break;
 3399     }
 3400     [[fallthrough]];
 3401
 3402   case Decl::ImplicitParam:
 3403   case Decl::ParmVar: {
 3404     // These are always l-values.
 3405     valueKind = VK_LValue;
 3406     type = type.getNonReferenceType();
 3407
 3408     // FIXME: Does the addition of const really only apply in
 3409     // potentially-evaluated contexts? Since the variable isn't actually
 3410     // captured in an unevaluated context, it seems that the answer is no.
 3411     if (!isUnevaluatedContext()) {
 3412       QualType CapturedType = getCapturedDeclRefType(cast<VarDecl>(VD), Loc);
 3413       if (!CapturedType.isNull())
 3414         type = CapturedType;
 3415     }
 3416
 3417     break;
 3418   }
.....
 3516 }

clang/test/SemaCXX/lambda-default-arg.cpp

7

Is the use of a in the lambda body intentional? It isn't needed to reproduce the assertion failure, so its presence here is a bit confusing from a test perspective as it implies that variable capture is somehow related. I suggest just returning true. Likewise for the other two tests.

[ahatanak]

It looks like a7579b25df78a9f53d62300020d4ae3c47666634 fixed the crash. Thank you!

clang/lib/Sema/SemaTemplateInstantiate.cpp

ExprResult TransformLambdaExpr(LambdaExpr *E) {
 
       CXXMethodDecl *MD = Result.getAs<LambdaExpr>()->getCallOperator();
       for (ParmVarDecl *PVD : MD->parameters()) {
         if (!PVD->hasDefaultArg())
           continue;
         Expr *UninstExpr = PVD->getUninstantiatedDefaultArg();
         // FIXME: Obtain the source location for the '=' token.
         SourceLocation EqualLoc = UninstExpr->getBeginLoc();
+
+        // Push an empty lambda scope. This is needed as SubstDefaultArgument
+        // pushes the context for the lambda's function call operator and we
+        // need to keep the contexts and the function scopes in sync.
+        getSema().PushLambdaScope();
+        Sema::FunctionScopeRAII FuncScopeCleanup(getSema());
+
         if (SemaRef.SubstDefaultArgument(EqualLoc, PVD, TemplateArgs)) {
           // If substitution fails, the default argument is set to a
           // RecoveryExpr that wraps the uninstantiated default argument so
           // that downstream diagnostics are omitted.
           ExprResult ErrorResult = SemaRef.CreateRecoveryExpr(
               UninstExpr->getBeginLoc(), UninstExpr->getEndLoc(),
               { UninstExpr }, UninstExpr->getType());
           if (ErrorResult.isUsable())

clang/test/SemaCXX/lambda-default-arg.cpp

+// RUN: %clang_cc1 -std=c++17 -verify -fsyntax-only %s
+
+template <class T> bool test0() {
+  constexpr float a = 1e-5f;
+  return [=](float b = a) -> bool {
+    return a < 0;
+  }();
+}
+
+bool b0 = test0<int>();
+
+template <class T> bool test1() {
+  float a = 1e-5f;
+  return [=](float b = a) -> bool { // expected-error {{default argument references local variable 'a'}}
+    return a < 0;
+  }();
+}
+
+bool b1 = test1<int>();