I landed the fix to unbreak our crashes and explored a bit of the alternative solution.

• Add a test for block pointers

In D146426#4207423, @shafik wrote:

As I noted in the bug report not doing D.setInvalidType(); does fix this bug and seems harmless since the error diagnostic should prevent us from getting to codegen but it is not clear to me if this has other negative impacts. Reading your replies it is not obvious you looked at this path or not.

In D146426#4207118, @aaron.ballman wrote:

This feels like it's heading in the wrong direction -- the AST should not have holes in it. An invalid type should be replaced by a valid type (after diagnosing the invalid type, of course) so that we can keep as much of the AST around as possible (for example, we typically stub in int and continue compilation, as in: https://godbolt.org/z/MvxjGovGh), which should then result in a non-null ParmVarDecl. This way, we don't need to sprinkle nullptr checks all over the compiler when inspecting a function's parameters.

• rename test file to GH61441.cpp, add a comment this checks for no crash
• Update outdated comment

In D146426#4206519, @shafik wrote:

Please edit the summary to indicate this fixes: https://github.com/llvm/llvm-project/issues/61441
I am happy someone had more time to dig into this problem after my initial investigation.

I suspect D145641 won't land today in the EU working hours.
My suggestion would be to revert this patch and reland together with D145641.
This should be in line with @ChuanqiXu's suggestions to wait for a response today.

In D145639#4198794, @ChuanqiXu wrote:

Reverting is common in Clang/LLVM. But let's wait for the response from @bruno temporarily. I think it makes sense to revert this one if @bruno don't response tomorrow. And we can commit this one and D145641 quickly the next time. Recommit is common too.

@bgraur, FYI, hope waiting for a day works for this case.

Reverting the RVO breaks some coroutine code we have. The short repro is https://gcc.godbolt.org/z/1543qc8Ee (code at the end of the post, very similar to the deleted constructor in warn-throw-out-noexcept-coro.cpp):
The RVO seems to be mandated by the standard and D145641 is in the works to fix this.

I don't have a lot of experience in codegen, so will let Aaron and Richard do the review.

In D142384#4101461, @ChuanqiXu wrote:

It should be good to prevent crashes. But it looks not good that it doesn't have a test. Do you have plans to add a test case for this soon?

In D142384#4098650, @usaxena95 wrote:

I think this is fine, and we should just use the definition when it is available without asking the callers to request fields only when definition is available.

I don't see any issues with this, so happy to LGTM. @rsmith any concerns on your side?
@usaxena95 could you give an example of the code that fails the assertion? Is it some of the tests?

This fix looks very sensible to me and @rsmith confirmed this pattern that we are seeing might happen (seeing members of something that was demoted from declaration to a definition).
@rsmith could you confirm the update version looks good to you?

In D142446#4078204, @dblaikie wrote:

Alternatively, how bad would it be if aggregate initialization were removed and these objects were initialized with ctor calls? (bad for an -O0 build, no doubt... (how bad?) calling all those ctors to build the tables, but does clang optimize all the ctor calls away to the equivalent of aggregate init when using optimizations?)

Ah, I keep forgetting that the unknown attributes are fine. My experiments with MSVC suggest that it will increase the struct size: https://gcc.godbolt.org/z/offYrhn4K.
libc++ uses msvc::no_unique_address, which seems to work, but is MSVC-specific.

For the record, the corresponding standard change is P1008R1.

To describe the problem, the following code breaks in C++20:

struct WantAggregate {
  WantAggregate(const WantAggregate&) = delete;
  int a;
  int b;
};

In D142214#4076245, @foad wrote:

Interesting. Is there any easy way I can test building in C++20 mode on my own machine?

Randomly chiming in here.
I never had a good model of where CorrectDelayedTyposInExpr, but wanted to note that ActOnFullExpr also calls it. This may be fine, I just wanted to mention it as it stood out.

LGTM

Slight NIT: we might want to initialize End and iterator_adaptor_base::I with WrappedIteratorT End = WrappedIteratorT() to avoid uninitialized data members when WrappedIteratorT is a pointer.
This does not happen in practice right now, but if someone actually starts using this constructor in the future, we will at least not have UB.

Sorry, didn't see your comment when submitting mine.

The change seems reasonable. Could you add a test with a crasher repro and some validation of the generated IR?

LGTM! Looks much cleaner now, BTW.

LGTM, would be happy to get those emails.

In D140547#4050752, @uabelho wrote:

Anyone else see this?

I have not checked, but I would not be surprised if we hit the stack size limits with asan enabled
@usaxena95, maybe reduce the number of instantiations from 10001 to 1001 or 101? It should not change the intention of the test and fix this failure.

There is potentially a way to move less code by keeping all declarations at place and only moving bodies of definitions of constructors and destructors to the .cpp file.
Not sure what's preferable (less code moves vs more functions inline in the header), @kadircet do you have an opinion?

LGTM, mostly based on the previous review.
I have not noticed any significant changes, so have not looked too cautiously. Let me know if there are any particular places that you someone to take a look at before landing.

