[clang-repl] VarDecl emitted outside TopLevelStmtDecl

[weliveindetail]

Works:

$ clang-repl
clang-repl> int a = 42;
clang-repl> int i = 0;
clang-repl> for (; i<42; i+=1) a-=1;
clang-repl> %quit

Fails:

$ clang-repl
clang-repl> int a = 42;
clang-repl> for(int i=0; i<42; i+=1) a-=1;
clang-repl: llvm-project/clang/lib/CodeGen/CGDecl.cpp:161: void clang::CodeGen::CodeGenFunction::EmitDecl(const Decl &): Assertion `VD.isLocalVarDecl() && "Should not see file-scope variables inside a function!"' failed.
Aborted (core dumped)

Other side-effects include incorrect behavior for definitions inside if conditions:

$ clang-repl -Xcc -Xclang,-ast-dump
clang-repl> int *a=nullptr;
TranslationUnitDecl 0x5555d8962468 prev 0x5555d893e690 <<invalid sloc>> <invalid sloc>
`-VarDecl 0x5555d8962518 <input_line_1:1:1, col:8> col:6 a 'int *' cinit
  `-ImplicitCastExpr 0x5555d8962590 <col:8> 'int *' <NullToPointer>
    `-CXXNullPtrLiteralExpr 0x5555d8962580 <col:8> 'std::nullptr_t'
clang-repl> if (auto *b=a) *b += 1;
TranslationUnitDecl 0x5555d8962608 prev 0x5555d8962468 <<invalid sloc>> <invalid sloc>
|-VarDecl 0x5555d89626c0 <input_line_2:1:5, col:13> col:11 used b 'int *' cinit
| `-ImplicitCastExpr 0x5555d8986b10 <col:13> 'int *' <LValueToRValue>
|   `-DeclRefExpr 0x5555d8962728 <col:13> 'int *' lvalue Var 0x5555d8962518 'a' 'int *'
`-TopLevelStmtDecl 0x5555d8986cb0 <col:1, col:22> col:1
  `-IfStmt 0x5555d8986c70 <col:1, col:22> has_var
    |-DeclStmt 0x5555d8986c98 <col:5, col:13>
    | `-VarDecl 0x5555d89626c0 <col:5, col:13> col:11 used b 'int *' cinit
    |   `-ImplicitCastExpr 0x5555d8986b10 <col:13> 'int *' <LValueToRValue>
    |     `-DeclRefExpr 0x5555d8962728 <col:13> 'int *' lvalue Var 0x5555d8962518 'a' 'int *'
    |-ImplicitCastExpr 0x5555d8986bb8 <col:11> 'bool' <PointerToBoolean>
    | `-ImplicitCastExpr 0x5555d8986ba0 <col:11> 'int *' <LValueToRValue>
    |   `-DeclRefExpr 0x5555d8986b80 <col:11> 'int *' lvalue Var 0x5555d89626c0 'b' 'int *'
    `-CompoundAssignOperator 0x5555d8986c40 <col:16, col:22> 'int' lvalue '+=' ComputeLHSTy='int' ComputeResultTy='int'
      |-UnaryOperator 0x5555d8986c08 <col:16, col:17> 'int' lvalue prefix '*' cannot overflow
      | `-ImplicitCastExpr 0x5555d8986bf0 <col:17> 'int *' <LValueToRValue>
      |   `-DeclRefExpr 0x5555d8986bd0 <col:17> 'int *' lvalue Var 0x5555d89626c0 'b' 'int *'
      `-IntegerLiteral 0x5555d8986c20 <col:22> 'int' 1
clang-repl> if (auto *b=a) *b += 1;
In file included from <<< inputs >>>:1:
input_line_3:1:11: error: redefinition of 'b'
    1 | if (auto *b=a) *b += 1;
      |           ^
<<< inputs >>>:1:1: note: '' included multiple times, additional include site here
<<< inputs >>>:1:1: note: '' included multiple times, additional include site here
error: Parsing failed.
clang-repl> %quit

I guess the VarDecl 0x5555d89626c0 for b should be emitted inside the TopLevelStmtDecl 0x5555d8986cb0 or even inside IfStmt 0x5555d8986c70.

[weliveindetail]

@vgvassilev Is this a known issue? Any idea how to fix it?

[vgvassilev]

@weliveindetail, without looking into the details I suspect we need to push a function scope when parsing the top-level statements. We already do that in

llvm-project/clang/lib/Parse/ParseDecl.cpp

Line 5679 in d7c80bb

Actions.PushFunctionScope();

however something extra might need to be added.

I'd wrap the broken code into a function and see what else clang pushes to parse i in function declaration context. Likely we will need to do the same in ParseTopLevelStatement...

[weliveindetail]

Thanks for the quick reply! Some observations follow.

Clang always wraps function bodies into CompoundStmt nodes, e.g.:

$ echo "int main() { return 0; }" | bin/clang++ -fsyntax-only -fno-color-diagnostics -Xclang -ast-dump -xc++ -c - -o -
TranslationUnitDecl 0x561d27ec49c8 <<invalid sloc>> <invalid sloc>
`-FunctionDecl 0x56342fd98948 <<stdin>:1:1, col:24> col:5 main 'int ()'
  `-CompoundStmt 0x56342fd98a98 <col:12, col:24>
    `-ReturnStmt 0x56342fd98a88 <col:14, col:21>
      `-IntegerLiteral 0x56342fd98a68 <col:21> 'int' 0

Pushing a CompoundScope on top of the FunctionScope fixes the assertion failure, but a copy of the VarDecl is still outside the TopLevelStmtDecl. Note that we end up with a duplicated initializer here:

clang-repl> for (int i=0; i<3; i+=1) ;
TranslationUnitDecl 0x555565d66f08 prev 0x555565d41f20 <<invalid sloc>> <invalid sloc>
|-VarDecl 0x555565d66f88 <input_line_1:1:6, col:12> col:10 used i 'int' cinit
| `-IntegerLiteral 0x555565d66ff0 <col:12> 'int' 0
`-TopLevelStmtDecl 0x555565d671a8 <col:1, col:26> col:1
  `-ForStmt 0x555565d67170 <col:1, col:26>
    |-DeclStmt 0x555565d67068 <col:6, col:13>
    | `-VarDecl 0x555565d66f88 <col:6, col:12> col:10 used i 'int' cinit
    |   `-IntegerLiteral 0x555565d66ff0 <col:12> 'int' 0
    | ...

