Skip to content
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

bpo-32782: PEP3118 itemsize of an empty ctypes array should not be 0 #5576

Merged
merged 6 commits into from Dec 23, 2022

Conversation

eric-wieser
Copy link
Contributor

@eric-wieser eric-wieser commented Feb 7, 2018

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.

The tests already expected this behavior, they were just missing the test-case needed to trigger it.

https://bugs.python.org/issue32782

(fixes #76963)

@eric-wieser eric-wieser force-pushed the ctypes-empty-array-itemsize branch 2 times, most recently from 147ab7d to 4383b40 Compare Feb 7, 2018
@eric-wieser eric-wieser force-pushed the ctypes-empty-array-itemsize branch from 4383b40 to 7b4c122 Compare Feb 7, 2018
@eric-wieser eric-wieser changed the title bpo-32782: Itemsize of an empty array should not be 0 bpo-32782: PEP3118 itemsize of an empty ctypes array should not be 0 Feb 7, 2018
@eric-wieser
Copy link
Contributor Author

eric-wieser commented Mar 27, 2018

@pv: Since you've made PEP3118 patches to cpython before, would you mind reviewing this?

Modules/_ctypes/_ctypes.c Outdated Show resolved Hide resolved
@eric-wieser eric-wieser force-pushed the ctypes-empty-array-itemsize branch from 7b4c122 to 29b3c32 Compare May 20, 2018
Modules/_ctypes/_ctypes.c Outdated Show resolved Hide resolved
@mattip
Copy link
Contributor

mattip commented May 20, 2018

LGTM

the test failures do not seem related to this pull request

@eric-wieser
Copy link
Contributor Author

eric-wieser commented May 21, 2018

@mattip: Can you approve via github so that this goes through the process automation that's set up?

mattip
mattip approved these changes May 21, 2018
@eric-wieser
Copy link
Contributor Author

eric-wieser commented May 21, 2018

Thanks @mattip. If you've more ctypes appetite left, there's also #5561

@eric-wieser
Copy link
Contributor Author

eric-wieser commented Aug 8, 2018

Re-opening to restart the tests

@terryjreedy
Copy link
Member

terryjreedy commented Apr 14, 2019

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?

@eric-wieser
Copy link
Contributor Author

eric-wieser commented Apr 14, 2019

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 terryjreedy requested a review from skrah Apr 14, 2019
Modules/_ctypes/_ctypes.c Outdated Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Dec 15, 2022

Deploy Preview for python-cpython-preview canceled.

Name Link
🔨 Latest commit accc969
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/639bb2ef300d190009b736b6

@eric-wieser
Copy link
Contributor Author

eric-wieser commented Dec 15, 2022

@mdickinson, I see you recently reviewed some ctypes PRs; would you mind taking a quick look at this one?

@mdickinson mdickinson self-requested a review Dec 15, 2022
@mdickinson
Copy link
Member

mdickinson commented Dec 15, 2022

@eric-wieser Will do. It might take me a few days to get to it.

Copy link
Member

@mdickinson mdickinson left a comment

LGTM, and works as expected on my machine (confirmed that the new unit tests fail before the fix and pass afterward). I'm wondering whether we should upgrade the asserts in PyCData_itemtype to use Py_FatalError instead. I was unable to convince myself that there was no possibility of stg_dict being NULL on this code path.

@mdickinson mdickinson added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Dec 20, 2022
@bedevere-bot
Copy link

bedevere-bot commented Dec 20, 2022

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit accc969 🤖

If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Dec 20, 2022
@eric-wieser
Copy link
Contributor Author

eric-wieser commented Dec 22, 2022

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?

@eric-wieser
Copy link
Contributor Author

eric-wieser commented Dec 22, 2022

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

@mdickinson
Copy link
Member

mdickinson commented Dec 23, 2022

Is that ok?

Yep, seems reasonable.

Buildbot failures look unrelated. Merging.

@mdickinson mdickinson merged commit 84bc6a4 into python:main Dec 23, 2022
16 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Dec 23, 2022

Thanks @eric-wieser for the PR, and @mdickinson for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒🤖

@miss-islington
Copy link
Contributor

miss-islington commented Dec 23, 2022

Thanks @eric-wieser for the PR, and @mdickinson for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒🤖

@bedevere-bot
Copy link

bedevere-bot commented Dec 23, 2022

GH-100451 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 23, 2022
…be 0 (pythonGH-5576)

The itemsize returned in a memoryview of a ctypes array is now computed from the item type, instead of dividing the total size by the length and assuming that the length is not zero.
(cherry picked from commit 84bc6a4)

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@bedevere-bot
Copy link

bedevere-bot commented Dec 23, 2022

GH-100452 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 23, 2022
…be 0 (pythonGH-5576)

The itemsize returned in a memoryview of a ctypes array is now computed from the item type, instead of dividing the total size by the length and assuming that the length is not zero.
(cherry picked from commit 84bc6a4)

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
mdickinson pushed a commit that referenced this pull request Dec 23, 2022
… be 0 (GH-5576) (#100451)

gh-76963: PEP3118 itemsize of an empty ctypes array should not be 0 (GH-5576)

The itemsize returned in a memoryview of a ctypes array is now computed from the item type, instead of dividing the total size by the length and assuming that the length is not zero.
(cherry picked from commit 84bc6a4)

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
mdickinson pushed a commit that referenced this pull request Dec 23, 2022
… be 0 (GH-5576) (GH-100452)

gh-76963: PEP3118 itemsize of an empty ctypes array should not be 0 (GH-5576)

The itemsize returned in a memoryview of a ctypes array is now computed from the item type, instead of dividing the total size by the length and assuming that the length is not zero.
(cherry picked from commit 84bc6a4)

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expert-ctypes type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

memoryview & ctypes: incorrect itemsize for empty array