Skip to content

[clang-repl] Always clean up scope and context for TopLevelStmtDecl #150215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 23, 2025

Conversation

devajithvs
Copy link
Contributor

@devajithvs devajithvs commented Jul 23, 2025

This fixes an issue introduced by #84150, where failing to pop compound scope, function scope info, and decl context after a failed statement could lead to an inconsistent internal state.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 23, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 23, 2025

@llvm/pr-subscribers-clang

Author: Devajith (devajithvs)

Changes

This fixes an issue introduced by llvm/llvm-project@4b70d17, where failing to pop compound scope, function scope info, and decl context after a failed statement could lead to an inconsistent internal state.


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

3 Files Affected:

  • (modified) clang/lib/Parse/ParseDecl.cpp (+1-2)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+2-1)
  • (modified) clang/test/Interpreter/fail.cpp (+8)
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 893ef02450921..e47caeb855d0c 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -5695,11 +5695,10 @@ Parser::DeclGroupPtrTy Parser::ParseTopLevelStmtDecl() {
                                Scope::CompoundStmtScope);
   TopLevelStmtDecl *TLSD = Actions.ActOnStartTopLevelStmtDecl(getCurScope());
   StmtResult R = ParseStatementOrDeclaration(Stmts, SubStmtCtx);
+  Actions.ActOnFinishTopLevelStmtDecl(TLSD, R.get());
   if (!R.isUsable())
     R = Actions.ActOnNullStmt(Tok.getLocation());
 
-  Actions.ActOnFinishTopLevelStmtDecl(TLSD, R.get());
-
   if (Tok.is(tok::annot_repl_input_end) &&
       Tok.getAnnotationValue() != nullptr) {
     ConsumeAnnotationToken();
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index fd22e012ea8b0..6e6669886ba36 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -20573,7 +20573,8 @@ TopLevelStmtDecl *Sema::ActOnStartTopLevelStmtDecl(Scope *S) {
 }
 
 void Sema::ActOnFinishTopLevelStmtDecl(TopLevelStmtDecl *D, Stmt *Statement) {
-  D->setStmt(Statement);
+  if (Statement)
+    D->setStmt(Statement);
   PopCompoundScope();
   PopFunctionScopeInfo();
   PopDeclContext();
diff --git a/clang/test/Interpreter/fail.cpp b/clang/test/Interpreter/fail.cpp
index 633d92794325c..c13029bc6e155 100644
--- a/clang/test/Interpreter/fail.cpp
+++ b/clang/test/Interpreter/fail.cpp
@@ -18,4 +18,12 @@ extern "C" int printf(const char *, ...);
 int i = 42;
 auto r1 = printf("i = %d\n", i);
 // CHECK: i = 42
+
+1aap = 42; // expected-error {{intended to fail the -verify test}}
+1aap = 42; i = 5;
+// expected-error {{intended to fail the -verify test but not hit an assertion}}
+
+printf("i = %d\n", i);
+// CHECK: i = 42
+
 %quit

@devajithvs
Copy link
Contributor Author

CC: @weliveindetail, @vgvassilev

Not sure fail.cpp is the best place for this test to go as this can be a bit confusing
cat %s | not clang-repl -Xcc -Xclang -Xcc -verify | FileCheck %s
is expected to pass as -verify is expected to fail. But the test itself will fail if it hits an assertion.

Fixes one of the failing test when updating ROOT/CLING to LLVM20: root-project/root#19242

@@ -18,4 +18,12 @@ extern "C" int printf(const char *, ...);
int i = 42;
auto r1 = printf("i = %d\n", i);
// CHECK: i = 42

1aap = 42; // expected-error {{intended to fail the -verify test}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we spell out the actual diagnostic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@devajithvs devajithvs force-pushed the cleanup-toplevelstmt branch from 6f8190a to 3111b1d Compare July 23, 2025 13:13

1aap = 42; // expected-error {{invalid digit 'a' in decimal constant}}
1aap = 42; i = 5;
// expected-error {{intended to fail the -verify test but not hit an assertion}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this one too.

This fixes an issue introduced by llvm/llvm-project@4b70d17bcffa, where
failing to pop compound scope, function scope info, and decl context
after a failed statement could lead to an inconsistent internal state.
@devajithvs devajithvs force-pushed the cleanup-toplevelstmt branch from 3111b1d to 92bcd85 Compare July 23, 2025 13:26
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @devajithvs, do you have commit access or should I merge?

@devajithvs
Copy link
Contributor Author

I don't have commit access. It'd be great if you can merge.

@vgvassilev vgvassilev merged commit 38a977d into llvm:main Jul 23, 2025
9 checks passed
mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Jul 28, 2025
…lvm#150215)

This fixes an issue introduced by
llvm#84150, where failing to pop
compound scope, function scope info, and decl context after a failed
statement could lead to an inconsistent internal state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants