[clang-repl] Support statements on global scope in incremental mode.

[v.g.vassilev]

Summary

[clang-repl] Support statements on global scope in incremental mode.

This patch teaches clang to parse statements on the global scope to allow:

 ./bin/clang-repl
clang-repl> int i = 12;
clang-repl> ++i;
clang-repl> extern "C" int printf(const char*,...);
clang-repl> printf("%d\n", i);
13
clang-repl> %quit

Generally, disambiguating between statements and declarations is a non-trivial task for a C++ parser. The challenge is to allow both standard C++ to be translated as if this patch does not exist and in the cases where the user typed a statement to be executed as if it were in a function body.

Clang's Parser does pretty well in disambiguating between declarations and expressions. We have added DisambiguatingWithExpression flag which allows us to preserve the existing and optimized behavior where needed and implement the extra rules for disambiguating. Only few cases require additional attention:

• Constructors/destructors -- Parser::isConstructorDeclarator was used in to disambiguate between ctor-looking declarations and statements on the global scope(eg. Ns::f()).
• The template keyword -- the template keyword can appear in both declarations and statements. This patch considers the template keyword to be a declaration starter which breaks a few cases in incremental mode which will be tackled later.
• The inline (and similar) keyword -- looking at the first token in many cases allows us to classify what is a declaration.
• Other language keywords and specifiers -- ObjC/ObjC++/OpenCL/OpenMP rely on pragmas or special tokens which will be handled in subsequent patches.

The patch conceptually models a "top-level" statement into a TopLevelStmtDecl. The TopLevelStmtDecl is lowered into a void function with no arguments. We attach this function to the global initializer list to execute the statement blocks in the correct order.

[v.g.vassilev]

Rely on the CXXScopeSpec to detect more accurately if we are parsing a constructor declarator.

[v.g.vassilev]

clang/lib/Parse/ParseDecl.cpp

3337

The only remaining failure is in test/Parser/cxx-class.cpp where we have something similar to:

namespace ctor_error {
  class Ctor { // expected-note{{not complete until the closing '}'}}
    Ctor(f)(int); // ok
    Ctor(g(int)); // ok
  };

  Ctor::Ctor (x) = { 0 }; // \
    // expected-error{{qualified reference to 'Ctor' is a constructor name}}
}

Now, Ctor::Ctor is recognized as a constructor and the diagnostic message regresses to:

error: 'error' diagnostics expected but not seen: 
  File /home/vvassilev/workspace/sources/llvm-project/clang/test/Parser/cxx-class.cpp Line 116: qualified reference to 'Ctor' is a constructor name
error: 'error' diagnostics seen but not expected: 
  File /home/vvassilev/workspace/sources/llvm-project/clang/test/Parser/cxx-class.cpp Line 116: unknown type name 'x'
2 errors generated.

It seems that the more accurate diagnostic used to come from Actions.getTypeName... Any ideas how to fix this?

[v.g.vassilev]

ping...

[v.g.vassilev]

Adding Aaron as a reviewer who hopefully could speed up the review.

[aaron.ballman]

The patch conceptually models the possible "top-level" statements between two top-level declarations into a block which is emitted as part of the global init function.
Currently we model this "statement block" as a function body to a void function taking no arguments. We attach this function to the global initializer list to execute the statement blocks in the correct order.

I think some of the tests have confusing behavior based on this conceptual model because the tests do things like declare namespaces and call functions; you can declare a namespace at file scope, not at function scope; you can't call a function at file scope (unless it's part of an initializer). So it the semantics wind up feeling confused because you can't tell whether you're at file scope or at function scope because you seem to be in both at the same time.

clang/include/clang/AST/ASTConsumer.h

54–59

In C and C++, the only thing that's at file scope (that the parser sees) are declarations, not statements, so the phrasing here seems off. I suspect this is because you don't *know* you're only going to get declarations (because it could be that we're trying to parse in the context of an imaginary function body)? If so, perhaps the comments could be updated to clarify that.

clang/include/clang/Parse/Parser.h

466

Drive by punctuation fix. I'm not certain I understand "as that is the only one used", but that's from the original comment. It's possible the comment holds no real weight any longer?

clang/lib/CodeGen/CodeGenModule.cpp

5107–5111 ↗

