-
Notifications
You must be signed in to change notification settings - Fork 2.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
Compile without exceptions #1703
Conversation
I believe the two failures on Travis are unrelated.
Looking into appveyor, but I'm not sure why this should affect those. |
I can confirm with this latest pull request (#1709) that the failing presubmits are unrelated to my change. |
@wjakob sorry to bug! Would it be possible to provide an ETA on any review? |
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.
I'm not an official maintainer, but this needs a change log entry and it would be nice to have tests to show what happens to these pieces of code when compiled without exceptions.
include/pybind11/detail/common.h
Outdated
try { \ | ||
PYBIND11_CONCAT(pybind11_init_, name)(module); \ | ||
return module.ptr(); \ | ||
} catch (pybind11::error_already_set & e) { \ |
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.
If we're replacing PYBIND11_CATCH_INIT_EXCEPTIONS
with this function, let's do it consistently and remove the macro completely, which means replacing the macro in the deprecated PYBIND11_PLUGIN
as well.
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.
That was not intentional. Merged the latest changes which should make it more clear.
include/pybind11/detail/common.h
Outdated
@@ -369,6 +396,23 @@ constexpr size_t instance_simple_holder_in_ptrs() { | |||
return size_in_ptrs(sizeof(std::shared_ptr<int>)); | |||
} | |||
|
|||
inline bool Py_VersionCheckPassed() { |
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.
Similar to my other comment, we should consistently replace the PYBIND11_CHECK_PYTHON_VERSION
macro.
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.
Done. Thanks!
Merging the latest changes.
…o the deprecated PYBIND11_PLUGIN as well.
@bstaletic do you mind please providing more insight into the change log entry? Any specific file I should modify or add a short note in the README? |
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.
The changelog is in docs/changelog.rst
.
static void PYBIND11_CONCAT(pybind11_init_, name)(pybind11::module &); \ | ||
PYBIND11_PLUGIN_IMPL(name) { \ | ||
PYBIND11_CHECK_PYTHON_VERSION \ | ||
auto m = pybind11::module(PYBIND11_TOSTRING(name)); \ |
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.
So what happens now if constructing an extension module throws?
Imagine a line inside a user's PYBIND11_MODULE
that says py::module foo = py::import("throwy_module");
and actually having a exception thrown from the throwy_module
.
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.
If you've set the flag PYBIND11_NOEXCEPTIONS and you run without enable exceptions I think it will fail to compile.
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.
I was talking about raising an exception from pure python, which can't be detected at compile time. I gave it a test and here are the two similar cases that I've tried:
// foo.cpp
#include <pybind11/pybind11.h>
PYBIND11_MODULE(foo, m) {
pybind11::module x = pybind11::module::import("boom");
}
If there is no boom.py
in python's path, the following happens:
Python 3.7.3 (default, Mar 27 2019, 19:19:44)
[GCC 8.2.1 20181127] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> try:
... import foo
... except Exception:
...
KeyboardInterrupt
>>> try:
... import foo
... except Exception as e:
... print(e)
...
terminate called after throwing an instance of 'pybind11::error_already_set'
what(): ModuleNotFoundError: No module named 'boom'
zsh: abort python
And if we add a boom.py
that looks like this:
raise RuntimeError
the error becomes more verbose:
Python 3.7.3 (default, Mar 27 2019, 19:19:44)
[GCC 8.2.1 20181127] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import boom
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/bstaletic/Temp/pybind11/boom.py", line 1, in <module>
raise RuntimeError
RuntimeError
>>> try:
... import foo
... except Exception as e:
... print(e)
...
terminate called after throwing an instance of 'pybind11::error_already_set'
what(): RuntimeError:
At:
/home/bstaletic/Temp/pybind11/boom.py(1): <module>
<frozen importlib._bootstrap>(219): _call_with_frames_removed
<frozen importlib._bootstrap_external>(728): exec_module
<frozen importlib._bootstrap>(677): _load_unlocked
<frozen importlib._bootstrap>(967): _find_and_load_unlocked
<frozen importlib._bootstrap>(983): _find_and_load
<frozen importlib._bootstrap>(219): _call_with_frames_removed
<frozen importlib._bootstrap_external>(1043): create_module
<frozen importlib._bootstrap>(583): module_from_spec
<frozen importlib._bootstrap>(670): _load_unlocked
<frozen importlib._bootstrap>(967): _find_and_load_unlocked
<frozen importlib._bootstrap>(983): _find_and_load
<stdin>(2): <module>
zsh: abort python
Either way, it quickly calls std::terminate()
, which is completely fine, but let's mention it in the changelog too.
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.
Ah whoops, I totally misread that. Okay I'll update the changelog. Thanks @bstaletic
I'm still unable to compile pybind with
|
Hi @av8ramit, apologies for the long delay -- this is a side project of mine, and my time for supporting it is limited. I'm not super-excited about this PR: exceptions are really central to the internals of pybind11. They can of course be commented out like what is done here, but this will break any kind of error handling in software using pybind11. I'll close this for now but am open to further discussion. Best, |
@wjakob thanks for taking a look! The purpose of the PR is to allow users to use pybind who for unrelated reasons cannot use C++ exceptions. Case in point, google compiles with I believe that pybind provides huge value, and I would like to be able to use it, and in the environment that we're in, I do not have to worry about exceptions (since they don't exist). This is true for anyone who would be using -fno-exceptions. I don't think this would break error handling, since by definition users using -fno-exceptions would not be able to create exceptions. It is very possible that I'm missing something here. We figured this would be the minimally intrusive way to make pybind work in an environment without exceptions. If you have suggestions that would make this more palatable, we're open to any sort of changes. Thank you! |
Hi @martinwicke, This PR simply turns off exceptions but punts on addressing the underlying problem, which is fundamental: Python makes copious use of exceptions — not just explicit ones but also implicit ones e.g. when querying an iterator. All of these exceptions need to be converted into something meaningful when crossing the Python -> C++ boundary. If exceptions are forbidden, what is pybind11 to do? The old school thing would be to use return values or manual error checks everywhere, but that is far too intrusive of a change to both pybind11 and projects using pybind11. FWIW Compiling with -fno-exception as a policy strikes me as pretty terrible default choice, especially for binding code that needs to interoperate with Python. Best, |
Yeah Your point is very valid. Right now, IIUC, any Python exception that gets exposed to C++ code via tha patched pybind would simply terminate. I agree stringing error handling code through everything is not desirable. Basically, the way that it is now (modulo bugs that need to be fixed) would be that crippling C++ by removing exceptions means that it is now illegal for an exception raised in python to be exposed to C++ via pybind (if that happens, the program terminates). I think this is pretty consistent. I understand that limits how good the Python integration can be (e.g., we won't be writing C++ code handling iterators), but it is still a big value-add to be able to conveniently expose C++ classes/functions to Python. |
Terminating the application when a Python exception reaches C++ sounds absolutely nuts to me! I think you should give some thought on how to get out of this situation that an unreasonable policy is forced on to you (rather than pushing it to dependency libraries). If your only recourse is to modify pybind11 in this way, then I would rather you did so in a fork. |
I have demonstrated what happens if a python exception reaches C++ when importing the module - it ends up calling What happens when importing doesn't fail, but then python layer calls a C++ function?
#include "include/pybind11/pybind11.h"
void f() {
pybind11::module x = pybind11::module::import("boom");
}
PYBIND11_MODULE(foo, m) {
m.def("f", f);
}
raise RuntimeError
So it's not always clear what happens when a python exception reaches C++. |
Wrapping exception logic in #ifdefs to allow for compilation without exceptions.
Context: We'd like to use pybind, but are in an environment where -fno-exceptions is required (Google). The suggested changes are sufficient to make it work, but if you prefer a different approach that'd be fine too.