-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
python_bindings: Fix SIGSEGV in HalidePythonCompileTimeErrorReporter #6635
Conversation
python_bindings/src/PyError.cpp
Outdated
py::gil_scoped_acquire acquire; | ||
py::print(msg, py::arg("end") = ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now exactly halide_python_print(nullptr, msg);
. Maybe you want to use that implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
I guess I should go read the guidelines on when acquiring/releasing the GIL is appropriate, since this is at least the second PR adding such a control... |
Can you add this test case to python_bindings/correctness? |
c01e524
to
635c74e
Compare
@alexreinking Added a test to compare the warnings from stdout. |
635c74e
to
f86d803
Compare
Any idea what failed in this build? https://buildbot.halide-lang.org/master/#builders/188/builds/47 |
buildbot hiccups from being restarted this morning, should be restarted shortly |
When I realize
g
in the snippet below, I get a SIGSEGV inPyErr_Occurred
. This example is run with python bindings fromrelease/13.x
branch.Expected: I see the warning
Actual: SIGSEGV
I added a
py::gil_scoped_acquire
before printing similar tohalide_python_print()
inPyError.cpp
and this fixes the output. Should I release the GIL?After the fix