bpo-32780: 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 https://github.com/numpy/numpy/issues/10528

https://bugs.python.org/issue32780

(#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!

[eric-wieser]

CLA is now marked as signed on bpo

[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.

[mattip]

ping

[eric-wieser]

Resolved merge conflicts with https://bugs.python.org/issue22273

[mattip]

Is there a buffer protocol expert who could help move this along?

[github-actions[bot]]

This PR is stale because it has been open for 30 days with no activity.

[mattip]

How can we move this 4-year old PR forward?

[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]

@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]

:warning::warning::warning: Buildbot failure :warning::warning::warning:

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

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 (90d85a9b4136aa1feb02f88aab614a3c29f20ed3) 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]

:warning::warning::warning: Buildbot failure :warning::warning::warning:

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

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 (90d85a9b4136aa1feb02f88aab614a3c29f20ed3) 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