(On Diff #449066)

You should fix the other identifiers in this function to match the usual coding style.

clang/lib/Parse/ParseDecl.cpp

5514–5521

A constructor declaration must be a definition, so if we don't see a {, :, or try after the declarator then it's an expression, not a declaration.

Consider:

struct S {
  S();
};

S::S() = default;

The out-of-line constructor definition is still a kind of declaration despite not ending in {, :, or try.

clang/lib/Parse/ParseTentative.cpp

59

http://eel.is/c++draft/dcl.dcl#nt:empty-declaration

65

You should assert this at the top of the file; we shouldn't call into here for C code.

66

Do you need special handling for:

struct S {
  operator int();
};

S::operator int() { return 0; }

clang/lib/Parse/Parser.cpp

729–730

How close are we to having this fixme fixed?

735

clang/test/Interpreter/disambiguate-decl-stmt.cpp

2 ↗

(On Diff #449066)

Why is this not covered by the REQUIRES above?

28–33 ↗

(On Diff #449066)

This behavior is very confusing to me -- you cannot declare a namespace at function scope and you cannot call a function at file scope, so one of these two seems like it should not work.

(Same kinds of confusion happen elsewhere in the tests.)

clang/test/Interpreter/execute-stmts.cpp

10–12

Should this code be removed or get a FIXME comment?

[v.g.vassilev]

Address review comments

[v.g.vassilev]

clang/include/clang/AST/ASTConsumer.h

54–59

I am a bit confused. Could you clarify what wording changes you had in mind?

clang/include/clang/Parse/Parser.h

466

It does not indeed carry weight. Let me try to improve it.

clang/lib/CodeGen/CodeGenModule.cpp

5107–5111 ↗

(On Diff #449066)

I have not paid too much attention in the coding style here as it was mostly for initiating a design discussion.

The body of this function breaks the design of clang which assumes AST to be created by the frontend and be essentially immutable in CodeGen. This FunctionDecl needs to be treated as a global initializer. That is, it needs to be run in a particular order to the other initializer following the lexical occurrence of the statement block. That leaves us with two options, either to expose the registration of global initializers outside of CodeGen, or we need to create that here.

Alternatively, we could make HandleTopLevelStmts take a FunctionDecl and we can create it in the IncrementalParser. That blurs the responsibility and coherence of the interfaces. We will have public interface HandleTopLevelStmts (or similar) which handles top-level statements but take a FunctionDecl as a parameter which is created by the user...

clang/lib/Parse/ParseDecl.cpp

5514–5521

That came up a lot in the testsuite but I've added a test for it.

clang/lib/Parse/ParseTentative.cpp

59

IIUC, clang models the ; as a NullStmt.

65

That's a good point.

66

We seem to need that. Thanks. I've added a FIXME.

clang/lib/Parse/Parser.cpp

729–730

2000 tests away. The tests are mostly in OpenCL, OpenMP, etc, which just need adding more entries in places such as isCXXDeclarationSpecifier. I've decided to keep the patch small and then add that once we merge this one.

clang/test/Interpreter/disambiguate-decl-stmt.cpp

2 ↗

(On Diff #449066)

Good question, @sunho, @lhames do you remember if we still need this exclusion here?

IIRC, there are platforms which support the llvm-jit but with a limited set of features, eg. some platforms do not support reliably exceptions, others relocations (eg some ppc little endian machines).

28–33 ↗

(On Diff #449066)

The mental model has been that we have a mix of statements and declarations on the global scope. The sequence of statements are only considered to be as if they have been wrapped in a function body. For example:

namespace Ns {namespace Ns { void Ns(); void Fs();}}
void Ns::Ns::Ns() { printf("void Ns::Ns::Ns()\n"); }
void Ns::Ns::Fs() {}
Ns::Ns::Fs();
Ns::Ns::Ns();

should be semantically equivalent to:

namespace Ns {namespace Ns { void Ns(); void Fs();}}
void Ns::Ns::Ns() { printf("void Ns::Ns::Ns()\n"); }
void Ns::Ns::Fs() {}

void stmt_block1() {
  Ns::Ns::Fs();
  Ns::Ns::Ns();
}
auto _ = (stmt_block1(), 1);

Does that help in making it less confusing when thinking about it?

[aaron.ballman]

clang/include/clang/AST/ASTConsumer.h

54–59

It wasn't clear to me whether this was misnamed (HandleTopLevelDecls()) or whether the comments didn't make it clear that this is actually "statements and/or declarations and we don't know whether they're at file scope or not."

clang/lib/CodeGen/CodeGenModule.cpp

5107–5111 ↗

(On Diff #449066)

I have not paid too much attention in the coding style here as it was mostly for initiating a design discussion.

No worries, so long as you run clang-format over the patch before landing, it'll be fine.

either to expose the registration of global initializers outside of CodeGen, or we need to create that here.

Ideally, this should be generated outside of CodeGen. It seems to me that this could perhaps be done once the TU has finished but before codegen has started (in ActOnEndOfTranslationUnit() or ActOnEndOfTranslationUnitFragment())?

clang/lib/Parse/ParseTentative.cpp

59

That depends on where the ; is: https://godbolt.org/z/Kz9zjjKoe

clang/lib/Parse/Parser.cpp

729–730

LOL, okay, so a ways away still. :-D

clang/test/Interpreter/disambiguate-decl-stmt.cpp

28–33 ↗

(On Diff #449066)

The mental model has been that we have a mix of statements and declarations on the global scope.

I don't understand that mental model because neither C nor C++ allow you to have statements at the global scope. So I'm not certain how to think about statements showing up there (and more importantly, the languages and the compiler wouldn't expect that anyway). For example, consider this:

int n = 12;
int array[n];

This is valid at block scope and invalid at global scope: https://godbolt.org/z/8zxn3h79E

Similarly, consider this code:

template <typename Ty>
void func();

This is valid at file scope but invalid at block scope.

My concern here is that it will be a game of whack-a-mole to identify all of these circumstances and try to get the semantics somewhat close to correct. What is the harm with the REPL always being a full TU, so if the user wants a block scope, they're responsible for writing a function and calling it from main()?

[rsmith]

In terms of the high-level direction here, I think it would make sense to approach this by adding a full-fledged language extension to Clang to allow statements at the top level (with a -f flag to enable it), and then enable that extension in the interpreter. The major change that shift in viewpoint provides is that we should have explicit modeling of these top-level statements in the AST, rather than having them only transiently exist until they get passed off to the AST consumer. I think I'd want to see something like TopLevelStmtDecl added to explicitly model the case where we parse a statement at the top level. You can then extend the various parts of Clang that deal with global variable initializers to also handle TopLevelStmtDecls.

[aaron.ballman]

In D127284#3776036, @rsmith wrote:

In terms of the high-level direction here, I think it would make sense to approach this by adding a full-fledged language extension to Clang to allow statements at the top level (with a -f flag to enable it), and then enable that extension in the interpreter. The major change that shift in viewpoint provides is that we should have explicit modeling of these top-level statements in the AST, rather than having them only transiently exist until they get passed off to the AST consumer. I think I'd want to see something like TopLevelStmtDecl added to explicitly model the case where we parse a statement at the top level. You can then extend the various parts of Clang that deal with global variable initializers to also handle TopLevelStmtDecls.

Thank you for the suggestion! I actually have a different view on this that I was thinking about last night, but it's somewhat similar to yours in that it involves a flag to opt into behavior.

I don't think we should add a feature flag for this to Clang or support this functionality in Clang's AST -- it does not meet our language extension requirements (this won't be proposed to any standards body, etc). Further, I don't think Clang maintainers should have to play cognitive whack-a-mole as new statements and features are added to C and C++, wondering how they should behave if at the top level. Instead, I think it would make sense to add a flag to clang-repl so the user can decide whether they want their REPL experience to be "whole TU" or "pretend we're wrapped in a function". For users who want their experience to be both at the same time: that's an interesting idea, but it is not a C or C++ REPL experience; I think those users can go with the "whole TU" approach and write an extra line of code + some braces, as needed.

The reason I'm so uncomfortable with putting this into Clang is because neither language is designed to expect this sort of behavior and the resulting code will misbehave in mysterious ways when we guess wrong and it will be very difficult to address them all. I expect some things to be easy to recognize (see the template keyword and you know you can't be within a function scope), other things will require a bit of token lookahead (unsigned long long _Thread_local i = 12; cannot appear at local scope), and still others will require looking ahead through a potentially arbitrary number of statements (like with a VLA). I think the user should know explicitly which context they're in given that it matters to the source languages (and this is before we start to think about REPL over something like ObjC/OpenCL/HLSL/etc which may have even more interesting situations to worry about).

[aaron.ballman]

In D127284#3776805, @aaron.ballman wrote:

In D127284#3776036, @rsmith wrote:

In terms of the high-level direction here, I think it would make sense to approach this by adding a full-fledged language extension to Clang to allow statements at the top level (with a -f flag to enable it), and then enable that extension in the interpreter. The major change that shift in viewpoint provides is that we should have explicit modeling of these top-level statements in the AST, rather than having them only transiently exist until they get passed off to the AST consumer. I think I'd want to see something like TopLevelStmtDecl added to explicitly model the case where we parse a statement at the top level. You can then extend the various parts of Clang that deal with global variable initializers to also handle TopLevelStmtDecls.

Thank you for the suggestion! I actually have a different view on this that I was thinking about last night, but it's somewhat similar to yours in that it involves a flag to opt into behavior.

I don't think we should add a feature flag for this to Clang or support this functionality in Clang's AST -- it does not meet our language extension requirements (this won't be proposed to any standards body, etc). Further, I don't think Clang maintainers should have to play cognitive whack-a-mole as new statements and features are added to C and C++, wondering how they should behave if at the top level. Instead, I think it would make sense to add a flag to clang-repl so the user can decide whether they want their REPL experience to be "whole TU" or "pretend we're wrapped in a function". For users who want their experience to be both at the same time: that's an interesting idea, but it is not a C or C++ REPL experience; I think those users can go with the "whole TU" approach and write an extra line of code + some braces, as needed.

The reason I'm so uncomfortable with putting this into Clang is because neither language is designed to expect this sort of behavior and the resulting code will misbehave in mysterious ways when we guess wrong and it will be very difficult to address them all. I expect some things to be easy to recognize (see the template keyword and you know you can't be within a function scope), other things will require a bit of token lookahead (unsigned long long _Thread_local i = 12; cannot appear at local scope), and still others will require looking ahead through a potentially arbitrary number of statements (like with a VLA). I think the user should know explicitly which context they're in given that it matters to the source languages (and this is before we start to think about REPL over something like ObjC/OpenCL/HLSL/etc which may have even more interesting situations to worry about).

I had a really great conversation with @v.g.vassilev off-list about my concerns and the clang-repl needs/desires (thank you for taking the time to have that chat with me!), and this is a summary of what we think makes sense as a way forward:

Technical:

• clang gets a -cc1 (only) flag (or some other kind of not-user-facing option) to enable a special mode where you can mix statements and declarations at TU scope.
• clang-repl uses that new flag.
• clang gets a new AST node for TopLevelStmtDecl that's a Decl AST node wrapping a Stmt node; it's documented as only existing to support REPL. Add an assertion somewhere sensible that we never see one of these AST nodes outside of a clang-repl context.
• clang-repl uses that new AST node as-needed.

Administrative:

• clang-repl continues to push on standardization efforts in WG21 (and potentially WG14).
  • If those efforts lead to an explicit rejection of the idea, we should discuss how to proceed at that time, but I would envision that we'd remove the new AST node and the flag to enable this functionality, and introduce interfaces allowing us to synthesize code during codegen to try to break clang-repl users as little as possible. This way we aren't eating into standards body design space that will potentially give us problems in the future if the committees elect to do something *incompatible* in this space. (Basically: in a fight between clang-repl users and a new standard feature, the new standard feature "wins".) This will break clang-repl users, so we should consider documenting the potential for this up front (basically, call it an experimental feature of clang-repl rather than promising backwards compatibility for it).
• Any failures related to TopLevelStmtDecl will require collaboration between clang-repl and clang maintainers, but should mostly be driven by clang-repl maintainers.

[v.g.vassilev]

Address comments.

[v.g.vassilev]

Rebase.

clang/test/Interpreter/disambiguate-decl-stmt.cpp

28–33 ↗

(On Diff #449066)

That was clarified in the off-list discussion. I will try to summarize it for having proper history:

Our new facility teaches the parser to recognize statements when they were not declarations. If they were declarations we prefer the language standard. If they were valid statements which are spelled out on the global scope we pretend they were written in block scope, form a function dedicated definition and order it for global initialization.

int n = 12;
int array[n];

is still invalid on global scope.

template <typename Ty>
void func();

is untouched.

[aaron.ballman]

It looks like precommit CI caught a relevant issue that should be addressed.

Also, there's an in-progress patch that might be worth keeping an eye on in case it changes the behavior for clang-repl in bad ways: https://reviews.llvm.org/D137020 -- the patch causes unknown declaration specifiers to be parsed as if they were a type specifier rather than an expression, which could potentially change repl behavior.

clang/include/clang/AST/Decl.h

4255 ↗

(On Diff #472224)

4258 ↗

(On Diff #472224)

4271–4276 ↗

(On Diff #472224)

I think we probably want this to assert(Statements.empty()) rather than silently fail to set the statements.

4274 ↗

(On Diff #472224)

It's a noop change, but perhaps std::unitialized_copy instead?

4287 ↗

(On Diff #472224)

Should we add an overload pair here for better const correctness? e.g.,

ArrayRef<const Stmt *> statements() const { return {Stmts, NumStmts}; }
ArrayRef<Stmt *> statements() { return {Stmts, NumStmts}; }

clang/include/clang/Basic/DeclNodes.td

98 ↗

(On Diff #472224)

Oh boy, that node name is a hoot. It ends in Stmt but it's a Decl. :-D I think it's fine because it matches the convention used for all the other identifiers here, but it may be worth thinking about whether we want a different name for the AST node like TopLevelPseudoStmtDecl or FileScopePseudoStatementDecl. I don't insist on a change here though, just something to consider.

clang/include/clang/Lex/Preprocessor.h

1782–1785 ↗

(On Diff #472224)

We should be able to drop this as part of this patch, right? (I think you can modify the IncrementalAction object so that it calls CI.getLangOpts().IncrementalExtensions = true; if needed, but you're passing the cc1 flag to the invocation and so I think you can maybe remove this call entirely.)

clang/lib/AST/Decl.cpp

5244 ↗

(On Diff #472224)

5251 ↗

(On Diff #472224)

5270–5273 ↗

(On Diff #472224)

Some renaming for style, but also, this gets a name that's actually a reserved identifier instead of one that the user could potentially be using.

5276–5277 ↗

(On Diff #472224)

5282 ↗

(On Diff #472224)

clang/lib/CodeGen/CodeGenModule.cpp

6358 ↗

(On Diff #472224)

clang/lib/Parse/ParseDecl.cpp

5241–5242

Are you planning on doing this as part of this patch?

clang/lib/Sema/SemaDecl.cpp

19540 ↗

(On Diff #472224)

Can use auto here since the type is spelled out in the initialization (for some reason Phab won't let me suggest an edit here).

clang/test/Interpreter/disambiguate-decl-stmt.cpp

2 ↗

(On Diff #449066)

Still wondering on this bit.

[v.g.vassilev]

Address more comments.

[v.g.vassilev]

clang/include/clang/Basic/DeclNodes.td

98 ↗

(On Diff #472224)

Indeed, I'd put the pseudo to qualify the decl, like TopLevelStmtPseudoDecl but I still don't think either is an improvement to what we have currently.

clang/include/clang/Lex/Preprocessor.h

1782–1785 ↗

(On Diff #472224)

I wanted to do this is a separate commit. I am worried of breaking downstream users. I remember long time ago @akyrtzi was using this logic.

There are also a bunch of tests in clang and lldb.

clang/lib/CodeGen/CodeGenModule.cpp

6358 ↗

(On Diff #472224)

I'd like to keep this as the FunctionDecl type makes it move obvious what getOrConvertToFunction returns.

clang/lib/Parse/ParseDecl.cpp

5241–5242

I'd like to do it in a following patch as the patch will mostly touch other languages and would spill this one.

clang/test/Interpreter/disambiguate-decl-stmt.cpp

2 ↗

(On Diff #449066)

// REQUIRES: host-supports-jit ensures that the system supports spawning a JIT.
// UNSUPPORTED: system-aix means that this test is not supported on AIX.

These options were discovered through a lot of pain and suffering by a lot of reverts. I would prefer to keep UNSUPPORTED line in this patch, and try to undo it in a small and atomic commit later to see if that test can be executed on AIX.

[v.g.vassilev]

In D127284#3902487, @aaron.ballman wrote:

It looks like precommit CI caught a relevant issue that should be addressed.

Yes, I have a comment (starting with "The only remaining failure is in test/Parser/cxx-class.cpp where we have something similar to") asking for help. I have accidentally marked it as resolved but it is not. I will spend some time tomorrow trying to figure out what's the best way forward.

Also, there's an in-progress patch that might be worth keeping an eye on in case it changes the behavior for clang-repl in bad ways: https://reviews.llvm.org/D137020 -- the patch causes unknown declaration specifiers to be parsed as if they were a type specifier rather than an expression, which could potentially change repl behavior.

Thanks for the heads up!

[rsmith]

clang/lib/Parse/ParseDecl.cpp

5520–5521

I don't think any of this should be necessary. Per the function comment, it's a precondition of this function that we're looking at either ClassName or ClassNameScope::ClassName and we shouldn't be re-checking that.

I also think we should not be returning true here if we see ClassName::ClassName without looking at the rest of the tokens. That'll cause diagnostic quality regressions like the one you're seeing. We should continue to the rest of this function to see if we have (Type or () next.

clang/lib/Parse/ParseTentative.cpp

66

I think you should check isCurrentClassName before calling this, like the other callers of this function do. My understanding is that the intent is for isConstructorDeclarator to only check the (potential) function declarator that appears after the name, not to check the name itself.

[v.g.vassilev]

Address comments. Call isCurrentClassName before calling isConstructorDeclarator.

clang/lib/Parse/ParseDecl.cpp

5520–5521

Indeed. I've reverted that and I did not need to have changes in this routine anymore. Thanks!!

clang/lib/Parse/ParseTentative.cpp

66

Ah, okay, that was the missing bit.

[v.g.vassilev]

Remove several fixmes. Now we can deal with templates, deduction guides and operators.

[v.g.vassilev]

clang/lib/Parse/Parser.cpp

1021

There is a remaining challenge which probably could be addressed outside of this patch.

Consider this statement block:

int i =  12;
++i; 
i--;

template<typename T> struct A { };

Ideally we should model ++i; i--; as a single TopLevelStmtDecl as the statement block is contiguous. That would require the creation of 2 AST nodes per block (one for the TopLevelStmtDecl and one for its conversion to FunctionDecl). This will give us also a nice property on the REPL side where the user could decide to squash multiple statements into a statement block to save on memory.

To do so, we will need to use isDeclarationStatement as a stop rule in ParseTopLevelDecl. In turn, this would mean that we should duplicate all of the switch cases described in the ParseExternalDeclaration function here. [We need teach isDeclarationStatement everything we know about declarations, eg. it must tell us to stop when we see definition struct A].

The last version of this patch goes in the opposite direction, trying to minimize the code duplication (bloat?) by wrapping each global statement into a TopLevelStmtDecl, reusing the logic in ParseExternalDeclaration. However, we pay the price for 2 AST node allocations per global statement. That is a serious hit for people that want to control the parsing granularity of an interpreter.

I wonder if we can do something better hitting both requirements in some smart way I cannot see...

[v.g.vassilev]

Rebase + fix the failure on windows

[rsmith]

clang/lib/AST/Decl.cpp

5264 ↗

(On Diff #475470)

I would hope that we can remove this. Instead, I think we can teach CodeGen to emit a sequence of TopLevelStmtDecls directly as an LLVM IR function -- if it's not emitted anything else nor flushed its IR output since it last emitted a TopLevelStmtDecl, then reuse and extend the previous Function, otherwise create a new one. That would also allow us to make TopLevelStmtDecl model exactly one Stmt, which seems cleaner.

clang/lib/Parse/ParseTentative.cpp

57–58

Can we sink this into the switch on the token kind below?

clang/lib/Parse/Parser.cpp

1021

It seems to me that the big cost here is creating a FunctionDecl and all of its ancillary components; a TopLevelStmtDecl is pretty cheap. I don't think it should be necessary to create that FunctionDecl at all -- we should be able to go straight from TopLevelStmtDecl to an IR function like we go straight from a VarDecl for a global function to its initializer IR function without creating a FunctionDecl.

[v.g.vassilev]

Address comments, support better C in clang-repl.

[v.g.vassilev]

Teach CodeGen to squash contiguous top-level stmt decl blocks into a single llvm::Function. Rebase.

[aaron.ballman]

In general, this looks like an improvement over the last iteration (thank you for the suggestions, @rsmith!). I think this LGTM but there's enough changes in CodeGen that I'd appreciate a second set of eyes on those changes before giving the go-ahead. I've added a few more reviewers to hopefully help with that.

clang/include/clang/AST/Decl.h

4264–4267 ↗

(On Diff #477312)

Both are now unused.

clang/include/clang/Basic/DeclNodes.td

98 ↗

(On Diff #472224)

Yeah, I'm "okay" with the name as-is.

clang/include/clang/Lex/Preprocessor.h

1782–1785 ↗

(On Diff #472224)

I wanted to do this is a separate commit. I am worried of breaking downstream users.

Downstream users have no expectation of this interface remaining stable to begin with, so I'd rather we remove the code unless someone speaks up with a concrete technical problem. That said, I'm fine doing it in a separate commit so that it's easier to raise awareness for downstreams if you think this will be disruptive to them.

clang/lib/AST/Decl.cpp

5249–5250 ↗

(On Diff #477312)

Might as well make this form of the constructor take a Stmt * instead of doing a two-step initialization.

clang/lib/AST/DeclPrinter.cpp

936–938 ↗

(On Diff #477312)

getStmt() can return null; should this assert we're not calling it in such a case?

clang/test/Interpreter/disambiguate-decl-stmt.cpp

2 ↗

(On Diff #449066)

SGTM, thank you for investigating!

[rsmith]

This is looking good to me. No further non-trivial concerns on my side.

clang/lib/CodeGen/CodeGenModule.cpp

6165 ↗

(On Diff #477312)

Instead of a name comparison, can you check whether CXXGlobalInits.back() == CurCGF->CurFn?

6172 ↗

(On Diff #477312)

Maybe add a TODO to ask the ABI name mangler to pick a name?

clang/lib/Parse/ParseDecl.cpp

5248

I guess this happens in some pragma situations? Can you put a real diagnostic in here rather than an assert?

clang/lib/Parse/Parser.cpp

1016–1018

Have you tried this with the latest version of the patch? Can the FIXME be removed?

[v.g.vassilev]

I think I have addressed all comments from @aaron.ballman and @rsmith.

clang/include/clang/Lex/Preprocessor.h

1782–1785 ↗

(On Diff #472224)

I'd prefer doing it in a separate commit. This patch is bulky and we may need to revert it making all bots happy. That'd be probably make downstream consumers green/red for a while and generate a some email traffic ;)

clang/lib/CodeGen/CodeGenModule.cpp

6165 ↗

(On Diff #477312)

Awesome! Thanks!

clang/lib/Parse/ParseDecl.cpp

5248

That's what I understood from the code. Thanks for clarifying. I added a diagnostic.

clang/lib/Parse/Parser.cpp

1016–1018

I have tried. The failures are a couple of hundred now. However, the failing tests are due to the fact that the tests check for expected diagnostics from ill-formed code. There the decisions if something that's a statement or a declaration are harder and sometimes we produce unexpected diagnostics. I am not sure if we should be fixing the test files.

I will investigate more thoroughly but I have removed the FIXME as the majority of the failures are resolved by the current version of the patch.

[v.g.vassilev]

rebase.

[rsmith]

clang/lib/Parse/Parser.cpp

1016–1018

OK, I think that's reasonable. I think it would be interesting to try building some non-trivial C++ code, perhaps bootstrapping Clang, with -Xclang -fincremental-extensions, as a check for whether there are any cases where we disambiguate a valid top-level declaration as a statement. But I don't think that should be a blocker for landing this patch.

[v.g.vassilev]

clang/lib/Parse/Parser.cpp

1016–1018

I don't want to hold off that patch any longer as well.

I started looking into that. Currently I have decided to track it here https://reviews.llvm.org/D139016 as some of the changes might require further review/discussion.

Parsing things seems fine, I get up to libSupport but then there are some linker failures I need to investigate.

[aaron.ballman]

clang/include/clang/Lex/Preprocessor.h

1782–1785 ↗

(On Diff #472224)

Doing it as a separate commit is fine by me.

clang/lib/Parse/ParseDecl.cpp

5248–5253

Why is this using a custom diagnostic instead of adding a typical diagnostic to DiagnosticParseKinds.td?

[v.g.vassilev]

clang/lib/Parse/ParseDecl.cpp

5248–5253

I don't have a problem converting this to a DiagnosticParseKind. However, this is a temporary diagnostic and we risk once the FIXME is resolved to leave a stray diagnostic id.

[aaron.ballman]

clang/lib/Parse/ParseDecl.cpp

5248–5253

It's been my experience that there's nothing more permanent than a temporary solution, so I'd say we should go with the regular diagnostic. Also, it should be worded unsupported statement at global scope to fit the usual diagnostic wording style.

[v.g.vassilev]

Address comments, fix a fixme.

clang/lib/Parse/ParseDecl.cpp

5248–5253

I took the liberty to address the FIXME.

[v.g.vassilev]

I screwed up with the last diff. Let me fix it in a bit...

[v.g.vassilev]

Fixed the diff. I accidentally have added the commits from https://reviews.llvm.org/D139016

[aaron.ballman]

LGTM!

[Michael137]

FYI, looks like this broke the LLDB incremental buildbot. Following tests started failing:

lldb-api.commands/expression/argument_passing_restrictions.TestArgumentPassingRestrictions.py
lldb-api.commands/expression/deleting-implicit-copy-constructor.TestDeletingImplicitCopyConstructor.py
lldb-api.commands/expression/diagnostics.TestExprDiagnostics.py
lldb-api.commands/expression/import-std-module/array.TestArrayFromStdModule.py
lldb-api.commands/expression/import-std-module/module-build-errors.TestStdModuleBuildErrors.py
lldb-api.commands/expression/rdar44436068.Test128BitsInteger.py
lldb-api.commands/expression/weak_symbols.TestWeakSymbols.py
lldb-api.commands/target/dump-pcm-info.TestDumpPCMInfo.py
lldb-api.functionalities/data-formatter/compactvectors.TestCompactVectors.py
lldb-api.functionalities/tail_call_frames/cross_object.TestCrossObjectTailCalls.py
lldb-api.functionalities/target_var/no_vars.TestTargetVarNoVars.py
lldb-api.lang/c/modules.TestCModules.py
lldb-api.lang/cpp/accelerator-table.TestCPPAccelerator.py
lldb-api.lang/cpp/modules-import.TestCXXModulesImport.py
lldb-api.lang/objc/modules.TestObjCModules.py
lldb-api.lang/objc/modules-auto-import.TestModulesAutoImport.py
lldb-api.lang/objc/modules-cache.TestClangModulesCache.py
lldb-api.lang/objc/modules-compile-error.TestModulesCompileError.py
lldb-api.lang/objc/modules-incomplete.TestIncompleteModules.py
lldb-api.lang/objc/modules-inline-functions.TestModulesInlineFunctions.py
lldb-api.lang/objc/modules-non-objc-target.TestObjCModulesNonObjCTarget.py
lldb-api.lang/objc/modules-objc-property.TestModulesObjCProperty.py
lldb-api.tools/lldb-server.TestLldbGdbServer.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48872/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48873/

Last successful build: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48871/

[v.g.vassilev]

In D127284#3969225, @Michael137 wrote:

FYI, looks like this broke the LLDB incremental buildbot. Following tests started failing:

lldb-api.commands/expression/argument_passing_restrictions.TestArgumentPassingRestrictions.py
lldb-api.commands/expression/deleting-implicit-copy-constructor.TestDeletingImplicitCopyConstructor.py
lldb-api.commands/expression/diagnostics.TestExprDiagnostics.py
lldb-api.commands/expression/import-std-module/array.TestArrayFromStdModule.py
lldb-api.commands/expression/import-std-module/module-build-errors.TestStdModuleBuildErrors.py
lldb-api.commands/expression/rdar44436068.Test128BitsInteger.py
lldb-api.commands/expression/weak_symbols.TestWeakSymbols.py
lldb-api.commands/target/dump-pcm-info.TestDumpPCMInfo.py
lldb-api.functionalities/data-formatter/compactvectors.TestCompactVectors.py
lldb-api.functionalities/tail_call_frames/cross_object.TestCrossObjectTailCalls.py
lldb-api.functionalities/target_var/no_vars.TestTargetVarNoVars.py
lldb-api.lang/c/modules.TestCModules.py
lldb-api.lang/cpp/accelerator-table.TestCPPAccelerator.py
lldb-api.lang/cpp/modules-import.TestCXXModulesImport.py
lldb-api.lang/objc/modules.TestObjCModules.py
lldb-api.lang/objc/modules-auto-import.TestModulesAutoImport.py
lldb-api.lang/objc/modules-cache.TestClangModulesCache.py
lldb-api.lang/objc/modules-compile-error.TestModulesCompileError.py
lldb-api.lang/objc/modules-incomplete.TestIncompleteModules.py
lldb-api.lang/objc/modules-inline-functions.TestModulesInlineFunctions.py
lldb-api.lang/objc/modules-non-objc-target.TestObjCModulesNonObjCTarget.py
lldb-api.lang/objc/modules-objc-property.TestModulesObjCProperty.py
lldb-api.tools/lldb-server.TestLldbGdbServer.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48872/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48873/

Last successful build: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48871/

Hi @Michael137, I cannot reproduce these failures on unix. Is there some special way I need to compile. I will try reproducing them on OSX. Did you have time to debug where the tests fail?

[v.g.vassilev]

In D127284#3969249, @v.g.vassilev wrote:

In D127284#3969225, @Michael137 wrote:

FYI, looks like this broke the LLDB incremental buildbot. Following tests started failing:

lldb-api.commands/expression/argument_passing_restrictions.TestArgumentPassingRestrictions.py
lldb-api.commands/expression/deleting-implicit-copy-constructor.TestDeletingImplicitCopyConstructor.py
lldb-api.commands/expression/diagnostics.TestExprDiagnostics.py
lldb-api.commands/expression/import-std-module/array.TestArrayFromStdModule.py
lldb-api.commands/expression/import-std-module/module-build-errors.TestStdModuleBuildErrors.py
lldb-api.commands/expression/rdar44436068.Test128BitsInteger.py
lldb-api.commands/expression/weak_symbols.TestWeakSymbols.py
lldb-api.commands/target/dump-pcm-info.TestDumpPCMInfo.py
lldb-api.functionalities/data-formatter/compactvectors.TestCompactVectors.py
lldb-api.functionalities/tail_call_frames/cross_object.TestCrossObjectTailCalls.py
lldb-api.functionalities/target_var/no_vars.TestTargetVarNoVars.py
lldb-api.lang/c/modules.TestCModules.py
lldb-api.lang/cpp/accelerator-table.TestCPPAccelerator.py
lldb-api.lang/cpp/modules-import.TestCXXModulesImport.py
lldb-api.lang/objc/modules.TestObjCModules.py
lldb-api.lang/objc/modules-auto-import.TestModulesAutoImport.py
lldb-api.lang/objc/modules-cache.TestClangModulesCache.py
lldb-api.lang/objc/modules-compile-error.TestModulesCompileError.py
lldb-api.lang/objc/modules-incomplete.TestIncompleteModules.py
lldb-api.lang/objc/modules-inline-functions.TestModulesInlineFunctions.py
lldb-api.lang/objc/modules-non-objc-target.TestObjCModulesNonObjCTarget.py
lldb-api.lang/objc/modules-objc-property.TestModulesObjCProperty.py
lldb-api.tools/lldb-server.TestLldbGdbServer.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48872/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48873/

Last successful build: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48871/

Hi @Michael137, I cannot reproduce these failures on unix. Is there some special way I need to compile. I will try reproducing them on OSX. Did you have time to debug where the tests fail?

I cannot reproduce this on osx either.

[Michael137]

In D127284#3969308, @v.g.vassilev wrote:

In D127284#3969249, @v.g.vassilev wrote:

In D127284#3969225, @Michael137 wrote:

FYI, looks like this broke the LLDB incremental buildbot. Following tests started failing:

lldb-api.commands/expression/argument_passing_restrictions.TestArgumentPassingRestrictions.py
lldb-api.commands/expression/deleting-implicit-copy-constructor.TestDeletingImplicitCopyConstructor.py
lldb-api.commands/expression/diagnostics.TestExprDiagnostics.py
lldb-api.commands/expression/import-std-module/array.TestArrayFromStdModule.py
lldb-api.commands/expression/import-std-module/module-build-errors.TestStdModuleBuildErrors.py
lldb-api.commands/expression/rdar44436068.Test128BitsInteger.py
lldb-api.commands/expression/weak_symbols.TestWeakSymbols.py
lldb-api.commands/target/dump-pcm-info.TestDumpPCMInfo.py
lldb-api.functionalities/data-formatter/compactvectors.TestCompactVectors.py
lldb-api.functionalities/tail_call_frames/cross_object.TestCrossObjectTailCalls.py
lldb-api.functionalities/target_var/no_vars.TestTargetVarNoVars.py
lldb-api.lang/c/modules.TestCModules.py
lldb-api.lang/cpp/accelerator-table.TestCPPAccelerator.py
lldb-api.lang/cpp/modules-import.TestCXXModulesImport.py
lldb-api.lang/objc/modules.TestObjCModules.py
lldb-api.lang/objc/modules-auto-import.TestModulesAutoImport.py
lldb-api.lang/objc/modules-cache.TestClangModulesCache.py
lldb-api.lang/objc/modules-compile-error.TestModulesCompileError.py
lldb-api.lang/objc/modules-incomplete.TestIncompleteModules.py
lldb-api.lang/objc/modules-inline-functions.TestModulesInlineFunctions.py
lldb-api.lang/objc/modules-non-objc-target.TestObjCModulesNonObjCTarget.py
lldb-api.lang/objc/modules-objc-property.TestModulesObjCProperty.py
lldb-api.tools/lldb-server.TestLldbGdbServer.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48872/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48873/

Last successful build: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48871/

Hi @Michael137, I cannot reproduce these failures on unix. Is there some special way I need to compile. I will try reproducing them on OSX. Did you have time to debug where the tests fail?

I cannot reproduce this on osx either.

Hmmm how are you running the tests? It reproduces pretty consistently with the patch and doesn't without it. I just run ninja check-lldb-api (assuming you built with LLDB_INCLUDE_TESTS=ON)

Let me try with a clean build

UPDATE: fails with clean build too

Might be best to revert it for now while we figure out what's wrong

[v.g.vassilev]

In D127284#3969398, @Michael137 wrote:

In D127284#3969308, @v.g.vassilev wrote:

In D127284#3969249, @v.g.vassilev wrote:

In D127284#3969225, @Michael137 wrote:

FYI, looks like this broke the LLDB incremental buildbot. Following tests started failing:

lldb-api.commands/expression/argument_passing_restrictions.TestArgumentPassingRestrictions.py
lldb-api.commands/expression/deleting-implicit-copy-constructor.TestDeletingImplicitCopyConstructor.py
lldb-api.commands/expression/diagnostics.TestExprDiagnostics.py
lldb-api.commands/expression/import-std-module/array.TestArrayFromStdModule.py
lldb-api.commands/expression/import-std-module/module-build-errors.TestStdModuleBuildErrors.py
lldb-api.commands/expression/rdar44436068.Test128BitsInteger.py
lldb-api.commands/expression/weak_symbols.TestWeakSymbols.py
lldb-api.commands/target/dump-pcm-info.TestDumpPCMInfo.py
lldb-api.functionalities/data-formatter/compactvectors.TestCompactVectors.py
lldb-api.functionalities/tail_call_frames/cross_object.TestCrossObjectTailCalls.py
lldb-api.functionalities/target_var/no_vars.TestTargetVarNoVars.py
lldb-api.lang/c/modules.TestCModules.py
lldb-api.lang/cpp/accelerator-table.TestCPPAccelerator.py
lldb-api.lang/cpp/modules-import.TestCXXModulesImport.py
lldb-api.lang/objc/modules.TestObjCModules.py
lldb-api.lang/objc/modules-auto-import.TestModulesAutoImport.py
lldb-api.lang/objc/modules-cache.TestClangModulesCache.py
lldb-api.lang/objc/modules-compile-error.TestModulesCompileError.py
lldb-api.lang/objc/modules-incomplete.TestIncompleteModules.py
lldb-api.lang/objc/modules-inline-functions.TestModulesInlineFunctions.py
lldb-api.lang/objc/modules-non-objc-target.TestObjCModulesNonObjCTarget.py
lldb-api.lang/objc/modules-objc-property.TestModulesObjCProperty.py
lldb-api.tools/lldb-server.TestLldbGdbServer.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48872/
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48873/

Last successful build: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48871/

Hi @Michael137, I cannot reproduce these failures on unix. Is there some special way I need to compile. I will try reproducing them on OSX. Did you have time to debug where the tests fail?

I cannot reproduce this on osx either.

Hmmm how are you running the tests? It reproduces pretty consistently with the patch and doesn't without it. I just run ninja check-lldb-api (assuming you built with LLDB_INCLUDE_TESTS=ON)

Let me try with a clean build

I used cmake -C ../../sources/llvm-project/lldb/cmake/caches/Apple-lldb-macOS.cmake -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi" -DLLDB_USE_SYSTEM_DEBUGSERVER=ON -DLLVM_ENABLE_PROJECTS="clang;lldb" -DLLVM_TARGETS_TO_BUILD=host -DCMAKE_BUILD_TYPE=Debug -DLLDB_INCLUDE_TESTS=ON -G Ninja ../../sources/llvm-project/llvm

I run:

ninja -j20 check-lldb-api -v
[0/3] cd /Users/vvassilev/workspace/builds/llvm-project/runtimes/runtimes-bins && /usr/local/Cellar/cmake/3.20.2/bin/cmake --build /Users/vvassilev/workspace/builds/llvm-project/runtimes/runtimes-bins/ --target cxx --config Debug
ninja: no work to do.
[1/3] cd /Users/vvassilev/workspace/builds/llvm-project/tools/lldb/test/API && /usr/local/Cellar/cmake/3.20.2/bin/cmake -E copy_if_different /Applications/Xcode.app/Contents/Developer/../SharedFrameworks/LLDB.framework/Resources/debugserver /Users/vvassilev/workspace/builds/llvm-project/./bin
[2/3] cd /Users/vvassilev/workspace/builds/llvm-project/tools/lldb/test/API && /Users/vvassilev/miniconda/bin/python3.9 /Users/vvassilev/workspace/builds/llvm-project/./bin/llvm-lit -sv /Users/vvassilev/workspace/builds/llvm-project/tools/lldb/test/API

Testing Time: 0.38s
  Unsupported: 1070

For some reasons the failing tests are marked as unresolved for me. I have osx 12.2.1

[v.g.vassilev]

UPDATE: fails with clean build too

Might be best to revert it for now while we figure out what's wrong

I fail to reproduce it, can you give me access to some node where I can debug? I suspect that should be something easy to fix.

UPDATE: Reproduced it. Debugging.

[v.g.vassilev]

In D127284#3969446, @v.g.vassilev wrote:

UPDATE: fails with clean build too

Might be best to revert it for now while we figure out what's wrong

I fail to reproduce it, can you give me access to some node where I can debug? I suspect that should be something easy to fix.

UPDATE: Reproduced it. Debugging.

@Michael137, I think I understand what happens. This patch introduces a new language option which is not benign from modules perspective. However, lldb sets up modules the old way and then switches to the incremental processing mode. We have two ways to fix this:

• Pass -Xclang -fincremental-extensions in ClangModulesDeclVendor.cpp and delete instance->getPreprocessor().enableIncrementalProcessing();; or
• Apply https://reviews.llvm.org/D139258 which already does that.

Can you check if https://reviews.llvm.org/D139258 passes the lldb testsuite. It does pass my reproduction by hand (I still cannot run full of the lldb-api tests)?

[Michael137]

In D127284#3969458, @v.g.vassilev wrote:

In D127284#3969446, @v.g.vassilev wrote:

UPDATE: fails with clean build too

Might be best to revert it for now while we figure out what's wrong

I fail to reproduce it, can you give me access to some node where I can debug? I suspect that should be something easy to fix.

UPDATE: Reproduced it. Debugging.

@Michael137, I think I understand what happens. This patch introduces a new language option which is not benign from modules perspective. However, lldb sets up modules the old way and then switches to the incremental processing mode. We have two ways to fix this:

• Pass -Xclang -fincremental-extensions in ClangModulesDeclVendor.cpp and delete instance->getPreprocessor().enableIncrementalProcessing();; or
• Apply https://reviews.llvm.org/D139258 which already does that.

Can you check if https://reviews.llvm.org/D139258 passes the lldb testsuite. It does pass my reproduction by hand (I still cannot run full of the lldb-api tests)?

Thanks for taking a look. I tried with the suggested patch on the Objective-C API tests and there's only 1 test failure remaining there:

lang/objc/modules-compile-error/TestModulesCompileError.py

Expecting sub string: "module.h:4:1: error: unknown type name 'syntax_error_for_lldb_to_find'" (was not found)

The test expects an error string that looks like unknown type name 'syntax_error_for_lldb_to_find' but gets use of 'undeclared identifier 'syntax_error_for_lldb_to_find'

Haven't looked much further than that. Does that sound familiar to you? I see a similar test fix as part of this patch. Maybe it's just a matter of fixing up the expected string.

I think we should revert for now until https://reviews.llvm.org/D139258 is ready to go. Just to unblock the buildbot

[v.g.vassilev]

In D127284#3969593, @Michael137 wrote:

In D127284#3969458, @v.g.vassilev wrote:

In D127284#3969446, @v.g.vassilev wrote:

UPDATE: fails with clean build too

Might be best to revert it for now while we figure out what's wrong

I fail to reproduce it, can you give me access to some node where I can debug? I suspect that should be something easy to fix.

UPDATE: Reproduced it. Debugging.

@Michael137, I think I understand what happens. This patch introduces a new language option which is not benign from modules perspective. However, lldb sets up modules the old way and then switches to the incremental processing mode. We have two ways to fix this:

• Pass -Xclang -fincremental-extensions in ClangModulesDeclVendor.cpp and delete instance->getPreprocessor().enableIncrementalProcessing();; or
• Apply https://reviews.llvm.org/D139258 which already does that.

Can you check if https://reviews.llvm.org/D139258 passes the lldb testsuite. It does pass my reproduction by hand (I still cannot run full of the lldb-api tests)?

Thanks for taking a look. I tried with the suggested patch on the Objective-C API tests and there's only 1 test failure remaining there:

lang/objc/modules-compile-error/TestModulesCompileError.py

Expecting sub string: "module.h:4:1: error: unknown type name 'syntax_error_for_lldb_to_find'" (was not found)

The test expects an error string that looks like unknown type name 'syntax_error_for_lldb_to_find' but gets use of 'undeclared identifier 'syntax_error_for_lldb_to_find'

Haven't looked much further than that. Does that sound familiar to you? I see a similar test fix as part of this patch. Maybe it's just a matter of fixing up the expected string.

I think we should revert for now until https://reviews.llvm.org/D139258 is ready to go. Just to unblock the buildbot

I have pushed a fix in https://github.com/llvm/llvm-project/commit/c95a0c91c0de66eb1066f23c69332522656f188e That should unblock the bot. If that does not work, I will revert.

[Michael137]

In D127284#3969975, @v.g.vassilev wrote:

In D127284#3969593, @Michael137 wrote:

In D127284#3969458, @v.g.vassilev wrote:

In D127284#3969446, @v.g.vassilev wrote:

UPDATE: fails with clean build too

Might be best to revert it for now while we figure out what's wrong

I fail to reproduce it, can you give me access to some node where I can debug? I suspect that should be something easy to fix.

UPDATE: Reproduced it. Debugging.

@Michael137, I think I understand what happens. This patch introduces a new language option which is not benign from modules perspective. However, lldb sets up modules the old way and then switches to the incremental processing mode. We have two ways to fix this:

• Pass -Xclang -fincremental-extensions in ClangModulesDeclVendor.cpp and delete instance->getPreprocessor().enableIncrementalProcessing();; or
• Apply https://reviews.llvm.org/D139258 which already does that.

Can you check if https://reviews.llvm.org/D139258 passes the lldb testsuite. It does pass my reproduction by hand (I still cannot run full of the lldb-api tests)?

Thanks for taking a look. I tried with the suggested patch on the Objective-C API tests and there's only 1 test failure remaining there:

lang/objc/modules-compile-error/TestModulesCompileError.py

Expecting sub string: "module.h:4:1: error: unknown type name 'syntax_error_for_lldb_to_find'" (was not found)

The test expects an error string that looks like unknown type name 'syntax_error_for_lldb_to_find' but gets use of 'undeclared identifier 'syntax_error_for_lldb_to_find'

Haven't looked much further than that. Does that sound familiar to you? I see a similar test fix as part of this patch. Maybe it's just a matter of fixing up the expected string.

I think we should revert for now until https://reviews.llvm.org/D139258 is ready to go. Just to unblock the buildbot

I have pushed a fix in https://github.com/llvm/llvm-project/commit/c95a0c91c0de66eb1066f23c69332522656f188e That should unblock the bot. If that does not work, I will revert.

Latest test run: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48911/execution/node/74/log/
I modified the expected string slightly to make it work: https://reviews.llvm.org/rG811ad246ac7b
Should be all good now. Thanks for looking into this!

[v.g.vassilev]

Latest test run: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/48911/execution/node/74/log/
I modified the expected string slightly to make it work: https://reviews.llvm.org/rG811ad246ac7b
Should be all good now. Thanks for looking into this!

Thanks a lot for pinging! Glad that it worked with a relatively little number of iterations considering the nature of this patch ;)

[UnionType]

clang/test/Interpreter/execute-stmts.cpp

35

It seems that top-level statements wrapped in braces cannot be handled, maybe I made a mistake.

for (; i > 4; --i) { printf("i = %d\n", i); }

{ i++; }

[v.g.vassilev]

clang/test/Interpreter/execute-stmts.cpp

35

Ah, you are right. I have a fix for this case here: https://reviews.llvm.org/D139798

[UnionType]

@v.g.vassilev Thanks for your reply, the patch is clean and clear.

[tahonermann]

This change appears to have negatively impacted some users that were dependent on the previous Preprocessing::enableIncrementalProcessing(true) behavior. See https://github.com/llvm/llvm-project/issues/62413.

[v.g.vassilev]

In D127284#4303390, @tahonermann wrote:

This change appears to have negatively impacted some users that were dependent on the previous Preprocessing::enableIncrementalProcessing(true) behavior. See https://github.com/llvm/llvm-project/issues/62413.

Thank you for the ping. I’ve replied there.

clang/include/clang/AST/ASTConsumer.h

 //===--- ASTConsumer.h - Abstract interface for reading ASTs ----*- C++ -*-===//
 //
 // 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 defines the ASTConsumer class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_AST_ASTCONSUMER_H
 #define LLVM_CLANG_AST_ASTCONSUMER_H
 
+namespace llvm {
+  template <typename T> class SmallVectorImpl;
+}
+
 namespace clang {
   class ASTContext;
   class CXXMethodDecl;
   class CXXRecordDecl;
   class Decl;
   class DeclGroupRef;
   class ASTMutationListener;
   class ASTDeserializationListener; // layering violation because void* is ugly
   class SemaConsumer; // layering violation required for safe SemaConsumer
+  class Stmt;
   class TagDecl;
   class VarDecl;
   class FunctionDecl;
   class ImportDecl;
 
 /// ASTConsumer - This is an abstract interface that should be implemented by
 /// clients that read ASTs.  This abstraction layer allows the client to be
 /// independent of the AST producer (e.g. parser vs AST dump file reader, etc).
 class ASTConsumer {
   /// Whether this AST consumer also requires information about
   /// semantic analysis.
   bool SemaConsumer;
 
   friend class SemaConsumer;
 
 public:
   ASTConsumer() : SemaConsumer(false) { }
 
   virtual ~ASTConsumer() {}
 
   /// Initialize - This is called to initialize the consumer, providing the
   /// ASTContext.
   virtual void Initialize(ASTContext &Context) {}
 
+  /// HandleTopLevelStmts - Handle the specified top-level statements. This is
+  /// called by the parser to process every top-level Stmt* in incremental
+  /// compilation mode.
+  ///
+  /// \returns true to continue parsing, or false to abort parsing.
+  virtual bool HandleTopLevelStmts(const llvm::SmallVectorImpl<Stmt*>& Stmts) {
+    return true;
+  }
+
   /// HandleTopLevelDecl - Handle the specified top-level declaration.  This is
   /// called by the parser to process every top-level Decl*.
   ///
   /// \returns true to continue parsing, or false to abort parsing.
   virtual bool HandleTopLevelDecl(DeclGroupRef D);
 
   /// This callback is invoked each time an inline (method or friend)
   /// function definition in a class is completed.

clang/include/clang/Parse/Parser.h

public:
   // different actual classes based on the actions in place.
   typedef OpaquePtr<DeclGroupRef> DeclGroupPtrTy;
   typedef OpaquePtr<TemplateName> TemplateTy;
 
   typedef SmallVector<TemplateParameterList *, 4> TemplateParameterLists;
 
   typedef Sema::FullExprArg FullExprArg;
 
+  /// A SmallVector of statements, with stack size 32 (as that is the only one
+  /// used.)
+  typedef SmallVector<Stmt*, 32> StmtVector;
+
+
   // Parsing methods.
 
   /// Initialize - Warm up the parser.
   ///
   void Initialize();
 
   /// Parse the first top-level declaration in a translation unit.
   bool ParseFirstTopLevelDecl(DeclGroupPtrTy &Result,
-                              Sema::ModuleImportState &ImportState);
+                              Sema::ModuleImportState &ImportState,
+                              StmtVector *Stmts = nullptr);
 
   /// ParseTopLevelDecl - Parse one top-level declaration. Returns true if
   /// the EOF was encountered.
   bool ParseTopLevelDecl(DeclGroupPtrTy &Result,
-                         Sema::ModuleImportState &ImportState);
+                         Sema::ModuleImportState &ImportState,
+                         StmtVector *Stmts = nullptr);
   bool ParseTopLevelDecl() {
     DeclGroupPtrTy Result;
     Sema::ModuleImportState IS = Sema::ModuleImportState::NotACXX20Module;
     return ParseTopLevelDecl(Result, IS);
   }
 
   /// ConsumeToken - Consume the current 'peek token' and lex the next one.
   /// This does not work with special tokens: string literals, code completion,
private:
   ExprResult ParseAssignmentExprWithObjCMessageExprStart(
       SourceLocation LBracloc, SourceLocation SuperLoc,
       ParsedType ReceiverType, Expr *ReceiverExpr);
   bool ParseObjCXXMessageReceiver(bool &IsExpr, void *&TypeOrExpr);
 
   //===--------------------------------------------------------------------===//
   // C99 6.8: Statements and Blocks.
 
-  /// A SmallVector of statements, with stack size 32 (as that is the only one
-  /// used.)
-  typedef SmallVector<Stmt*, 32> StmtVector;
   /// A SmallVector of expressions, with stack size 12 (the maximum used.)
   typedef SmallVector<Expr*, 12> ExprVector;
   /// A SmallVector of types.
   typedef SmallVector<ParsedType, 12> TypeVector;
 
   StmtResult
   ParseStatement(SourceLocation *TrailingElseLoc = nullptr,
                  ParsedStmtContext StmtCtx = ParsedStmtContext::SubStmt);

clang/lib/CodeGen/CodeGenAction.cpp

bool HandleTopLevelDecl(DeclGroupRef D) override {
       if (TimerIsEnabled) {
         LLVMIRGenerationRefCount -= 1;
         if (LLVMIRGenerationRefCount == 0)
           LLVMIRGeneration.stopTimer();
       }
 
       return true;
     }
+    bool HandleTopLevelStmts(const llvm::SmallVectorImpl<Stmt*> &Stmts) override {
+      return Gen->HandleTopLevelStmts(Stmts);
+    }
 
     void HandleInlineFunctionDefinition(FunctionDecl *D) override {
       PrettyStackTraceDecl CrashInfo(D, SourceLocation(),
                                      Context->getSourceManager(),
                                      "LLVM IR generation of inline function");
       if (TimerIsEnabled)
         LLVMIRGeneration.startTimer();
 

clang/lib/CodeGen/ModuleBuilder.cpp

public:
 
     void HandleCXXStaticMemberVarInstantiation(VarDecl *VD) override {
       if (Diags.hasErrorOccurred())
         return;
 
       Builder->HandleCXXStaticMemberVarInstantiation(VD);
     }
 
+    bool HandleTopLevelStmts(const llvm::SmallVectorImpl<Stmt*> &Stmts) override {
+      if (Diags.hasErrorOccurred())
+        return false; // Abort parsing.
+
+      // FIXME: We should reimplement this in a proper way where we append the
+      // statements to the global init function.
+      static unsigned id = 0;
+      // Make sure to emit all elements of a Decl.
+      for (Stmt *S : Stmts) {
+        Expr *E = cast<Expr>(S);
+        IdentifierInfo* name = &Ctx->Idents.get("_stmt" + std::to_string(id++));
+        VarDecl *VD = VarDecl::Create(*Ctx, Ctx->getTranslationUnitDecl(),
+                                      E->getBeginLoc(), SourceLocation(),
+                                      name, E->getType(), /*TSI*/nullptr,
+                                      SC_None);
+        VD->setInit(E);
+        HandleTopLevelDecl(DeclGroupRef(VD));
+      }
+
+      return true;
+    }
+
     bool HandleTopLevelDecl(DeclGroupRef DG) override {
+      // FIXME: Why not return false and abort parsing?
       if (Diags.hasErrorOccurred())
         return true;
 
       HandlingTopLevelDeclRAII HandlingDecl(*this);
 
       // Make sure to emit all elements of a Decl.
       for (DeclGroupRef::iterator I = DG.begin(), E = DG.end(); I != E; ++I)
         Builder->EmitTopLevelDecl(*I);

clang/lib/Interpreter/IncrementalParser.cpp

if (P->getCurToken().is(tok::eof)) {
     S.CurContext = nullptr;
     // Start a new PTU.
     P->EnterScope(Scope::DeclScope);
     S.ActOnTranslationUnitScope(P->getCurScope());
   }
 
   Parser::DeclGroupPtrTy ADecl;
   Sema::ModuleImportState ImportState;
-  for (bool AtEOF = P->ParseFirstTopLevelDecl(ADecl, ImportState); !AtEOF;
-       AtEOF = P->ParseTopLevelDecl(ADecl, ImportState)) {
+  Parser::StmtVector Stmts;
+  for (bool AtEOF = P->ParseFirstTopLevelDecl(ADecl, ImportState, &Stmts);
+       !AtEOF; AtEOF = P->ParseTopLevelDecl(ADecl, ImportState, &Stmts)) {
     // If we got a null return and something *was* parsed, ignore it.  This
     // is due to a top-level semicolon, an action override, or a parse error
     // skipping something.
+    if (!Stmts.empty() && !Consumer->HandleTopLevelStmts(Stmts))
+      return llvm::make_error<llvm::StringError>("Parsing failed. "
+                                                 "The consumer rejected a stmt",
+                                                 std::error_code());
+
     if (ADecl && !Consumer->HandleTopLevelDecl(ADecl.get()))
       return llvm::make_error<llvm::StringError>("Parsing failed. "
                                                  "The consumer rejected a decl",
                                                  std::error_code());
   }
 
   DiagnosticsEngine &Diags = getCI()->getDiagnostics();
   if (Diags.hasErrorOccurred()) {

clang/lib/Parse/ParseDecl.cpp

case tok::annot_cxxscope: {
 
       // Check whether this is a constructor declaration. If we're in a
       // context where the identifier could be a class name, and it has the
       // shape of a constructor declaration, process it as one.
       if ((DSContext == DeclSpecContext::DSC_top_level ||
            DSContext == DeclSpecContext::DSC_class) &&
           Actions.isCurrentClassName(*Next.getIdentifierInfo(), getCurScope(),
                                      &SS) &&
           isConstructorDeclarator(/*Unqualified*/ false))
         goto DoneWithDeclSpec;
 
       // C++20 [temp.spec] 13.9/6.
       // This disables the access checking rules for function template explicit
       // instantiation and explicit specialization:
       // - `return type`.
       SuppressAccessChecks SAC(*this, IsTemplateSpecOrInst);
 
case tok::kw__Atomic:
     return true;
   }
 }
 
 /// isDeclarationSpecifier() - Return true if the current token is part of a
 /// declaration specifier.
 ///
 /// \param DisambiguatingWithExpression True to indicate that the purpose of
 /// this check is to disambiguate between an expression and a declaration.
 bool Parser::isDeclarationSpecifier(bool DisambiguatingWithExpression) {
   switch (Tok.getKind()) {
   default: return false;
 
+  // ObjC
+  case tok::at:
+    return getLangOpts().ObjC;
+
+  // OpenCL
+  case tok::kw___kernel:
+    return getLangOpts().OpenCL;
+
   // OpenCL 2.0 and later define this keyword.
   case tok::kw_pipe:
     return getLangOpts().OpenCL &&
            getLangOpts().getOpenCLCompatibleVersion() >= 200;
 
+  case tok::kw_asm:
+    return true;
+
   case tok::identifier:   // foo::bar
     // Unfortunate hack to support "Class.factoryMethod" notation.
     if (getLangOpts().ObjC && NextToken().is(tok::period))
       return false;
     if (TryAltiVecVectorToken())
       return true;
     LLVM_FALLTHROUGH;
   case tok::kw_decltype: // decltype(T())::type
bool Parser::isConstructorDeclarator(bool IsUnqualified, bool DeductionGuide) {
 
   // Current class name must be followed by a left parenthesis.
   if (Tok.isNot(tok::l_paren)) {
     TPA.Revert();
     return false;
   }
   ConsumeParen();
 
+  // FIXME: this returns true for NS::f();
   // A right parenthesis, or ellipsis followed by a right parenthesis signals
   // that we have a constructor.
   if (Tok.is(tok::r_paren) ||
       (Tok.is(tok::ellipsis) && NextToken().is(tok::r_paren))) {
     TPA.Revert();
     return true;
   }
 
   // A C++11 attribute here signals that we have a constructor, and is an
   // attribute on the first constructor parameter.
   if (getLangOpts().CPlusPlus11 &&
       isCXX11AttributeSpecifier(/*Disambiguate*/ false,
                                 /*OuterMightBeMessageSend*/ true)) {
     TPA.Revert();
     return true;

clang/lib/Parse/ParseTentative.cpp

 using namespace clang;
 
 /// isCXXDeclarationStatement - C++-specialized function that disambiguates
 /// between a declaration or an expression statement, when parsing function
 /// bodies. Returns true for declaration, false for expression.
 ///
 ///         declaration-statement:
 ///           block-declaration
+///           template-declaration
 ///
 ///         block-declaration:
 ///           simple-declaration
 ///           asm-definition
 ///           namespace-alias-definition
 ///           using-declaration
 ///           using-directive
 /// [C++0x]   static_assert-declaration
 ///
 ///         asm-definition:
 ///           'asm' '(' string-literal ')' ';'
 ///
 ///         namespace-alias-definition:
 ///           'namespace' identifier = qualified-namespace-specifier ';'
 ///
 ///         using-declaration:
 ///           'using' typename[opt] '::'[opt] nested-name-specifier
 ///                 unqualified-id ';'
 ///           'using' '::' unqualified-id ;
 ///
 ///         using-directive:
 ///           'using' 'namespace' '::'[opt] nested-name-specifier[opt]
 ///                 namespace-name ';'
 ///
+///         template-declaration:
+///           'template' declaration
+///           'template' concept-definition
+///
 bool Parser::isCXXDeclarationStatement() {
   switch (Tok.getKind()) {
-    // asm-definition
+  // OpenCL
+  case tok::kw___kernel:
+    return getLangOpts().OpenCL;
+
   case tok::kw_asm:
+  case tok::kw_export:
+  case tok::kw_import:
     // namespace-alias-definition
+  case tok::kw_inline: // inline namespace
   case tok::kw_namespace:
+    // template-declaration
+  case tok::kw_template:
     // using-declaration
     // using-directive
   case tok::kw_using:
     // static_assert-declaration
   case tok::kw_static_assert:
   case tok::kw__Static_assert:
     return true;
+
+  case tok::identifier:
+    if (getLangOpts().CPlusPlus) {
+      // FIXME: This is quite broken as it considers f() from void f(){}; f();
+      // to be a constructor declarator...
+      if (isConstructorDeclarator(/*Unqualified=*/false))
+        return true;
+      // Check if this is a dtor.
+      if (!TryAnnotateTypeOrScopeToken() && Tok.is(tok::annot_cxxscope) &&
+          NextToken().is(tok::tilde))
+        return true;
+    }
     // simple-declaration
+    LLVM_FALLTHROUGH;
   default:
     return isCXXSimpleDeclaration(/*AllowForRangeDecl=*/false);
   }
 }
 
 /// isCXXSimpleDeclaration - C++-specialized function that disambiguates
 /// between a simple-declaration or an expression-statement.
 /// If during the disambiguation process a parsing error is encountered,
Parser::isCXXDeclarationSpecifier(Parser::TPResult BracedCastResult,
   case tok::kw___interface:
     // enum-specifier
   case tok::kw_enum:
     // cv-qualifier
   case tok::kw_const:
   case tok::kw_volatile:
     return TPResult::True;
 
+    // ObjC
+  case tok::at:
+    if (getLangOpts().ObjC)
+      return TPResult::True;
+    LLVM_FALLTHROUGH;
+
     // OpenCL address space qualifiers
   case tok::kw_private:
     if (!getLangOpts().OpenCL)
       return TPResult::False;
     LLVM_FALLTHROUGH;
   case tok::kw___private:
   case tok::kw___local:
   case tok::kw___global:

clang/lib/Parse/Parser.cpp

 /// [C]     external-declaration
 /// [C]     translation-unit external-declaration
 /// [C++]   top-level-declaration-seq[opt]
 /// [C++20] global-module-fragment[opt] module-declaration
 ///                 top-level-declaration-seq[opt] private-module-fragment[opt]
 ///
 /// Note that in C, it is an error if there is no first declaration.
 bool Parser::ParseFirstTopLevelDecl(DeclGroupPtrTy &Result,
-                                    Sema::ModuleImportState &ImportState) {
+                                    Sema::ModuleImportState &ImportState,
+                                    StmtVector *Stmts) {
   Actions.ActOnStartOfTranslationUnit();
 
   // For C++20 modules, a module decl must be the first in the TU.  We also
   // need to track module imports.
   ImportState = Sema::ModuleImportState::FirstDecl;
-  bool NoTopLevelDecls = ParseTopLevelDecl(Result, ImportState);
+  bool NoTopLevelDecls = ParseTopLevelDecl(Result, ImportState, Stmts);
 
   // C11 6.9p1 says translation units must have at least one top-level
   // declaration. C++ doesn't have this restriction. We also don't want to
   // complain if we have a precompiled header, although technically if the PCH
   // is empty we should still emit the (pedantic) diagnostic.
   // If the main file is a header, we're only pretending it's a TU; don't warn.
   if (NoTopLevelDecls && !Actions.getASTContext().getExternalSource() &&
       !getLangOpts().CPlusPlus && !getLangOpts().IsHeaderFile)
     Diag(diag::ext_empty_translation_unit);
 
   return NoTopLevelDecls;
 }
 
 /// ParseTopLevelDecl - Parse one top-level declaration, return whatever the
 /// action tells us to.  This returns true if the EOF was encountered.
 ///
 ///   top-level-declaration:
 ///           declaration
 /// [C++20]   module-import-declaration
 bool Parser::ParseTopLevelDecl(DeclGroupPtrTy &Result,
-                               Sema::ModuleImportState &ImportState) {
+                               Sema::ModuleImportState &ImportState,
+                               StmtVector *Stmts /*=nullptr*/) {
   DestroyTemplateIdAnnotationsRAIIObj CleanupRAII(*this);
 
   // Skip over the EOF token, flagging end of previous input for incremental
   // processing
   if (PP.isIncrementalProcessingEnabled() && Tok.is(tok::eof))
     ConsumeToken();
 
   Result = nullptr;
bool Parser::ParseTopLevelDecl(DeclGroupPtrTy &Result,
 
   default:
     break;
   }
 
   ParsedAttributes attrs(AttrFactory);
   MaybeParseCXX11Attributes(attrs);
 
+  // FIXME: Remove the incremental processing pre-condition and verify clang
+  // still can pass its test suite, which will harden `isDeclarationStatement`.
+  // It is known to have several weaknesses, for example in
+  // isConstructorDeclarator, infinite loop in c-index-test, etc..
+  // Parse a block of top-level-stmts.
+  while (PP.isIncrementalProcessingEnabled() && Stmts &&
+         !isDeclarationStatement()) {
+    //isStmtExpr ? ParsedStmtContext::InStmtExpr : ParsedStmtContext()
+    ParsedStmtContext SubStmtCtx = ParsedStmtContext();
+    auto R = ParseStatementOrDeclaration(*Stmts, SubStmtCtx);
+    if (!R.isUsable())
+      return true;
+    Stmts->push_back(R.get());
+    if (Tok.is(tok::eof))
+      return false;
+  }
+
   Result = ParseExternalDeclaration(attrs);
   // An empty Result might mean a line with ';' or some parsing error, ignore
   // it.
   if (Result) {
     if (ImportState == Sema::ModuleImportState::FirstDecl)
       // First decl was not modular.
       ImportState = Sema::ModuleImportState::NotACXX20Module;
     else if (ImportState == Sema::ModuleImportState::ImportAllowed)
case tok::kw_module:
     return nullptr;
 
   default:
   dont_know:
     if (Tok.isEditorPlaceholder()) {
       ConsumeToken();
       return nullptr;
     }
     // We can't tell whether this is a function-definition or declaration yet.
     return ParseDeclarationOrFunctionDefinition(Attrs, DS);
   }
 
   // This routine returns a DeclGroup, if the thing we parsed only contains a
   // single decl, convert it now.
   return Actions.ConvertDeclToDeclGroup(SingleDecl);
 }
 
 /// Determine whether the current token, if it occurs after a
 /// declarator, continues a declaration or declaration list.
 bool Parser::isDeclarationAfterDeclarator() {
   // Check for '= delete' or '= default'
   if (getLangOpts().CPlusPlus && Tok.is(tok::equal)) {

clang/test/Interpreter/execute-stmts.cpp

+// REQUIRES: host-supports-jit
+// UNSUPPORTED: system-aix
+// RUN: cat %s | clang-repl | FileCheck %s
+
+int i = 12;
+++i;
+extern "C" int printf(const char*,...);
+printf("i = %d\n", i);
+// CHECK: i = 13
+
+// FIXME: isConstructorDeclarator thinks these are ctors:
+// namespace Ns { void f(){} }
+// Ns::f();
+
+// void g() {}
+// g();
+
+quit