I guess this is important for cases, where we have only a declaration, but no actual statement to be executed, e.g.:

clang-repl> int i=0;
TranslationUnitDecl 0x555565d66f08 prev 0x555565d41f20 <<invalid sloc>> <invalid sloc>
`-VarDecl 0x555565d66f88 <input_line_1:1:1, col:7> col:5 i 'int' cinit
  `-IntegerLiteral 0x555565d66ff0 <col:7> 'int' 0

Is this something we do explicitly in incremental mode? If so, we should probably make it conditional.

Apart from that. What is the reason for TopLevelStmtDecl to be a Decl and no actual Stmt? Couldn't we emit a CompoundStmt instead? (Or a specific TopLevelStmt that derives from it?)

[vgvassilev]

Thanks for the quick reply! Some observations follow.

Clang always wraps function bodies into CompoundStmt nodes, e.g.:

$ echo "int main() { return 0; }" | bin/clang++ -fsyntax-only -fno-color-diagnostics -Xclang -ast-dump -xc++ -c - -o -
TranslationUnitDecl 0x561d27ec49c8 <<invalid sloc>> <invalid sloc>
`-FunctionDecl 0x56342fd98948 <<stdin>:1:1, col:24> col:5 main 'int ()'
  `-CompoundStmt 0x56342fd98a98 <col:12, col:24>
    `-ReturnStmt 0x56342fd98a88 <col:14, col:21>
      `-IntegerLiteral 0x56342fd98a68 <col:21> 'int' 0

I am aware of:

void f() 
  try {}
  catch(...) {}

that's modeled with:

-FunctionDecl 0xd18e778 <<source>:1:1, line:3:15> line:1:6 f 'void ()'
  `-CXXTryStmt 0xd18e8a8 <line:2:3, line:3:15>

However, that does not invalidate your point.

Pushing a CompoundScope on top of the FunctionScope fixes the assertion failure, but a copy of the VarDecl is still outside the TopLevelStmtDecl. Note that we end up with a duplicated initializer here:

clang-repl> for (int i=0; i<3; i+=1) ;
TranslationUnitDecl 0x555565d66f08 prev 0x555565d41f20 <<invalid sloc>> <invalid sloc>
|-VarDecl 0x555565d66f88 <input_line_1:1:6, col:12> col:10 used i 'int' cinit
| `-IntegerLiteral 0x555565d66ff0 <col:12> 'int' 0
`-TopLevelStmtDecl 0x555565d671a8 <col:1, col:26> col:1
  `-ForStmt 0x555565d67170 <col:1, col:26>
    |-DeclStmt 0x555565d67068 <col:6, col:13>
    | `-VarDecl 0x555565d66f88 <col:6, col:12> col:10 used i 'int' cinit
    |   `-IntegerLiteral 0x555565d66ff0 <col:12> 'int' 0
    | ...

We have not considered this case where the control statements define variables which need to have a declaration context where they live. I am wondering why when parsing a for loop we put the VarDecl on file scope. Regular clang does not seem to put the the VarDecl on function scope. We must be doing something weird...

I guess this is important for cases, where we have only a declaration, but no actual statement to be executed, e.g.:

