Skip to content

[cling] Unload all redecls of ClassTemplateDecl #19394

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devajithvs
Copy link
Contributor

@devajithvs devajithvs commented Jul 17, 2025

When unloading a ClassTemplateDecl, no redeclarations were handled.

Additional redecls may still be part of the chain leading to duplicates
and broken redecl chains that cause assertion failures like:
"Passed first decl twice, invalid redecl chain!"

Fixes assertion failures post llvm commit:
llvm/llvm-project@17d8ed7
for the test roottest-root-meta-cmsUnload-make

This Pull request:

Changes or fixes:

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #
The failing test roottest-root-meta-cmsUnload-make in #19242

When unloading a ClassTemplateDecl, no redeclarations were handled

Additional redecls may still be part of the chain leading to duplicates
and broken redecl chains that cause assertion failures like:
"Passed first decl twice, invalid redecl chain!"

Fixes assertion failures post llvm commit:
llvm/llvm-project@17d8ed7
for the test `roottest-root-meta-cmsUnload-make`
@devajithvs devajithvs requested a review from dpiparo as a code owner July 17, 2025 11:34
@devajithvs devajithvs self-assigned this Jul 17, 2025
@devajithvs devajithvs requested review from hahnjo and vgvassilev July 17, 2025 11:34
Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance for a test case?

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. In my understanding, this currently leaves the AST in a partially invalid state. I'm not sure if this is easy to test at the moment, but will be exercised by the upgrade.

@devajithvs
Copy link
Contributor Author

devajithvs commented Jul 21, 2025

I've been looking into adding a test case, but it's a bit difficult to test this within the ROOT/Cling testing scope. We could add a simplified test similar to roottest-root-meta-cmsUnload-make, but will need LLVM commit llvm/llvm-project@17d8ed7 to trigger an assertion.

@vgvassilev
Copy link
Member

Ok, any chance in remembering to add the test when that lands. I presume the test case is unloading the test case introduced by the commit you mentioned.

@devajithvs
Copy link
Contributor Author

Ok, any chance in remembering to add the test when that lands.

The test with full commit for reference: 08e304b

I was using a simplified version of this test for debugging. But the above test should cover the case when unloading after the upgrade.

case1.script

#include "inc/Nothing.h"
// gDebug=1;
.L lib/libEdm.so
.L lib/libStrip.so
// step2.cxx does a lookup that provokes a auto-parsing following by
// an (intentional) compilation error and thus a set of unloading.
.x scripts/step2.cxx
// This use to lead to a strange error (complain that a class is a being
// defined a second time on the ... same line in the same file) which was
// due to the unloading now removing the DefinitionData cached in the forward
// declaration decls left (intentionally) by the unloading.
gInterpreter->AutoParse("reco::utils::ClusterTotals");

@hahnjo
Copy link
Member

hahnjo commented Jul 21, 2025

So just to summarize: cmsUnload will test this fix automatically after the upgrade. I don't see a need to add another test for this.

@vgvassilev
Copy link
Member

cmsUnload is a very complex test which is at integration level, I wanted something that tests the particular feature and when it regresses we can debug and fix it easily without having to build ROOT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants