New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix lazy loading of template specializations #13139
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I believe we need to put a |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Ok, not sure I fully understand what you propose... Can you add this change before the final "fixup" commit? You can push to the PR, right, or do I need to enable this somehow? |
Starting build on |
Build failed on ROOT-debian10-i386/soversion. Failing tests: |
@phsft-bot build just on mac11/cxx17, mac11arm/cxx17, mac12/cxx17, mac12arm/cxx17, mac13/cxx20, mac13arm/cxx20 with flags -DCTEST_TEST_EXCLUDE_NONE=ON |
Starting build on |
Build failed on mac13arm/cxx20. Failing tests: |
Build failed on mac11arm/cxx17. Warnings:
And 126 more Failing tests: |
@phsft-bot build with flags -Druntime_cxxmodules=OFF -DCTEST_TEST_EXCLUDE_NONE=ON |
Starting build on |
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests: |
Build failed on mac12arm/cxx20. Failing tests: |
Build failed on mac11/noimt. Failing tests: |
Starting build on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really a great job, @hahnjo!
For hashing, we should not differentiate between templates and qualified templates and only look at the underlying name. See also https://reviews.llvm.org/D153003
This avoids deserialization if necessary, for example in DeclUnloader. See also https://reviews.llvm.org/D154328
Due to hash collisions, it can happen that we load another template specialization with the same hash. This is fine, as long as the next call to findSpecializationImpl does not find a matching Decl for the template arguments.
The LLVM 13 port of the patch missed to adapt findSpecialization of ClassTemplateDecl and VarTemplateDecl. During the upgrade, we only noticed and fixed FunctionTemplateDecl::findSpecialization.
Starting build on |
Build failed on mac11/noimt. Failing tests: |
No description provided.