LGTM, but please address the NITs before submitting.

LGTM, thanks for fixing this!

Are there any uses of u8 string literals with formatted_raw_ostream outside tests? What was the effect on the final behavior here?

LGTM. The presubmit failures are definitely unrelated as they are in a lit test for a different tool.

In D141358#4039265, @royjacobson wrote:

It's dr2237, see https://github.com/llvm/llvm-project/issues/55164

LGTM to unbreak the buildbots, this change definitely does not change semantics.

I think the only major problem is not checking for error case when accessing TransReq, the rest are NITs.
Thanks for the change! Will be happy to LGTM it as soon as the access to TransReq is fixed.

Should access checks should happen in the context where concept is written or where it's used? Is there a standard wording around it?
If access-checking should happen where concept is defined, having a hard error seems fine because of the wording you quoted:

If the substitution of template arguments into a requirement would always result in a substitution failure, the program is ill-formed; no diagnostic required.

The program is ill-formed and we show the diagnostic even though it's not required.

LGTM to unbreak clangd, this seems to pop up a lot for Chrome developers.

• Use kubernetes resource YAML for deployment instead of terraform config. I had problems updating our terraform deployment, it seems broken. I will follow up by moving existing clangd buildbot out of terrafrom too, the kubectl-based workflow seems prefereable.
• Set version of the new docker image to 0 (script updates it on build).

LGTM

• fix a typo in /vol/ccache
• use password from clangd worker (approved by Galina in our email exchange)
• leave out the targets and checks, use defaults that build and test everything
• Use $CC and $LD instead of the wrong compilers in run.sh

Sending this out early to have it reviewed by the time I get the required credentials.
This will also require the corresponding configuration changes in our GCP changes before being submitted.

LGTM! Thanks a lot for driving this to conclusion! Please note that there is one small NIT you may want to fix before landing this.
UB definitely explains why we saw the bugs. It's good that we caught it before this landed.

Since the errors only shows up in modular builds, I would look closely for bugs inside ASTReader/ASTWriter.
Also, it seems that ArrayFiller is not taken in to account in computeDependence and maybe it should be. I am not 100% sure, though: if ArrayFiller is only used for non-dependent code, it should not case this bug. It also does not explain the variation between modular and non-modular builds.

This LG, but not accepting as I believe libc++ is still broken if built with modules, right?
I have run check-cxx locally and it seems to work, but I suspect that's not using modules by default. I have clicked through the links in phrabricator and the errors seem to come from the latest uploaded diff.
Let me know if I'm missing something.

LGTM. BTW, feel free to send small changes like these without review. The common jargon for them in LLVM is NFC (non-functional change)

I have sent a follow-up NFC change to fix a typo in the documentation of --ranking-model=heuristics. Note that I decided to avoid changing the order of options for --ranking-model in the documentation, this didn't seem important.

• Add a comment to CMakelists.txt explaining motivation for the option
• Add #include "Feature.h"
• Revert changes to the order of compiler flags to keep this change more focused
• clang-format
• replace #ifndef with #if ! in ClangdMain.cpp

Thanks, LGTM!

I have two major comments and also inline NITs. Not sure if we should block on those, just wanted to hear your opinions:

• InitListExpr and CXXParenInitListExpr have some code in common. Code duplication is substantial and I think sharing the common implementation is warranted. E.g. if some code would want to change something with ArrayFiller or make a use of it, the work will probably need to be duplicated. To reiterate what I mentioned earlier, I believe deriving CXXParenInitListExpr from InitListExpr is not a very good option, but we might explore other possibilities: a base class or factoring out common code in a separate struct. I believe this could be done with a follow-up change, though, as the amount of changes required does not seem too big, I would be happy with first landing this patch and then cleaning up with a follow-up change.
• Did we explore the possibility of modelling this differently and somehow avoiding making CXXParenInitListExpr an expression. This seems to borrow from InitListExpr, but maybe for the wrong reasons. Braced initializers can actually occur in expressions positions, e.g. int a = {123}. However, CXXParenInistListExpr cannot occur in expression positions outside initializations, e.g. having CXXFunctionalCastExpr for aggregate_struct(123) feels somewhat wrong to me and seems to lead to confusing error messages too. Maybe we should model it as a new expression that contains both the type and the argument list? I.e. more similar to CXXConstructExpr?

• Merge DecisionForest.cpp and *_disabled.cpp, change #ifndef to #if
• Add decision_forest to the feature string
• Set Heuristics as the default code completion model in tests
• Do not compile decision forest tests when it's disabled
• Remove the use of preprocessor for defining the default value of -ranking-model flag to clangd

This seems to build in both modes, but I still need to figure out what to do with tests with decision forest off.
Also, maybe there is a way to minimize the use of preprocessor even more.

LGTM with a few NITs. Thanks!

Thanks for the explanation. LGTM! And thanks for adding an assert.

Thanks for addressing the comments and sorry for a wait before the comments.
Please see the comment about syntactic form, I might be holding it wrong, but it looks like we actually loose the syntactic form completely in this case.