gh-76961: Fix the PEP3118 format string for ctypes.Structure

[eric-wieser]

The summary of this diff is that it:

• adds a _ctypes_alloc_format_padding function to append strings like 37x to a format string to indicate 37 padding bytes
• removes the branches that amount to "give up on producing a valid format string if the struct is packed"
• combines the resulting adjacent if (isStruct) {s now that neither is if (isStruct && !isPacked) {
• invokes _ctypes_alloc_format_padding to 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

Fixes #76961

• Issue: ctypes: memoryview gives incorrect PEP3118 format strings for both packed and unpacked structs #76961

[the-knights-who-say-ni]

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!

[mattip]

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.

[eric-wieser]

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!

[eric-wieser]

@abalkin: Any chance you could take a look at this?

[eric-wieser]

@brettcannon: I'd argue this is type-bugfix, not type-enhancement - the implementation in master produces invalid buffers in almost all cases. This isn't just adding support for _pack_ (an enhancement), but also fixing the behavior for when _pack_ is absent.

[abalkin]

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; 

[abalkin]

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.

[eric-wieser]

Can this be rewritten by computing powers of 10 until n is exceeded?

This is risky because the power can overflow

[abalkin]

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.

It took me a little longer than I expected to get back to this, but the code that I was looking for is in Python/dtoa.c.

@mdickinson - Is the approximation used in dtoa applicable to the problem at hand? If so, do you think that code can be factored out and called here?

[mdickinson]

@abalkin Looks like the relevant code is no longer part of the PR. But the Python/dtoa.c code is for floats, and assumes IEEE 754 format; it's not clear to me how it could be used here. (In general, I'm reluctant to add floating-point dependence to code that doesn't need it.)

[abalkin]

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?

[eric-wieser]

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 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. Nevermind, PyOS_snprintf does not support this feature of snprintf (#95993)

[eric-wieser]

I've pushed the version with stack allocation as requested

[eric-wieser]

@mdickinson, I really appreciate that you were able to review #5576 a few weeks ago. Do you think you'd be able to take a look at this one too?

[mdickinson]

@eric-wieser Yes, I'll take a look. It won't be this weekend, though, I'm afraid.

[mdickinson]

This LGTM, and behaves as expected in my manual testing; the code looks good. I left a couple of nitpick-level comments / suggestions.

@abalkin This is still assigned to you; is there anything you're aware of that would mean this shouldn't be merged?

The issue is marked as a "bug" (which makes some sense), but I think this is sufficiently new-feature'y that the changes shouldn't be backported to Python <3.12. @abalkin: thoughts?

The way we build up the format string doesn't seem ideal - it looks as though it would take time quadratic in the number of fields of the struct. But AFAICT that's a pre-existing issue, not introduced in this PR. Presumably for the sort of things for which this is used in practice this hasn't yet been a real issue.

Observation: it seems there's no documentation of the "data format description" language outside PEP 3118 itself, unless I'm missing something. I'd expect the ctypes documentation to at least have a pointer to PEP 3118 (though it would be better to have a description in the docs themselves at some point, since PEPs are historical documents that shouldn't generally by relied upon to be up to date with implementations.) Anyway, that's off-topic for this PR.

[mdickinson]

@eric-wieser: Changes LGTM; thank you.

@abalkin Python 3.12a5 is due tomorrow, which means that if we leave this PR open for another couple of days we'll end up having to nudge the news entry again. (That was poor timing on my part - sorry about that.) But I think this is ready, so I'll take the EAFP approach: I'll merge, and we can tweak later if necessary.

[abalkin]

@mdickinson - my only concern was with the floor log10 computation that I felt could be done better. If it looks good enough to you, I have no objections to the merge.

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit 90d85a9.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/464/builds/3771) and take a look at the build logs.
4. Check if the failure is related to this commit (90d85a9) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/464/builds/3771

Failed tests:

• test_ctypes

Failed subtests:

• test_native_types - test.test_ctypes.test_pep3118.Test.test_native_types

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

413 tests OK.

1 test failed:
test_ctypes

17 tests skipped:
test_asdl_parser test_check_c_globals test_clinic test_devpoll
test_gdb test_ioctl test_kqueue test_launcher test_msilib
test_peg_generator test_perf_profiler test_startfile
test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test_ctypes

Total duration: 22 min

Click to see traceback logs

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.installed/build/target/lib/python3.12/test/test_ctypes/test_pep3118.py", line 27, in test_native_types
    self.assertEqual(normalize(v.format), normalize(fmt))
AssertionError: 'T{<b:x:3x<Q:y:}' != 'T{<b:x:7x<Q:y:}'
- T{<b:x:3x<Q:y:}
?        ^
+ T{<b:x:7x<Q:y:}
?        ^

[mdickinson]

Urgh. That looks like a legitimate failure on 32-bit platforms, introduced by this PR. @eric-wieser I think this is a problem with the test rather than the core logic. Do you agree?

[eric-wieser]

Yes, you're right; the cases that don't set _pack_ at all are platform-dependent.

I think a test written for 32 bit platforms would pass on both; so replacing the uint64 with uint32s should make the problem go away

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Non-Debug with X 3.x has failed when building commit 90d85a9.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/58/builds/3813) and take a look at the build logs.
4. Check if the failure is related to this commit (90d85a9) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/58/builds/3813

Failed tests:

• test_ctypes

Failed subtests:

• test_native_types - test.test_ctypes.test_pep3118.Test.test_native_types

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

419 tests OK.

10 slowest tests:

• test_math: 4 min 3 sec
• test_multiprocessing_spawn: 3 min 45 sec
• test_asyncio: 2 min 46 sec
• test_concurrent_futures: 2 min 45 sec
• test_tokenize: 1 min 46 sec
• test_multiprocessing_forkserver: 1 min 32 sec
• test_unparse: 1 min 24 sec
• test_multiprocessing_fork: 1 min 15 sec
• test_signal: 1 min 12 sec
• test_compileall: 1 min 2 sec

1 test failed:
test_ctypes

14 tests skipped:
test_check_c_globals test_devpoll test_ioctl test_kqueue
test_launcher test_msilib test_peg_generator test_perf_profiler
test_startfile test_winconsoleio test_winreg test_winsound
test_wmi test_zipfile64

1 re-run test:
test_ctypes

Total duration: 26 min 53 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.x.ware-gentoo-x86.nondebug/build/Lib/test/test_ctypes/test_pep3118.py", line 27, in test_native_types
    self.assertEqual(normalize(v.format), normalize(fmt))
AssertionError: 'T{<b:x:3x<Q:y:}' != 'T{<b:x:7x<Q:y:}'
- T{<b:x:3x<Q:y:}
?        ^
+ T{<b:x:7x<Q:y:}
?        ^

[mdickinson]

I think a test written for 32 bit platforms would pass on both [...]

I'll give that a go; PR shortly. I'm wondering whether I'm going to end up hitting problems with ILP32 platforms not being consistent about whether uint32 is aliased to unsigned int or to unsigned long, though.

[mdickinson]

PR shortly

#101587