clang-repl> int i=0;
TranslationUnitDecl 0x555565d66f08 prev 0x555565d41f20 <<invalid sloc>> <invalid sloc>
`-VarDecl 0x555565d66f88 <input_line_1:1:1, col:7> col:5 i 'int' cinit
  `-IntegerLiteral 0x555565d66ff0 <col:7> 'int' 0

Is this something we do explicitly in incremental mode? If so, we should probably make it conditional.

Apart from that. What is the reason for TopLevelStmtDecl to be a Decl and no actual Stmt? Couldn't we emit a CompoundStmt instead? (Or a specific TopLevelStmt that derives from it?)

As part of the design work we decided to have a distinguished top-level entity which in clang is a Decl. Many interfaces, including CodeGen, work with declarations and that was the minimal, non-intrusive approach to enable what we needed.

[weliveindetail]

On current mainline I am getting CompoundStmt nodes for each scope: function, try and catch

$ echo "void f() { try {} catch(...) {} }" | bin/clang -xc++ -fsyntax-only -Xclang -ast-dump -c - -o -
TranslationUnitDecl 0x55d5b6a0a8f8 <<invalid sloc>> <invalid sloc>
`-FunctionDecl 0x55d5b6a54870 <<stdin>:1:1, col:33> col:6 f 'void ()'
  `-CompoundStmt 0x55d5b6a549c0 <col:10, col:33>
    `-CXXTryStmt 0x55d5b6a549a0 <col:12, col:31>
      |-CompoundStmt 0x55d5b6a54960 <col:16, col:17>
      `-CXXCatchStmt 0x55d5b6a54980 <col:19, col:31>
        |-<<<NULL>>>
        `-CompoundStmt 0x55d5b6a54970 <col:30, col:31>

As part of the design work we decided to have a distinguished top-level entity which in clang is a Decl. Many interfaces, including CodeGen, work with declarations and that was the minimal, non-intrusive approach to enable what we needed.

Thanks, that's reasonable. So, this is the REPL's equivalent for clang's FunctionDecl more or less.

I am wondering why when parsing a for loop we put the VarDecl on file scope. Regular clang does not seem to put the the VarDecl on function scope. We must be doing something weird...

Yes exactly. No worries, I will figure it out. Just wanted to check, if we know anything about it already.

[vgvassilev]

On current mainline I am getting CompoundStmt nodes for each scope: function, try and catch

$ echo "void f() { try {} catch(...) {} }" | bin/clang -xc++ -fsyntax-only -Xclang -ast-dump -c - -o -
TranslationUnitDecl 0x55d5b6a0a8f8 <<invalid sloc>> <invalid sloc>
`-FunctionDecl 0x55d5b6a54870 <<stdin>:1:1, col:33> col:6 f 'void ()'
  `-CompoundStmt 0x55d5b6a549c0 <col:10, col:33>
    `-CXXTryStmt 0x55d5b6a549a0 <col:12, col:31>
      |-CompoundStmt 0x55d5b6a54960 <col:16, col:17>
      `-CXXCatchStmt 0x55d5b6a54980 <col:19, col:31>
        |-<<<NULL>>>
        `-CompoundStmt 0x55d5b6a54970 <col:30, col:31>

I meant echo "void f() try {} catch(...) {} " | bin/clang -xc++ -fsyntax-only -Xclang -ast-dump -c - -o -

As part of the design work we decided to have a distinguished top-level entity which in clang is a Decl. Many interfaces, including CodeGen, work with declarations and that was the minimal, non-intrusive approach to enable what we needed.

Thanks, that's reasonable. So, this is the REPL's equivalent for clang's FunctionDecl more or less.

Yes, but this "FunctionDecl" is part of the static initialization process, eg. it is run as if it was called on file scope.

[weliveindetail]

I have a first patch here which mimics Clang's handling in terms of redefinition errors: weliveindetail@f86a8a6

It's not trivial to fix the asserts, because we have no notion of incremental mode in CG right? With this the frontend is happy, but it still generates the extra top-level VarDecl and thus, Orc fails with a DuplicateSymbol error. Any thoughts?

[vgvassilev]

I have a first patch here which mimics Clang's handling in terms of redefinition errors: weliveindetail@f86a8a6

It's not trivial to fix the asserts, because we have no notion of incremental mode in CG right? With this the frontend is happy, but it still generates the extra top-level VarDecl and thus, Orc fails with a DuplicateSymbol error. Any thoughts?

We do: it should be available via ASTContext::getLangOpts().IncrementalExtensions.

[vgvassilev]

I have a first patch here which mimics Clang's handling in terms of redefinition errors: weliveindetail@f86a8a6
It's not trivial to fix the asserts, because we have no notion of incremental mode in CG right? With this the frontend is happy, but it still generates the extra top-level VarDecl and thus, Orc fails with a DuplicateSymbol error. Any thoughts?

