[cling] DeclUnloader: fix unloading of member functions of templated classes #13565

jalopezg-git · 2023-08-29T14:58:14Z

This pull request fixes the unloading of member functions of templated classes.

Specifically, the body of member functions of a templated class only gets instantiated when the function is first used, e.g. in this example below, the body for the member function T f(T x) (where T is a typename template argument for the templated struct Foo) will not get instantiated until f() is first used.

`-ClassTemplateDecl
  |-TemplateTypeParmDecl referenced typename depth 0 index 0 T
  |-CXXRecordDecl struct Foo definition
  | |-DefinitionData
  | `-CXXMethodDecl f 'T (T)'
  |   |-ParmVarDecl 0x55e5787cac70 referenced x 'T'
  |   `-CompoundStmt
  |     `-ReturnStmt
  |       `-DeclRefExpr 'T' lvalue ParmVar 0x55e5787cac70 'x' 'T'
  `-ClassTemplateSpecializationDecl struct Foo definition
    |-DefinitionData
    |-TemplateArgument type 'int'
    | `-BuiltinType 'int'
    |-CXXMethodDecl f 'int (int)'    <<<< Instantiation pending
    | `-ParmVarDecl x 'int':'int'
    |-CXXConstructorDecl implicit used constexpr Foo 'void () noexcept' inline default trivial

Such functions should not be deleted from the AST, but returned to the 'pending instantiation' state.

Also, any function template instantiation, even if coming from an external AST source, needs some handling in order for it to be re-emitted the next time.

Changes or fixes:

Replace StaticVarCollector by simpler code. Static local variables always appear in the enclosing DeclContext
Apply the patch described above.

Checklist:

tested changes locally
updated the docs (if necessary)
Passes cling test suite

This PR fixes #10049, #6439, #7970, ROOT-10848 and ROOT-8084 and (hopefully also ROOT-8245).

phsft-bot · 2023-08-29T14:58:23Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

phsft-bot · 2023-08-29T14:58:29Z

Build failed on windows10/default.
See console output.

phsft-bot · 2023-08-29T15:30:39Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2023-08-29T15:46:08Z

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2023-08-29T16:33:56Z

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2023-08-29T17:46:18Z

Build failed on mac11/noimt.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2023-08-29T18:15:55Z

Build failed on mac12arm/cxx20.
Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

github-actions · 2023-08-29T19:01:44Z

Test Results

      10 files       10 suites 1d 15h 31m 33s ⏱️
  2 483 tests   2 483 ✔️ 0 💤 0 ❌
23 757 runs 23 757 ✔️ 0 💤 0 ❌

Results for commit c224025.

♻️ This comment has been updated with latest results.

jalopezg-git · 2023-08-31T11:00:05Z

Build failed on mac12arm/cxx20. Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build See console output.

Failing tests:

* [projectroot.test.test_stressinterpreter](https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/183553/testReport/projectroot/test/test_stressinterpreter/)

