Also, I would like to add that the current test present in this diff does not fail in the existing system but it was the best I and Vassil could come up with to replicate our problem.

Is there a way to run something like creduce to come up with a test case that does replicate the issue? I'd like to ensure we have a test case that fails without the patch applied so that we can be sure we don't regress the behavior.

Makes sense to me!

@hubert.reinterpretcast, this patch is part of our internal forks which we would like to put upstream. The author of the previous patch does not have bandwidth to work on it and @reikdas kindly volunteered to take over. I can close the previous one if that's preferable.

https://reviews.llvm.org/D41416 could be relevant. I am not an expert but I think when reading the DWARF you could only register 'lazy' specializations which will be imported only when really required.

ping...

Thanks!

r365153.

Add comments, add missing callbacks. Thanks Richard!

Indeed. IIUC, here is no way clang-cl to load plugins if compiled BUILD_SHARED_LIB=0. It is not clear to me if that's a design limitation or something else. I'd love to be able to load plugins on a 'standard' clang-cl instance.

@chapuni, you have reverted this patch and the commit message says "investigating". Did you come to a conclusion? Can we reland it?

Is this change still relevant?

ping...

Ping...

@rsmith, @bruno could you verify this patch on your codebases?

Add parens.

It looks like the diagnostic options and few others were moved out from the control block in r297655 by Manman Ren. The header files are still part of the control block. The best I can do is buffer that information and print it out last. Is that what we want?

@rtrieu, your comment was along the lines of what I was thinking of. I have updated the patch. Without testing it on large scale, I the current change seems to work for us.

Remove the TypeMap

LGTM!

r332816.

Ok, that's great! Sorry for the delay and thanks for landing a similar patch. Btw, we should probably find a more terse way to test the ODRHash, eg. with unittests.

I am not sure if this is the right time but maybe testing and comparing the behavior of this patch against something such as the ODRHash will help you, me (and probably others) to understand the current patch better.

Add sanity check.

Reading back the patch, we are turning on the bindings by default ;)

Could you double-check if we can add this to an existing test? Would test/SemaCXX/pr25181-crash-on-invalid.cpp fit and potentially rename it.

ping...

ping2...

LGTM!

LGMT! We can rely on post-commit reviews for changes like this one.

LGTM!

Finer grained clang stats can be seen here.

Reduce further the deserializations from 451 to 449 by providing a more complete implementation of ODRHash::AddTemplateArgument.

Reduce hash collisions for template specializations.

+1 Thanks for working on this. Great to see this happening!

Reverse template specialization order.

Zero IsPartial and improve comments in assert and free text.

I have created a simple (-ish) benchmark targeted to stress test modules with a good number of template specializations. I use the mp11 library from boost. I am building two modules which contain around 1K specializations (mostly full specializations). The example can be found here. I use a small fraction of them. This patch deserializes (only) 1117 specializations for around 0.2 seconds whereas without it clang deserializes 1905 specializations for 0.4 s.

Loading of lazy partial template specializations should not trigger loading of full template specializations.

Address comments:

• Fix style;
• Do not potentially deserialize a specialization in debug mode.

Teach ASTReader::CompleteRedeclChain to load only the template specializations with the same template argument list.

Address inline comments: order the read operations.

Fixed a comment typo.

Thanks! Landed in r320763.

Fix preexisting comment typo.

For the record: relanded in r306903.

LGTM!

Reverted in r316153...

Revised patch landed in r316150.

I assume this would fix it:

Reverted in r316029 because the bots got angry at me:

Landed in r316025.

Address comments.

This patch can rely on a post commit review if necessary.

You should probably update the code creating the vfs before the call to createFileManager in lib/Frontend/FrontendAction.cpp.

Add a test case.

@bruno, what's the fate of this?

@rsmith ping...

ping...

Sorry, undo ping... we have a test case.

LGTM! I'd reword the commit message, saying that we intended to add a fast path in DeclContext::removeDecl which checks if the decl is hidden to avoid addition in the lookup tables. That caused problems with modules, because the modules system extends the definition of visibility and this is a test making sure we do not reintroduce such an issue.

ping...

LGTM +ping!

Landed in r311844.

A version of this landed in r311843. I am keeping in mind this discussion and I'd like to follow up with @rjmccall once I open the more major review item (libInterpreter).

Type& name -> Type &name.

std::string& -> llvm::StringRef.

Fix StringRef allocation.