Skip to content

[cling] Work around constructor priority bug #16420

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

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Sep 13, 2024

LLVM had a bug where constructors with the same priority would not be stably sorted. This has been fixed upstream by llvm/llvm-project#95532, but to avoid relying on a backport this commit works around the issue: The idea is that we lower the default priority of concerned constructors to make them sort correctly.


As discussed, this provides a bit of magic 🪄 to work around the bug if we don't have the backport, for example in the Conda releases. I propose we still leave the backport in our copy of LLVM though.

@hahnjo hahnjo self-assigned this Sep 13, 2024
@hahnjo hahnjo requested a review from pcanal as a code owner September 13, 2024 06:49
Copy link

github-actions bot commented Sep 13, 2024

Test Results

    13 files      13 suites   3d 4h 20m 22s ⏱️
 3 030 tests  3 030 ✅ 0 💤 0 ❌
33 869 runs  33 869 ✅ 0 💤 0 ❌

Results for commit 705771a.

♻️ This comment has been updated with latest results.

LLVM had a bug where constructors with the same priority would not be
stably sorted. This has been fixed upstream by
llvm/llvm-project#95532, but to avoid relying
on a backport this commit works around the issue: The idea is that we
lower the default priority of concerned constructors to make them sort
correctly.
@vgvassilev
Copy link
Member

I feel uneasy about this change. @smuzaffar, can you run the cmssw CI on this PR?

@smuzaffar
Copy link
Contributor

smuzaffar commented Sep 16, 2024

I feel uneasy about this change. @smuzaffar, can you run the cmssw CI on this PR?

cmssw tests started via cms-sw#210

@smuzaffar
Copy link
Contributor

cmssw tests look good cms-sw#210 (comment)

@vgvassilev
Copy link
Member

Thank you, @smuzaffar!

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 7db43f7 into root-project:master Sep 18, 2024
18 checks passed
@hahnjo hahnjo deleted the cling-constructors branch September 18, 2024 07:19
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.

3 participants