[C++20] [Modules] Pop Expression Evaluation Context when we skip its body during parsing

[ChuanqiXu]

Summary

Close https://github.com/llvm/llvm-project/issues/60275

The root cause of issue 60275 is the imbalance of PushExpressionEvaluationContext() and PopExpressionEvaluationContext().

See https://github.com/llvm/llvm-project/blob/f1c4f927f7c15b5efdc3589c050fd0513bf6b303/clang/lib/Parse/Parser.cpp#L1396-L1437

We will PushExpressionEvaluationContext() in ActOnStartOfFunctionDef() in line 1396 and we should pop it in ActOnFinishFunctionBody later. However if we skip the function body in line 1402, the expression evaluation context will not be popped. Then here is the issue report. I fix the issue by inserting codes to pop the expression evaluation context explicitly if the function body is skipped. Maybe this looks like an ad-hoc fix. But if we want to fix this in a pretty way, we should refactor the current framework for pushing and popping expression evaluation contexts. Currently there are 23 PushExpressionEvaluationContext() callsities and 21 PopExpressionEvaluationContext() callsites in the code. And it seems not easy to balance them well and fast. So I suggest to land this fix first. At least it can prevent the crash.

[erichkeane]

@cor3ntin did a bunch of work here lately, so he's probably a better reviewer than I am.

[shafik]

clang/lib/Parse/Parser.cpp

1415

It does seem a bit ad-hoc

[cor3ntin]

clang/lib/Parse/Parser.cpp

1415

could we use ExitFunctionBodyRAII here, maybe ? It already deals with lambdas

[ChuanqiXu]

clang/lib/Parse/Parser.cpp

1415

It does seem a bit ad-hoc

I agree. I feel the current explicit style for pushing and popping different contexts looks not easy to maintain... I just want to prevent the crash for now and I feel the patch wouldn't be a burden when someday we want to refactor this.

could we use ExitFunctionBodyRAII here, maybe ? It already deals with lambdas

Maybe. But it wouldn't be simpler. We need to move ExitFunctionBodyRAII and we need to write:

ExitFunctionBodyRAII ExitRAII(Actions, isLambdaCallOperator(dyn_cast_if_present<FunctionDecl>(Res)));
return Res;

which looks odd and unnecessary.

And if you're saying something like:

Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D,
                                              TemplateInfo.TemplateParams
                                                  ? *TemplateInfo.TemplateParams
                                                  : MultiTemplateParamsArg(),
                                              &SkipBody, BodyKind);


ExitFunctionBodyRAII ExitRAII(Actions, isLambdaCallOperator(dyn_cast_if_present<FunctionDecl>(Res)));

and remove the ExitFunctionBodyRAII in ActOnFinishFunctionBody. I guess it can't work in this way simply. Since the comment (https://github.com/llvm/llvm-project/blob/c6795b1d37cee586d9b98dade64432f8f6bd004b/clang/lib/Sema/SemaDecl.cpp#L15811-L15818) says we need to pop expression evaluation context before popping decl context (and function scope info?).

[cor3ntin]

clang/lib/Parse/Parser.cpp

1415

I think you are right, your change is probably as good as anything without doing a major refactor.

clang/lib/Parse/Parser.cpp

 //===--- Parser.cpp - C Language Family Parser ----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file implements the Parser interfaces.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Parse/Parser.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ASTLambda.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
 #include "llvm/Support/Path.h"
 using namespace clang;
Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D,
                                                   : MultiTemplateParamsArg(),
                                               &SkipBody, BodyKind);
 
   if (SkipBody.ShouldSkip) {
     // Do NOT enter SkipFunctionBody if we already consumed the tokens.
     if (BodyKind == Sema::FnBodyKind::Other)
       SkipFunctionBody();
 
+    // ExpressionEvaluationContext is pushed in ActOnStartOfFunctionDef
+    // and it would be popped in ActOnFinishFunctionBody.
+    // We pop it explcitly here since ActOnFinishFunctionBody won't get called.
+    //
+    // Do not call PopExpressionEvaluationContext() if it is a lambda because
+    // one is already popped when finishing the lambda in BuildLambdaExpr().
+    //
+    // FIXME: It looks not easy to balance PushExpressionEvaluationContext()
+    // and PopExpressionEvaluationContext().
+    if (!isLambdaCallOperator(dyn_cast_if_present<FunctionDecl>(Res)))
+      Actions.PopExpressionEvaluationContext();
     return Res;
   }
 
   // Break out of the ParsingDeclarator context before we parse the body.
   D.complete(Res);
 
   // Break out of the ParsingDeclSpec context, too.  This const_cast is
   // safe because we're always the sole owner.

clang/test/Modules/pr60275.cppm

+// Address: https://github.com/llvm/llvm-project/issues/60275
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/b.cpp -fmodule-file=%t/a.pcm -emit-llvm -o - | FileCheck %t/b.cpp
+//--- foo.h
+
+consteval void global() {}
+
+//--- a.cppm
+module;
+#include "foo.h"
+export module a;
+
+//--- b.cpp
+#include "foo.h"
+import a;
+
+consteval int b() {
+  return 0;
+}
+
+struct bb {
+  int m = b();
+};
+
+void bbb() {
+  bb x;
+}
+
+// CHECK: define{{.*}}_ZN2bbC2Ev({{.*}}[[THIS:%.+]])
+// CHECK-NEXT: entry:
+// CHECK-NEXT:   [[THIS_ADDR:%.*]] = alloca ptr
+// CHECK-NEXT:   store ptr [[THIS]], ptr [[THIS_ADDR]]
+// CHECK-NEXT:   [[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]]
+// CHECK-NEXT:   [[M_ADDR:%.*]] = getelementptr{{.*}}%struct.bb, ptr [[THIS1]], i32 0, i32 0
+// CHECK-NEXT:   store i32 0, ptr [[M_ADDR]]