-
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
Fix unsigned error value casting #931
Conversation
fd471ff
to
bfeb2d3
Compare
The added tests segfaulted under PyPy; it turns out to be a latent issue (i.e. occurs in current master) when attempting to call: m.def("f", [](int32_t) {}); with a value that fits in a I've squashed the fix into this commit: we aren't allowed to call |
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 looks great. 👍 for adding the detailed tests and catching the exception issue.
tests/test_builtin_casters.cpp
Outdated
@@ -72,6 +72,11 @@ TEST_SUBMODULE(builtin_casters, m) { | |||
m.def("string_view32_return", []() { return std::u32string_view(U"utf32 secret \U0001f382"); }); | |||
#endif | |||
|
|||
m.def("i32_str", [](std::int32_t v) { return std::to_string(v); }); |
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.
Can you add a comment above this line to associate it with the Python test function:
// test_integer_casting
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.
When casting to an unsigned type from a python 2 `int`, we currently cast using `(unsigned long long) PyLong_AsUnsignedLong(src.ptr())`. If the Python cast fails, it returns (unsigned long) -1, but then we cast this to `unsigned long long`, which means we get 4294967295, but because that isn't equal to `(unsigned long long) -1`, we don't detect the failure. This commit moves the unsigned casting into a `detail::as_unsigned` function which, upon error, casts -1 to the final type, and otherwise casts the return value to the final type to avoid the problematic double cast when an error occurs. The error most commonly shows up wherever `long` is 32-bits (e.g. under both 32- and 64-bit Windows, and under 32-bit linux) when passing a negative value to a bound function taking an `unsigned long`. Fixes pybind#929. The added tests also trigger a latent segfault under PyPy: when casting to an integer smaller than `long` (e.g. casting to a `uint32_t` on a 64-bit `long` architecture) we check both for a Python error and also that the resulting intermediate value will fit in the final type. If there is no conversion error, but we get a value that would overflow, we end up calling `PyErr_ExceptionMatches()` illegally: that call is only allowed when there is a current exception. Under PyPy, this segfaults the test suite. It doesn't appear to segfault under CPython, but the documentation suggests that it *could* do so. The fix is to only check for the exception match if we actually got an error.
bfeb2d3
to
dc7a677
Compare
When casting to an unsigned type from a python 2
int
, we currently cast using(unsigned long long) PyLong_AsUnsignedLong(src.ptr())
. If the Python cast fails, it returns(unsigned long) -1
, but then wecast this to
unsigned long long
, which means we get 4294967295, but because that isn't equal to(unsigned long long) -1
, we don't detect the failure.This commit moves the unsigned casting into a
detail::as_unsigned
function which, upon error, casts -1 to the final type, and otherwise casts the return value to the final type to avoid the problematic double cast when an error occurs.The error most commonly shows up under Python 2 wherever
long
is 32-bits (e.g. under both 32- and 64-bit Windows, and under 32-bit linux) when passing a negative value to a bound function taking anunsigned long
. The same issue would show up, however, for other size-increasing casts regardless of the Python version (for example, when binding a function taking gcc's__uint128_t
type).Fixes #929.