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

[cling] Move static init function renaming to BackendPasses.cpp #17406

Merged
merged 2 commits into from
Jan 20, 2025
Merged

[cling] Move static init function renaming to BackendPasses.cpp #17406

merged 2 commits into from
Jan 20, 2025

Conversation

devajithvs
Copy link
Contributor

@devajithvs devajithvs commented Jan 10, 2025 •
edited
Loading

This Pull request:

Changes or fixes:

Checklist:

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

This PR removes the need for the clang patch: root-project/llvm-project@d84a0e3104c5

This is already tested with roottest-cling-staticinit-ROOT-7775

@devajithvs devajithvs requested review from hahnjo and pcanal January 10, 2025 13:59
@devajithvs devajithvs self-assigned this Jan 10, 2025
devajithvs
devajithvs commented Jan 10, 2025
View reviewed changes
interpreter/cling/lib/Interpreter/BackendPasses.cpp Outdated
@@ -420,6 +462,7 @@ void BackendPasses::CreatePasses(int OptLevel, llvm::ModulePassManager& MPM,
StandardInstrumentations& SI) {

// TODO: Remove this pass once we upgrade past LLVM 19 that includes the fix.
MPM.addPass(UniqueInitFunctionNamePass());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Didn't notice the comment. I'll move the comment down when I push to update LLVM tag.

Copy link
Member

Choose a reason for hiding this comment

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

Not addressed yet AFACIT; the comment applies directly to WorkAroundConstructorPriorityBugPass and should move there...

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 10, 2025 •
edited
Loading

Test Results

    18 files      18 suites   4d 9h 42m 22s ⏱️
 2 695 tests  2 695 ✅ 0 💤 0 ❌
46 780 runs  46 780 ✅ 0 💤 0 ❌

Results for commit 20797f7.

♻️ This comment has been updated with latest results.

hahnjo
hahnjo reviewed Jan 11, 2025
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.

FWIW still needs a tag (we'll figure out a sequencing between all pending changes) and we should either reverse the order of commits or squash them so that the test passes at all points in the commit history.

interpreter/cling/lib/Interpreter/BackendPasses.cpp Outdated Show resolved Hide resolved
@devajithvs devajithvs force-pushed the dev.make_unique branch 2 times, most recently from 9c62c73 to 1eedab3 Compare January 16, 2025 16:17
@devajithvs
Copy link
Contributor Author

@hageboeck I've updated the tag here: https://github.com/root-project/llvm-project/releases/tag/ROOT-llvm18-20250116-01 to also backport llvm/llvm-project#101761

hahnjo
hahnjo approved these changes Jan 17, 2025
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.

LGTM, after moving the comment as indicated.

I've updated the tag here: https://github.com/root-project/llvm-project/releases/tag/ROOT-llvm18-20250116-01 to also backport llvm/llvm-project#101761

Ideally I would have preferred this to be a separate PR because it has nothing to do with moving the renaming pass and dropping a Clang patch. Also in the future, please use cherry-pick -x for upstream commits in https://github.com/root-project/llvm-project/ so that we record the upstream commit hash. (no need to create another tag right now, we can fix it if we ever need to touch upstream commits again)

interpreter/cling/lib/Interpreter/BackendPasses.cpp Outdated
@@ -420,6 +462,7 @@ void BackendPasses::CreatePasses(int OptLevel, llvm::ModulePassManager& MPM,
StandardInstrumentations& SI) {

// TODO: Remove this pass once we upgrade past LLVM 19 that includes the fix.
MPM.addPass(UniqueInitFunctionNamePass());
Copy link
Member

Choose a reason for hiding this comment

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

Not addressed yet AFACIT; the comment applies directly to WorkAroundConstructorPriorityBugPass and should move there...

@devajithvs
[cling] Move static init function renaming to BackendPasses.cpp
0ace7d6 
This moves the logic for making static initialization function names
unique to `BackendPasses.cpp`

Functionality remains the same as introduced in commit
d84ee9c. The only difference being
the the names are suffixed with numbering (_1, _2, etc.) before the
module suffix:

__cxx_global_var_initcling_module_27_
__cxx_global_var_init_1cling_module_27_
__cxx_global_var_init_2cling_module_27_
__cxx_global_var_init_3cling_module_27_

instead of

__cxx_global_var_initcling_module_27_
__cxx_global_var_initcling_module_27__1
__cxx_global_var_initcling_module_27__2
__cxx_global_var_initcling_module_27__3
@devajithvs
[llvm-project] Revert the clang patch fixing ROOT-7775
20797f7 
Revert the patch:
`Fix ROOT-7775 by making all static init function name unique.`
@devajithvs
Copy link
Contributor Author

devajithvs commented Jan 17, 2025 •
edited
Loading

LGTM, after moving the comment as indicated.

I've updated the tag here: https://github.com/root-project/llvm-project/releases/tag/ROOT-llvm18-20250116-01 to also backport llvm/llvm-project#101761

Ideally I would have preferred this to be a separate PR because it has nothing to do with moving the renaming pass and dropping a Clang patch. Also in the future, please use cherry-pick -x for upstream commits in https://github.com/root-project/llvm-project/ so that we record the upstream commit hash. (no need to create another tag right now, we can fix it if we ever need to touch upstream commits again)

Thanks Jonas, my bad, I've created a new tag with just the change needed for this PR. I will make a separate PR with the backport later.

@devajithvs devajithvs merged commit 22e243b into root-project:master Jan 20, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpiparo dpiparo dpiparo approved these changes

@hahnjo hahnjo hahnjo approved these changes

@pcanal pcanal Awaiting requested review from pcanal

@vgvassilev vgvassilev Awaiting requested review from vgvassilev vgvassilev is a code owner

Assignees

@devajithvs devajithvs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@devajithvs @hahnjo @dpiparo