We do: it should be available via ASTContext::getLangOpts().IncrementalExtensions.

This is my take on fixing this issue: vgvassilev@96b93df

It still has 3 failing tests to be debugged:

Clang :: Interpreter/error-recovery-pragmas.cpp
Clang :: Interpreter/execute-stmts.cpp
Clang :: Interpreter/template-recovery.cpp

I can probably spend more time on this in a week but I thought that might get you going to fix the issue on your own.

[weliveindetail]

Thanks! In https://github.com/weliveindetail/llvm-project/commits/clang-repl-fix-var-def-in-control-stmts I just added the CompoundScope again and all tests pass for me on Ubuntu 22.04:

-- Testing: 23 of 18636 tests, 16 workers --
PASS: Clang :: Interpreter/sanity.c (1 of 23)
PASS: Clang :: Interpreter/stmt-serialization.cpp (2 of 23)
UNSUPPORTED: Clang :: Interpreter/plugins.cpp (3 of 23)
UNSUPPORTED: Clang :: Interpreter/CUDA/device-function-template.cu (4 of 23)
UNSUPPORTED: Clang :: Interpreter/CUDA/sanity.cu (5 of 23)
UNSUPPORTED: Clang :: Interpreter/CUDA/memory.cu (6 of 23)
UNSUPPORTED: Clang :: Interpreter/CUDA/host-and-device.cu (7 of 23)
UNSUPPORTED: Clang :: Interpreter/CUDA/device-function.cu (8 of 23)
PASS: Clang :: Interpreter/disambiguate-decl-stmt.cpp (9 of 23)
PASS: Clang :: Interpreter/incremental-mode.cpp (10 of 23)
PASS: Clang :: Interpreter/dynamic-library.cpp (11 of 23)
PASS: Clang :: Interpreter/global-dtor.cpp (12 of 23)
PASS: Clang :: Interpreter/simple-exception.cpp (13 of 23)
PASS: Clang :: Interpreter/execute-weak.cpp (14 of 23)
PASS: Clang :: Interpreter/multiline.cpp (15 of 23)
PASS: Clang :: Interpreter/cxx20-modules.cppm (16 of 23)
PASS: Clang :: Interpreter/code-undo.cpp (17 of 23)
PASS: Clang :: Interpreter/fail.cpp (18 of 23)
PASS: Clang :: Interpreter/lambda.cpp (19 of 23)
PASS: Clang :: Interpreter/execute.cpp (20 of 23)
PASS: Clang :: Interpreter/inline-virtual.cpp (21 of 23)
PASS: Clang :: Interpreter/execute-stmts.cpp (22 of 23)
PASS: Clang :: Interpreter/const.cpp (23 of 23)

Testing Time: 0.50s

Total Discovered Tests: 18636
  Excluded   : 18613 (99.88%)
  Unsupported:     6 (0.03%)
  Passed     :    17 (0.09%)

Execution seems to be exactly what we need. What is confusing is that this affects the AST dump, for example:

clang-repl> int i=1; i+=1;
TranslationUnitDecl 0x55a1d4d0b678 prev 0x55a1d4ce6690 <<invalid sloc>> <invalid sloc>
|-VarDecl 0x55a1d4d0b6f8 <input_line_1:1:1, col:7> col:5 used i 'int' cinit
| `-IntegerLiteral 0x55a1d4d0b760 <col:7> 'int' 1
`-TopLevelStmtDecl 0x55a1d4d0b848 <col:10, col:13> col:10
  `-CompoundAssignOperator 0x55a1d4d0b818 <col:10, col:13> 'int' lvalue '+=' ComputeLHSTy='int' ComputeResultTy='int'
    |-DeclRefExpr 0x55a1d4d0b7d8 <col:10> 'int' lvalue Var 0x55a1d4d0b6f8 'i' 'int'
    `-IntegerLiteral 0x55a1d4d0b7f8 <col:13> 'int' 1

When I apply your patch, the TopLevelStmtDecl is missing:

clang-repl> int i=1; i+=1;
TranslationUnitDecl 0x555565d64cc8 prev 0x555565d3fce0 <<invalid sloc>> <invalid sloc>
`-VarDecl 0x555565d64d48 <input_line_1:1:1, col:7> col:5 used i 'int' cinit
  `-IntegerLiteral 0x555565d64db0 <col:7> 'int' 1

I will look into it a little more and see if I can figure it out. If you have any ideas, let me know. Thx

[vgvassilev]

I will look into it a little more and see if I can figure it out. If you have any ideas, let me know. Thx

In my patch we made the TopLevelStmtDecl a DeclContext, too. I suspect some of the VisitTopLevelStmtDecl in the DeclPrinter will need to be adjusted?