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

Avoid infinite recursion in implicit conversion #17536

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

vepadulano
Copy link
Member

Fixes #17497

@guitargeek These changes, taken from https://github.com/wlav/cppyy-backend/blob/master/clingwrapper/src/clingwrapper.cxx , fix the reproducer at the linked issue. Let's see what the rest of the CI says. Then we may want to discuss how to incorporate these changes into the repository.

The check `klass == ctxt->fCurScope` in ConvertImplicit was always returning
false due to a bad construction of the `ctxt` variable of type
`CPyCppyy::CallContext`.

In particular, the `fCurScope` of the context is taken as the `fScope` data
member of the instance converter. This value is usually created at construction
time, for example in the `STLStringViewConverter` constructor, by calling
`Cppyy::GetScope`.

The `GetScope` implementation of ROOT's clingwrapper was outdated w.r.t. the
cppyy-backend implementation. This commit aligns the two implementations, thus
fixing issues such as the one reported at
root-project#17497.
Copy link

github-actions bot commented Jan 27, 2025

Test Results

    18 files      18 suites   3d 22h 25m 6s ⏱️
 2 683 tests  2 683 ✅ 0 💤 0 ❌
46 598 runs  46 598 ✅ 0 💤 0 ❌

Results for commit 053d1bc.

♻️ This comment has been updated with latest results.

@bellenot
Copy link
Member

bellenot commented Jan 29, 2025

So the failure on Windows is due to throw std::runtime_error which is not working on Windows x64!!!

C:\root-dev\build\x64\gh-17497>root -l
root [0] void fun(){throw std::runtime_error("");}
root [1] fun()

C:\root-dev\build\x64\gh-17497>

@vepadulano
Copy link
Member Author

@bellenot thank you for your investigation! I will then disable this test on Windows pending a fix to a much larger issue on the platform.

@vepadulano vepadulano force-pushed the gh-17497 branch 2 times, most recently from 38e81ae to 50515b0 Compare January 29, 2025 14:31
Disabled on MacOS ARM and Windows because LLVM JIT fails to catch exceptions.
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thank you! Good that you fixed the issue, and reducing the diff is always welcome 👍

@vepadulano vepadulano merged commit 078cf6c into root-project:master Jan 31, 2025
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Particular combination of function overload and std::runtime_error leads to segfault
3 participants