gh-156933: Widen narrow integer results in ctypes callbacks - #157045
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
This failure is a 10 minute timeout in test_subprocess's test_check on the macOS Intel runner, unrelated to this change (which only touches _ctypes/callbacks.c). |
This comment was marked as resolved.
This comment was marked as resolved.
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
This needs a test case.
|
I couldn't write a test that fails before this fix and passes after it on any CI architecture. The existing |
|
I'm confused; how did you ensure that your fix works if you can't test on s390x? This feels a lot like you fed an issue into Codex/Claude/whatever, and you just sent the PR to us. Please don't try to fix issues that you can't even reproduce. |
ZeroIntensity
added
the
pending
|
It was AI-assisted, yes. I reviewed and drove the actual fix, and I get why the s390x question is fair. Here's what I actually checked: the widening logic follows libffi's own contract for If someone with s390x access can confirm the fix before/after, that's better evidence than anything I can produce here — happy to wait on that if you'd rather not merge on the reasoning alone. |
|
@miladfarca can you help testing this fix or find someone with the access to a s390x machine? |
|
I really don't appreciate you copy-pasting AI output as a response. Please use your own words. I'm going to refrain from reviewing this until someone (preferably from the LLVM team) can confirm that this is the right solution for s390x. I don't completely understand this fix, and given that you can't even reproduce the bug locally, I'm not so sure that you do either. |
|
I have created this small test case to test this PR, it fails without the patch and passes with it on s390x, I cannot run the entire test suite as I don't have the proper environment: diff --git a/Lib/test/test_ctypes/test_callbacks.py b/Lib/test/test_ctypes/test_callbacks.py
index 6c7c2e52707..204eb6da301 100644
--- a/Lib/test/test_ctypes/test_callbacks.py
+++ b/Lib/test/test_ctypes/test_callbacks.py
@@ -328,6 +328,14 @@ def func():
f"of ctypes callback function {func!r}")
self.assertIsNone(cm.unraisable.object)
+ def test_narrow_int_return_widened(self):
+ @CFUNCTYPE(c_int)
+ def cb():
+ return -1
+
+ addr = ctypes.cast(cb, ctypes.c_void_p).value
+ wide = CFUNCTYPE(c_longlong)(addr)
+ self.assertEqual(wide(), -1)
if __name__ == '__main__':
unittest.main() |
|
Thanks for testing this on s390x! I added your test in ebf7568. I put a 64-bit skip on it, because on 32-bit targets a |
|
I limited the test to s390x in 9338911. It failed on x86-64 CI even with the fix: libffi zero-extends a 32-bit result there, and the x86-64 ABI leaves the upper bits undefined, so reading an |
ZeroIntensity
removed
the
pending
|
Thanks @miladfarca for confirming. I think we do have a few s390x buildbots lying around too. I'll try to run them once the review comments are addressed. @lazerg In the future, please only work on issues that you can actually verify, and please don't use LLM output to reply to comments. I get that AI is good at solving issues like this, but it ultimately just shifts the cognitive effort to the maintainer/reviewer. |
|
Yes, understood. |
|
!buildbot s390x |
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit 5693c85 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F157045%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
ZeroIntensity
added
needs backport to 3.14
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
Thanks, this looks good. The buildbot failures on Fedora look unrelated; I've opened an issue: #158294.
|
Thanks @lazerg for the PR, and @ZeroIntensity for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15. |
|
GH-158295 is a backport of this pull request to the 3.15 branch. |
bedevere-app
Bot
removed
the
needs backport to 3.15
|
Sorry, @lazerg and @ZeroIntensity, I could not cleanly backport this to |
|
GH-158296 is a backport of this pull request to the 3.14 branch. |
|
GH-158297 is a backport of this pull request to the 3.13 branch. |
…H-157045) (GH-158297) _CallPythonObject() only wrote restype->size bytes into the closure's result buffer, leaving the unused high-order bits of the ffi_arg-sized register untouched. libffi's ffi_prep_closure_loc() documents that integral types narrower than a machine register must be widened to fill it, sign-extending signed types. On architectures that always read the full register for narrow return values (s390x), this leaves garbage in the high bits, which broke libclang callbacks used by cindex.py. (cherry picked from commit b6f9a50) Co-authored-by: Lazizbek Ergashev <lazerg2@gmail.com>
6 participants
_CallPythonObject() only wrote restype->size bytes into the closure's result buffer, leaving the unused high-order bits of the ffi_arg-sized register untouched. libffi's ffi_prep_closure_loc() documents that integral types narrower than a machine register must be widened to fill it, sign-extending signed types. On architectures that always read the full register for narrow return values (s390x), this leaves garbage in the high bits, which broke libclang callbacks used by cindex.py.
The fix widens narrow integer results into a register-sized buffer before writing them back, sign- or zero-extending depending on the type, replacing the old big-endian-only pointer offset that didn't actually widen anything.
Fixes #156933.