Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Dec 13, 2023
Merged

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

merged 5 commits into from
Dec 13, 2023

Conversation

jalopezg-git
Copy link
Collaborator

This pull request is a backport of PR #13534 (a NFC; makes the code more readable) and PR #13565 (actual fix for unloading member functions of templated classes).
The aforementioned PRs have been reviewed separately.

Changes or fixes:

For the actual list of changes, see

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).

@jalopezg-git jalopezg-git self-assigned this Sep 6, 2023
@phsft-bot
Copy link
Collaborator

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
Copy link
Collaborator

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

@phsft-bot
Copy link
Collaborator

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

Failing tests:

@phsft-bot
Copy link
Collaborator

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

Warnings:

  • [2023-09-06T17:07:55.619Z] /Users/sftnight/build/workspace/root-pullrequests-build/build/ginclude/tbb/machine/macos_common.h:55:13: warning: 'OSAtomicCompareAndSwap64Barrier' is deprecated: first deprecated in macOS 10.12 - Use std::atomic_compare_exchange_strong() from <atomic> instead [-Wdeprecated-declarations]
  • [2023-09-06T17:07:55.619Z] /Users/sftnight/build/workspace/root-pullrequests-build/build/ginclude/tbb/machine/macos_common.h:59:28: warning: 'OSAtomicAdd64' is deprecated: first deprecated in macOS 10.12 - Use std::atomic_fetch_add_explicit(std::memory_order_relaxed) from <atomic> instead [-Wdeprecated-declarations]
  • [2023-09-06T17:07:55.619Z] /Users/sftnight/build/workspace/root-pullrequests-build/build/ginclude/tbb/machine/macos_common.h:100:13: warning: 'OSAtomicCompareAndSwap32Barrier' is deprecated: first deprecated in macOS 10.12 - Use std::atomic_compare_exchange_strong() from <atomic> instead [-Wdeprecated-declarations]
  • [2023-09-06T17:07:55.619Z] /Users/sftnight/build/workspace/root-pullrequests-build/build/ginclude/tbb/machine/macos_common.h:110:12: warning: 'OSAtomicAdd32Barrier' is deprecated: first deprecated in macOS 10.12 - Use std::atomic_fetch_add() from <atomic> instead [-Wdeprecated-declarations]
  • [2023-09-06T17:07:55.619Z] /Users/sftnight/build/workspace/root-pullrequests-build/build/ginclude/tbb/machine/macos_common.h:116:12: warning: 'OSAtomicAdd64Barrier' is deprecated: first deprecated in macOS 10.12 - Use std::atomic_fetch_add() from <atomic> instead [-Wdeprecated-declarations]
  • [2023-09-06T17:07:55.619Z] /Users/sftnight/build/workspace/root-pullrequests-build/build/ginclude/tbb/tbb_machine.h:339:31: warning: 'OSMemoryBarrier' is deprecated: first deprecated in macOS 10.12 - Use std::atomic_thread_fence() from <atomic> instead [-Wdeprecated-declarations]
  • [2023-09-06T17:07:55.619Z] /Users/sftnight/build/workspace/root-pullrequests-build/build/ginclude/tbb/tbb_machine.h:608:9: warning: 'OSMemoryBarrier' is deprecated: first deprecated in macOS 10.12 - Use std::atomic_thread_fence() from <atomic> instead [-Wdeprecated-declarations]
  • [2023-09-06T17:07:55.619Z] /Users/sftnight/build/workspace/root-pullrequests-build/build/ginclude/tbb/tbb_machine.h:612:9: warning: 'OSMemoryBarrier' is deprecated: first deprecated in macOS 10.12 - Use std::atomic_thread_fence() from <atomic> instead [-Wdeprecated-declarations]
  • [2023-09-06T17:07:55.619Z] /Users/sftnight/build/workspace/root-pullrequests-build/build/ginclude/tbb/tbb_machine.h:635:9: warning: 'OSMemoryBarrier' is deprecated: first deprecated in macOS 10.12 - Use std::atomic_thread_fence() from <atomic> instead [-Wdeprecated-declarations]
  • [2023-09-06T17:07:55.619Z] /Users/sftnight/build/workspace/root-pullrequests-build/build/ginclude/tbb/tbb_machine.h:645:9: warning: 'OSMemoryBarrier' is deprecated: first deprecated in macOS 10.12 - Use std::atomic_thread_fence() from <atomic> instead [-Wdeprecated-declarations]

