[cling] Unload implicit instantiations also from ASTFile#21043
[cling] Unload implicit instantiations also from ASTFile#21043hahnjo wants to merge 1 commit intoroot-project:masterfrom
Conversation
| if (D->isFromASTFile()) { | ||
| // We cannot ignore instantiated CXXMethodDecl's in templated classes: | ||
| // They may have failing static_assert. | ||
| if (auto* M = dyn_cast<CXXMethodDecl>(D)) { |
There was a problem hiding this comment.
Is that change sufficient for these things to be reinstantiated? In particular, non-static_assert method decls coming from ast files? We should probably add some tests.
There was a problem hiding this comment.
This could be relatively easy, right? If I understand correctly we need a template broghut into memory from a pcm and then 2 instantiations in a row - please correct me if I am wrong.
If the above is correct, and this cannot be put in cling since we need a dictionary, we can comfortably put the test in roottest, maybe even profiting from ROOT helpers that at runtime allow us to generate dictionaries on the fly
There was a problem hiding this comment.
Yes, Dev has a test case (in roottest) for the problem I'm trying to solve. We don't have a (new) test for an unloaded method that does not have a static_assert (I believe that's what Vassil is asking about). It's probably covered by other tests, but I need to check. Moreover there are two failing tests (roottest-cling-reload-reloadvector and roottest-root-tree-selector-selector) that I don't have locally...
There was a problem hiding this comment.
Yes, having tests in cling is better because when we break this it's far easier to debug. The test does not need a dictionary. We need a module file with a static assert declaration.
There was a problem hiding this comment.
We don't have a single test using modules / pcm's in interpreter/cling/test and I'm not going to invent the infrastructure for this PR. The test is going to go into roottest.
There was a problem hiding this comment.
Thank you for being constructive. In that case please hold off merging this PR until I provide you with the infrastructure (which is 3 lines).
There was a problem hiding this comment.
In fact the infrastructure seems to be there already. Please take a look at cling/test/CodeUnloading/PCH/. We should be able to extend some of these tests to cover that case, too.
There was a problem hiding this comment.
I added a Cling test that is fixed by the change. I also had a quick look at roottest-root-tree-selector-selector and it seems that one is failing because it cannot resolve _ZNKSt13__atomic_baseIhEcvhEv = std::__atomic_base<unsigned char>::operator unsigned char() and some related methods. It's probably now unloaded because of previous errors, but not correctly re-instantiated?
There was a problem hiding this comment.
Yes, because the ASTReader has a cache of deserialized declarations. Maybe some parts are in that cache?
Test Results 22 files 22 suites 3d 20h 24m 32s ⏱️ For more details on these failures, see this check. Results for commit 1bad32b. ♻️ This comment has been updated with latest results. |
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
This PR needs the appropriate test in cling. I will provide the infrastructure in the next week or so.
The assumption is that serialized declarations must be valid, however that doesn't hold anymore in the presence of static_assert in templated classes. Fixes root-project#20638
hahnjo
force-pushed
the
cling-unload-implicit-instantiation
branch
from
f83ce07 to
1bad32b
Compare
3 participants
The assumption is that serialized declarations must be valid, however that doesn't hold anymore in the presence of static_assert in templated classes.
Fixes #20638