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.

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

[cling] Forcefully clean up TemplateIdAnnotations #16150

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Aug 1, 2024

Upstream Clang keeps TemplateIdAnnotations around "if they might still be in the token stream." See upstream commit for more details: llvm/llvm-project@6163aa9 (included in Clang 11, in ROOT since the upgrade to LLVM 13)

This reasoning doesn't apply when we fully reset the Parser state in ParserStateRAII's destructor, and we expect the swapped out vector of TemplateIdAnnotations to be empty in order to not leak.

Fixes #16121

Upstream Clang keeps TemplateIdAnnotations around "if they might
still be in the token stream." See upstream commit for more details:
llvm/llvm-project@6163aa9
(included in Clang 11, in ROOT since the upgrade to LLVM 13)

This reasoning doesn't apply when we fully reset the Parser state in
ParserStateRAII's destructor, and we expect the swapped out vector of
TemplateIdAnnotations to be empty in order to not leak.

Fixes root-project#16121
@vgvassilev
Copy link
Member

@smuzaffar, could we test that PR on cmssw?

@smuzaffar
Copy link
Contributor

sure, cmssw tests are running via cms-sw#207 now

Copy link

github-actions bot commented Aug 1, 2024

Test Results

    13 files      13 suites   2d 18h 21m 13s ⏱️
 3 025 tests  3 025 ✅ 0 💤 0 ❌
33 788 runs  33 788 ✅ 0 💤 0 ❌

Results for commit 9f6c0b3.

@smuzaffar
Copy link
Contributor

cmssw tests look good

@vgvassilev
Copy link
Member

cmssw tests look good

Thanks Shahzad!

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.

LGTM!

@hahnjo hahnjo merged commit e6bae6a into root-project:master Aug 2, 2024
18 checks passed
@hahnjo hahnjo deleted the cling-template-ids branch August 2, 2024 06:18
@hahnjo
Copy link
Member Author

hahnjo commented Aug 2, 2024

@dpiparo this could be a candidate for backporting. Probably not to 6.30, which is already in production, but we may still have a window to fix this memory leak before 6.32 gets fully approved for data taking next year...

@hahnjo
Copy link
Member Author

hahnjo commented Aug 7, 2024

Discussed with Danilo: We will not backport this to 6.32 because there is a risk of breaking stuff. It fixes a memory leak, but it is bounded as confirmed by Dev's tests, and was only found one and half years after it's introduction during the upgrade to LLVM 13.

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

Successfully merging this pull request may close these issues.

Potential memory leak in clang triggered by findScope
3 participants