And 113 more

Failing tests:

hahnjo
hahnjo requested changes Oct 6, 2023
View reviewed changes
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.

to make it explicit: this is causing failures of stressInterpreter on macOS and must not be merged for now.

@jalopezg-git
Copy link
Collaborator Author

to make it explicit: this is causing failures of stressInterpreter on macOS and must not be merged for now.

That's true; I'll have to check how to fix this... Currently, it seems non-trivial: we probably need to judge whether to unload a declaration or not by looking at the whole chain of (implicit) instantiations 🤷‍♂️.

@guitargeek guitargeek changed the title [cling,v6-28] DeclUnloader: fix unloading of member functions of templated classes [v628][cling] DeclUnloader: fix unloading of member functions of templated classes Nov 5, 2023
@jalopezg-git
Copy link
Collaborator Author

Unsure as to whether backporting to v6.28 is still needed. Feel free to close the PR otherwise 🙂.

@hahnjo
Copy link
Member

hahnjo commented Dec 5, 2023

Unsure as to whether backporting to v6.28 is still needed. Feel free to close the PR otherwise 🙂.

I think we want to backport, I will take care of updating the PR in the next days. Also, we definitely want a backport to v6.30, also to come in the next days.

jalopezg-git and others added 5 commits December 13, 2023 08:36
@jalopezg-git @hahnjo
[cling][NFC] Move helper functions to anonymous namespace at the begi…
2006924 
…nning

Cherry-picked from fa2cacb.
@jalopezg-git @hahnjo
[cling] Fix unmatched #pragma clang diagnostic push
df781ce 
Cherry-picked from 94c5c9e.
@jalopezg-git @hahnjo
[cling] DeclUnloader: remove StaticVarCollector
63fb8e9 
StaticVarCollector recursively visited descendants of a `FunctionDecl`
node to collect static local variables.  However, these always appear
in the enclosing DeclContext.

(cherry picked from commit bd28a5b)
@jalopezg-git @hahnjo
[cling] DeclUnloader: do not delete instantiated member functions
9724f3e 
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.

(cherry picked from commit afba1d7)
@hahnjo
[cling] DeclUnloader: Remove extra check isInstantiatedInPCH
f5d4a18 
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.

(cherry picked from commit 1c3ea0a)
@hahnjo hahnjo force-pushed the cling-DeclUnloader-fix-memberfunc_6.28 branch from 8d506b8 to f5d4a18 Compare December 13, 2023 07:48
@phsft-bot
Copy link
Collaborator

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

hahnjo
hahnjo approved these changes Dec 13, 2023
View reviewed changes
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.

All green now 😃

@hahnjo hahnjo merged commit 1766f21 into root-project:v6-28-00-patches Dec 13, 2023
1 of 2 checks passed
@jalopezg-git jalopezg-git deleted the cling-DeclUnloader-fix-memberfunc_6.28 branch December 13, 2023 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hahnjo hahnjo hahnjo approved these changes

@pcanal pcanal pcanal approved these changes

@Axel-Naumann Axel-Naumann Awaiting requested review from Axel-Naumann Axel-Naumann is a code owner

@vgvassilev vgvassilev Awaiting requested review from vgvassilev

Assignees

@Axel-Naumann Axel-Naumann

@jalopezg-git jalopezg-git

Labels
affects:6.28 in:Cling pr:backport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jalopezg-git @phsft-bot @hahnjo @pcanal @Axel-Naumann