cpython
cpython copied to clipboard
python
bpo-32782: PEP3118 itemsize of an empty ctypes array should not be 0
The itemsize should be computed from the item type, not by dividing the total size by the length and assuming that the length is not zero.
https://bugs.python.org/issue32782
(fixes #76963)
@pv: Since you've made PEP3118 patches to cpython before, would you mind reviewing this?
@mattip: Can you approve via github so that this goes through the process automation that's set up?
Thanks @mattip. If you've more ctypes appetite left, there's also #5561
Tests for empty arrays of floats and doubles are added.
My naive question is this: The PEP suggests that itemsize should cache the return from PyBuffer_SizeFromFormat. Was that function actual added? Can it be used?
I can't find an reference to PyBuffer_SizeFromFormat in the source code other than a prototype, and the docs say it's unimplemented.
At any rate, ctypes knows the size without having to resort to string parsing
@terryjreedy No, PyBuffer_SizeFromFormat() has never been added. struct.calcsize() works for simple formats but cannot handle records.
To implement that function properly for nested types, you need some sort of a type tree.
@skrah: Updated the existing commit to fix PEP7, and made a follow-up to use the approach you suggested. It's been over a year since I authored this, so I no longer remember how to run the tests on my setup - I'll leave that to CI.
Ci fails on the second commit - an assertion error checking that stgdict.itemsize was populated by the time get_buffer is called.
There are way too many places that construct stgdict objects fieldwise, and while I tried to find all of them to populate the itemsize member. I seems to have missed some. At any rate, storing information in two places seems like a recipe for it to become out of sync, and I don't think we care about whatever marginal speed boost caching gives here - memoryviews are probably used mostly on large buffers, and the overhead of my original approach scales with number of dimensions (rarely more than 3) not array size.
@skrah, are you happy with the patch of the first commit by itself?
@eric-wieser Thanks for trying out the other approach, if it's too much work I agree that we should not spend too much time on it. The principle of f4e3ca9 looks good to me, I haven't really reviewed it yet though.
I'm at a loss here. Tests pass on my machine (WSL on windows 10), but fail on CI with:
Re-running test_ctypes in verbose mode
Fatal Python error: Floating point exception
I'm miles away from floating point here, so I have no clue what is happening.
Edit: https://github.com/python/cpython/pull/12660/files#r291465946 introduced a new bug in the year since I submitted this.
@skrah: All passing - although probably best to backport #13881 to 2.7 first so that this passes when backported
@csabella, do you have suggestions for finding a core reviewer with time to review this?
I have not looked at this closely, but it seems like @skrah has reviewed it, and this has been looked at by a few people involved in heavier buffer protocol users (i.e. NumPy). Can we somehow help moving issues like this ahead, by reviewing it, or is this hinging too much at specific python core devs? In the back of my mind, the buffer protocol came as much from NumPy as from Python itself, which maybe adds to making it difficult to push these type of issues over the finish line?
I've only reviewed the principle. The buffer protocol is maintained, it is ctypes that hardly anyone wants to touch.
The buffer protocol part in the abstract looks correct, I don't have time or inclination for a deep ctypes review.
This missed the boat for inclusion in Python 3.9 which accepts security fixes only as of today.
Is there anything I can do to help prevent this forever being pushed to a later release?
This PR is stale because it has been open for 30 days with no activity.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
The CI failure here looks completely unrelated to the change; is there anything I can do about it?
Deploy Preview for python-cpython-preview canceled.
| Name | Link |
|---|---|
| Latest commit | accc969bef59e488ca0ae12d4df855ba9700df7c |
| Latest deploy log | https://app.netlify.com/sites/python-cpython-preview/deploys/639bb2ef300d190009b736b6 |
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
@mdickinson, I see you recently reviewed some ctypes PRs; would you mind taking a quick look at this one?
:robot: New build scheduled with the buildbot fleet by @mdickinson for commit accc969bef59e488ca0ae12d4df855ba9700df7c :robot:
If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.
Thanks for testing!
Given this file has 52 asserts (11 on std_dict) and no Py_FatalErrors, I'd be inclined to declare that change out of scope.
Is that ok?
~~Oops, just realized that I probably clobbered the buildbot status by merging main; github was giving me a red cross so I clicked the update button without looking carefully enough...~~
Edit: nevermind, results are here