* [projectroot.roottest.root.meta.cmsUnload.roottest_root_meta_cmsUnload_make](https://lcgapp-services.cern.ch/root-jenkins/job/root-pullrequests-build/183553/testReport/projectroot.roottest.root.meta/cmsUnload/roottest_root_meta_cmsUnload_make/)

I'll check these failing tests in the next few days, but for the rest it should be ready for review.

phsft-bot · 2023-09-06T12:22:13Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

jalopezg-git · 2023-09-06T12:50:31Z

This patchset should be ready now 🚀! @Axel-Naumann, @hahnjo, @vgvassilev Could you review when you have some spare time?

phsft-bot · 2023-09-06T13:09:39Z

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2023-09-06T14:23:06Z

Build failed on mac11/noimt.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.test.test_stressinterpreter

Axel-Naumann · 2023-09-06T16:00:55Z

Fantastic, thanks, @jalopezg-git ! Is the mac11 failure in stressInterpreter caused by this PR?

jalopezg-git · 2023-09-06T16:16:59Z

Fantastic, thanks, @jalopezg-git ! Is the mac11 failure in stressInterpreter caused by this PR?

Apparently, yes. Ugghh, that wasn't expected; all the other platforms seem to be happy now.

Any chance that I can still access one of these machines to debug the issue?

phsft-bot · 2023-09-06T16:55:09Z

Build failed on mac12arm/cxx20.
Running on macphsft26.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

pcanal · 2023-09-26T02:36:42Z

@phsft-bot build just on mac11/noimt

phsft-bot · 2023-09-26T02:36:49Z

Starting build on mac11/noimt
How to customize builds

phsft-bot · 2023-09-26T03:48:06Z

Build failed on mac11/noimt.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.test.test_stressinterpreter

phsft-bot · 2023-11-22T11:00:26Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

hahnjo · 2023-11-22T11:02:22Z

For now, this is just a rebase of Javier's original changes. I think I have an idea what could be wrong, but I first want to have a clean set of reports from Jenkins and the new CI. In any case, I'll try to take care of this PR now because it seems required for some recent progress I'm making regarding #13815, in turn blocking the upgrade to LLVM 16.

phsft-bot · 2023-11-22T14:11:41Z

Build failed on mac12arm/cxx20.
Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.test.test_stressinterpreter

StaticVarCollector recursively visited descendants of a `FunctionDecl` node to collect static local variables. However, these always appear in the enclosing DeclContext.

The body of member functions of a templated class only gets instantiated when the function is used. These `CXXMethodDecl` should not be deleted from the AST; instead return them to the 'instantiation pending' state.

This effectively reverts commit 74472ca ("[cling] Fixes issue in DeclUnloader: do not unload templates intantiated in the PCH"), it's not needed anymore, all tests pass and the snippet in the summary of root-project#4447 works, but it filters too many declarations from the unloader.

phsft-bot · 2023-11-24T14:12:14Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

phsft-bot · 2023-11-24T15:42:39Z

Build failed on windows10/default.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

jalopezg-git · 2023-11-24T20:58:19Z

For now, this is just a rebase of Javier's original changes. I think I have an idea what could be wrong, but I first want to have a clean set of reports from Jenkins and the new CI. In any case, I'll try to take care of this PR now because it seems required for some recent progress I'm making regarding #13815, in turn blocking the upgrade to LLVM 16.

Great! Let's give this a push 🙂!

hahnjo · 2023-11-24T21:03:28Z

So it wasn't what I initially thought, but it seems to work now on all platforms (except the spurious failure on Windows).

hahnjo

I'm accepting this pro-forma, Javier's changes look good to me and my additional commit is a continuation of that work. All tests pass and check-cling stays clean (on my EL8 at least). If others want more time to review this, please comment!

vgvassilev

LGTM!

jalopezg-git · 2023-11-29T18:43:09Z

Awesome! @Axel-Naumann, @hahnjo, I'll let you decide when to click merge 🙂!

jalopezg-git added bug affects:master in:Cling labels Aug 29, 2023

jalopezg-git requested review from Axel-Naumann, hahnjo, pcanal and vgvassilev August 29, 2023 14:58

jalopezg-git self-assigned this Aug 29, 2023

jalopezg-git mentioned this pull request Aug 31, 2023

[WIP] Improved error handling for template instantiation #12449

Open

jalopezg-git force-pushed the cling-DeclUnloader-fix-memberfunc branch from 509ca0d to fdf6c53 Compare September 6, 2023 12:22

jalopezg-git mentioned this pull request Sep 6, 2023

[v628][cling] DeclUnloader: fix unloading of member functions of templated classes #13618

Merged

3 tasks

jalopezg-git mentioned this pull request Sep 6, 2023

Closes JIRA issue ROOT-8245 (re-enable cling/reload/reloadvector test) root-project/roottest#598

Merged

pcanal assigned Axel-Naumann Sep 26, 2023

hahnjo mentioned this pull request Oct 12, 2023

Cling (rightfully) confused about forward-declared template specializations #13815

Closed

hahnjo force-pushed the cling-DeclUnloader-fix-memberfunc branch from fdf6c53 to 0089635 Compare November 22, 2023 11:00

jalopezg-git and others added 3 commits November 24, 2023 11:13

[cling] DeclUnloader: remove StaticVarCollector

58104d0

StaticVarCollector recursively visited descendants of a `FunctionDecl` node to collect static local variables. However, these always appear in the enclosing DeclContext.

[cling] DeclUnloader: do not delete instantiated member functions

fe6c0d9

The body of member functions of a templated class only gets instantiated when the function is used. These `CXXMethodDecl` should not be deleted from the AST; instead return them to the 'instantiation pending' state.

hahnjo force-pushed the cling-DeclUnloader-fix-memberfunc branch from 0089635 to c224025 Compare November 24, 2023 14:12

hahnjo mentioned this pull request Nov 25, 2023

Update ROOT and Cling to LLVM 16 #13273

Merged

23 tasks

hahnjo approved these changes Nov 28, 2023

View reviewed changes

vgvassilev approved these changes Nov 29, 2023

View reviewed changes

hahnjo merged commit 1c3ea0a into root-project:master Nov 30, 2023
14 of 16 checks passed

jalopezg-git deleted the cling-DeclUnloader-fix-memberfunc branch December 1, 2023 10:33

hahnjo mentioned this pull request Dec 13, 2023

Backport Cling fixes for v6.30 #14224

Merged

guitargeek mentioned this pull request Jan 8, 2024

[math] Tests roottest_root_math_smatrix_testKalman and testOperations fail since 1st of December 2023 on debian10-i386 #14306

Closed

hahnjo mentioned this pull request Feb 2, 2024

Failed compilation leaves cling in a "bad state" #6439

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cling] DeclUnloader: fix unloading of member functions of templated classes #13565

