This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by v.g.vassilev on Jun 8 2022, 3:13 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
v.g.vassilev edited the summary of this revision. (Show Details)

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

v.g.vassilev added inline comments.Aug 1 2022, 10:28 AM
clang/lib/Parse/ParseDecl.cpp
3452–3454

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?

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

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 ↗(On Diff #449066)

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
468

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
5290–5294

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

clang/lib/Parse/ParseDecl.cpp
5685–5692

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
54
60

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

61

Do you need special handling for:

struct S {
  operator int();
};

S::operator int() { return 0; }
clang/lib/Parse/Parser.cpp
737–738

How close are we to having this fixme fixed?

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

Why is this not covered by the REQUIRES above?

29–34

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 marked 9 inline comments as done.

Address review comments

v.g.vassilev added inline comments.
clang/include/clang/AST/ASTConsumer.h
54–59 ↗(On Diff #449066)

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

clang/include/clang/Parse/Parser.h
468

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

clang/lib/CodeGen/CodeGenModule.cpp
5290–5294

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
5685–5692

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

clang/lib/Parse/ParseTentative.cpp
54

IIUC, clang models the ; as a NullStmt.

60

That's a good point.

61

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

clang/lib/Parse/Parser.cpp
737–738

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
3

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).

29–34

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 added inline comments.Sep 6 2022, 8:09 AM
clang/include/clang/AST/ASTConsumer.h
54–59 ↗(On Diff #449066)

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
5290–5294

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
54

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

clang/lib/Parse/Parser.cpp
737–738

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

clang/test/Interpreter/disambiguate-decl-stmt.cpp
29–34

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 added a comment.Sep 7 2022, 5:23 PM

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.

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).

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 marked 4 inline comments as done.

Address comments.

v.g.vassilev edited the summary of this revision. (Show Details)Nov 1 2022, 12:54 AM
v.g.vassilev edited the summary of this revision. (Show Details)

Rebase.

clang/test/Interpreter/disambiguate-decl-stmt.cpp
29–34

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.

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
4258
4271–4276

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

4274

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

4287

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

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

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
5251
5270–5273

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
5282
clang/lib/CodeGen/CodeGenModule.cpp
6358
clang/lib/Parse/ParseDecl.cpp
5395–5396

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

clang/lib/Sema/SemaDecl.cpp
19540

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
3

Still wondering on this bit.

v.g.vassilev marked 21 inline comments as done.

Address more comments.

v.g.vassilev added inline comments.
clang/include/clang/Basic/DeclNodes.td
98

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

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

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

clang/lib/Parse/ParseDecl.cpp
5395–5396

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
3

// 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 marked an inline comment as not done.Nov 2 2022, 12:54 PM

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 added inline comments.Nov 4 2022, 11:37 AM
clang/lib/Parse/ParseDecl.cpp
5690–5691

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
61

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 marked 2 inline comments as done.
v.g.vassilev edited the summary of this revision. (Show Details)

Address comments. Call isCurrentClassName before calling isConstructorDeclarator.

clang/lib/Parse/ParseDecl.cpp
5690–5691

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

clang/lib/Parse/ParseTentative.cpp
61

Ah, okay, that was the missing bit.

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

v.g.vassilev added inline comments.Nov 6 2022, 4:57 AM
clang/lib/Parse/Parser.cpp
1033

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 marked an inline comment as not done.

Rebase + fix the failure on windows

rsmith added inline comments.Nov 15 2022, 11:40 PM
clang/lib/AST/Decl.cpp
5265

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
52–53

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

clang/lib/Parse/Parser.cpp
1033

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 marked 2 inline comments as done.
v.g.vassilev edited the summary of this revision. (Show Details)

Address comments, support better C in clang-repl.

v.g.vassilev marked an inline comment as done.

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

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

Both are now unused.

clang/include/clang/Basic/DeclNodes.td
98

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

clang/include/clang/Lex/Preprocessor.h
1782–1785

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
5250–5251

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

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

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

SGTM, thank you for investigating!

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

clang/lib/CodeGen/CodeGenModule.cpp
6155

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

6162

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

clang/lib/Parse/ParseDecl.cpp
5402

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
1028–1030

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

v.g.vassilev marked 10 inline comments as done.

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

clang/include/clang/Lex/Preprocessor.h
1782–1785

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
6155

Awesome! Thanks!

clang/lib/Parse/ParseDecl.cpp
5402

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

clang/lib/Parse/Parser.cpp
1028–1030

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.

rsmith added inline comments.Nov 28 2022, 2:03 PM
clang/lib/Parse/Parser.cpp
1028–1030

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 marked an inline comment as done.Nov 30 2022, 7:53 AM
v.g.vassilev added inline comments.
clang/lib/Parse/Parser.cpp
1028–1030

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 added inline comments.Nov 30 2022, 11:25 AM
clang/include/clang/Lex/Preprocessor.h
1782–1785

Doing it as a separate commit is fine by me.

clang/lib/Parse/ParseDecl.cpp
5402–5407

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

v.g.vassilev marked 3 inline comments as done.Nov 30 2022, 12:02 PM
v.g.vassilev added inline comments.
clang/lib/Parse/ParseDecl.cpp
5402–5407

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 added inline comments.Dec 1 2022, 5:02 AM
clang/lib/Parse/ParseDecl.cpp
5402–5407

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 marked 2 inline comments as done.

Address comments, fix a fixme.

clang/lib/Parse/ParseDecl.cpp
5402–5407

I took the liberty to address the FIXME.

v.g.vassilev planned changes to this revision.Dec 1 2022, 8:37 AM

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

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

This revision is now accepted and ready to land.Dec 2 2022, 7:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2022, 11:18 PM

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/

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?

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 added a comment.EditedDec 4 2022, 10:49 AM

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

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 added a comment.EditedDec 4 2022, 11:46 AM

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.

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 added a comment.EditedDec 4 2022, 4:47 PM

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

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.

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!

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 added inline comments.Dec 11 2022, 7:48 AM
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 marked an inline comment as done.Dec 11 2022, 1:13 PM
v.g.vassilev added inline comments.
clang/test/Interpreter/execute-stmts.cpp
35

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

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

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 marked an inline comment as done.Apr 27 2023, 10:26 PM

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.