bpo-32780: Fix the PEP3118 format string for ctypes.Structure #5561
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
eric-wieser
force-pushed
the
ctypes-padding
branch
5 times, most recently
from
d851b6e
to
9dbc70a
Compare
Without this fix, it would never include padding bytes - an assumption that was only valid in the case when `_pack_` was set - and this case was explicitly not implemented. This should allow conversion from ctypes structs to numpy structs.
This comment has been minimized.
This comment has been minimized.
LGTM. The PR is sufficient to fix the problems described. Perhaps judicious use of spacing between fields could make the format clearer, especially when anonymous padding is added. For instance, I would find "<b:x:7x<Q:y:" clearer as "<b:x: 7x <Q:y:". Not critical as the format is probably going to be machine-parsed anyway.
|
That's looks like a reasonable suggestion, bit possibly out of scope for this PR. Less intrusively, I could add whitespace in the test expectations, and remove it before comparing. Let's see how the core devs feel. Thanks for the review! |
|
@abalkin: Any chance you could take a look at this? |
|
@brettcannon: I'd argue this is |
brettcannon
added
type-bug
|
ping |
|
Resolved merge conflicts with https://bugs.python.org/issue22273 |
|
Is there a buffer protocol expert who could help move this along? |
|
This PR is stale because it has been open for 30 days with no activity. |
|
How can we move this 4-year old PR forward? |
Modules/_ctypes/stgdict.c
Outdated
| Py_ssize_t log_n = 0; | ||
| while (n > 0) { | ||
| log_n++; | ||
| n /= 10; |
There was a problem hiding this comment.
Multiplication is often faster than division. Can this be rewritten by computing powers of 10 until n is exceeded?
Better yet, just inline linear search.
if (n < 10ULL)
return 1;
if (n < 100ULL)
return 2;
...
if (n < 10_000_000_000_000_000_000ULL)
return 20; There was a problem hiding this comment.
On the second thought, I would be surprised if this has not been implemented elsewhere in cpython code base. Off the top of my head, I cannot recall where it could be, but I will try to search. If someone beats me to it - please leave a note.
There was a problem hiding this comment.
Can this be rewritten by computing powers of 10 until n is exceeded?
This is risky because the power can overflow
Modules/_ctypes/stgdict.c
Outdated
| } | ||
|
|
||
| /* decimal characters + x + null */ | ||
| buf = PyMem_Malloc(clog10(padding) + 2); |
There was a problem hiding this comment.
I left a comment about log10 implementation above, but looking at the actual use, I don't see why it is needed. Can't we just make buf
char buf[20];and not allocate it on the heap?
There was a problem hiding this comment.
I wanted to avoid risking introducing a buffer overflow by accident by choosing too short a buffer; especially since I don't think we care about performance here. The whole framework for building the format strings here consists of repeated heap allocations, so one more allocation doesn't seem like a big deal.
I think 20 isn't actually enough, as an int64 can need up to 19 digits, and then we need the x and the null.
I could ask Nevermind, PyOS_snprintf to compute the size for me if you'd prefer? Although I can't see any evidence that PyOS_snprintf is actually called with a null buffer anywhere in CPython.PyOS_snprintf does not support this feature of snprintf (#95993)
There was a problem hiding this comment.
I've pushed the version with stack allocation as requested
github-actions
bot
removed
the
stale
The summary of this diff is that it:
_ctypes_alloc_format_paddingfunction to append strings like37xto a format string to indicate 37 padding bytesif (isStruct) {s now that neither isif (isStruct && !isPacked) {_ctypes_alloc_format_paddingto add padding between structure fields, and after the last structure field. The computation used for the total size is unchanged from ctypes already used.This patch does not affect any existing aligment computation; all it does is use subtraction to deduce the amount of paddnig introduced by the existing code.
Without this fix, it would never include padding bytes - an assumption that was only
valid in the case when
_pack_was set - and this case was explicitly not implemented.This should allow conversion from ctypes structs to numpy structs
Fixes numpy/numpy#10528
https://bugs.python.org/issue32780
(#76961)