[cling] DeclUnloader: fix unloading of member functions of templated classes #13565

jalopezg-git commented Aug 29, 2023 •

edited

phsft-bot commented Aug 29, 2023

phsft-bot commented Aug 29, 2023

phsft-bot commented Aug 29, 2023

phsft-bot commented Aug 29, 2023

phsft-bot commented Aug 29, 2023

phsft-bot commented Aug 29, 2023

phsft-bot commented Aug 29, 2023

github-actions bot commented Aug 29, 2023 •

edited

jalopezg-git commented Aug 31, 2023

Failing tests:

phsft-bot commented Sep 6, 2023

jalopezg-git commented Sep 6, 2023

phsft-bot commented Sep 6, 2023

phsft-bot commented Sep 6, 2023

Axel-Naumann commented Sep 6, 2023

jalopezg-git commented Sep 6, 2023

phsft-bot commented Sep 6, 2023

pcanal commented Sep 26, 2023

phsft-bot commented Sep 26, 2023

phsft-bot commented Sep 26, 2023

phsft-bot commented Nov 22, 2023

hahnjo commented Nov 22, 2023

phsft-bot commented Nov 22, 2023

phsft-bot commented Nov 24, 2023

phsft-bot commented Nov 24, 2023

jalopezg-git commented Nov 24, 2023

hahnjo commented Nov 24, 2023

hahnjo left a comment

vgvassilev left a comment

jalopezg-git commented Nov 29, 2023 •

edited

[cling] DeclUnloader: fix unloading of member functions of templated classes #13565

[cling] DeclUnloader: fix unloading of member functions of templated classes #13565

Conversation

jalopezg-git commented Aug 29, 2023 • edited

Changes or fixes:

Checklist:

phsft-bot commented Aug 29, 2023

phsft-bot commented Aug 29, 2023

phsft-bot commented Aug 29, 2023

Failing tests:

phsft-bot commented Aug 29, 2023

Failing tests:

phsft-bot commented Aug 29, 2023

Failing tests:

phsft-bot commented Aug 29, 2023

Failing tests:

phsft-bot commented Aug 29, 2023

Failing tests:

github-actions bot commented Aug 29, 2023 • edited

Test Results

jalopezg-git commented Aug 31, 2023

Failing tests:

phsft-bot commented Sep 6, 2023

jalopezg-git commented Sep 6, 2023

phsft-bot commented Sep 6, 2023

Failing tests:

phsft-bot commented Sep 6, 2023

Failing tests:

Axel-Naumann commented Sep 6, 2023

jalopezg-git commented Sep 6, 2023

phsft-bot commented Sep 6, 2023

Failing tests:

pcanal commented Sep 26, 2023

phsft-bot commented Sep 26, 2023

phsft-bot commented Sep 26, 2023

Failing tests:

phsft-bot commented Nov 22, 2023

hahnjo commented Nov 22, 2023

phsft-bot commented Nov 22, 2023

Failing tests:

phsft-bot commented Nov 24, 2023

phsft-bot commented Nov 24, 2023

Failing tests:

jalopezg-git commented Nov 24, 2023

hahnjo commented Nov 24, 2023

hahnjo left a comment

Choose a reason for hiding this comment

vgvassilev left a comment

Choose a reason for hiding this comment

jalopezg-git commented Nov 29, 2023 • edited

jalopezg-git commented Aug 29, 2023 •

edited

github-actions bot commented Aug 29, 2023 •

edited

jalopezg-git commented Nov 29, 2023 •

edited