bpo-10746: ctypes: Fix PEP 3118 type codes for c_long, c_bool, c_int #31

pv · 2017-02-11T19:35:34Z

Ctypes currently produces wrong pep3118 type codes for several types.
E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms,
but it should be "<q" instead for sizeof(c_long) == 8

The problem is that the '<>' endian specification in the struct syntax
also turns on the "standard size" mode, which makes type characters have
a platform-independent meaning, which does not match with the codes used
internally in ctypes. The struct module format syntax also does not
allow specifying native-size non-native-endian items.

This commit adds a converter function that maps the internal ctypes
codes to appropriate struct module standard-size codes in the pep3118
format strings. The tests are modified to check for this.

Example of the current problem in practice:

>>> import numpy, ctypes
>>> numpy.asarray(memoryview(ctypes.c_long(42)))
/usr/lib64/python3.5/site-packages/numpy/core/numeric.py:482: RuntimeWarning: Item size computed from the PEP 3118 buffer format string does not match the actual item size.
  return array(a, dtype, copy=False, order=order)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.5/site-packages/numpy/core/numeric.py", line 482, in asarray
    return array(a, dtype, copy=False, order=order)
ValueError: setting an array element with a sequence.

https://bugs.python.org/issue10746

the-knights-who-say-ni · 2017-02-11T19:35:35Z

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 these steps to rectify the issue:

Sign the PSF contributor agreement
Wait at least a day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

codecov · 2017-02-11T20:06:04Z

Codecov Report

Merging #31 into master will increase coverage by <.01%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
+ Coverage   82.37%   82.37%   +<.01%     
==========================================
  Files        1427     1427              
  Lines      350948   350957       +9     
==========================================
+ Hits       289088   289114      +26     
+ Misses      61860    61843      -17

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7ffb99...b4cf54b. Read the comment docs.

pv · 2017-02-14T19:18:30Z

CLA signed.

pitrou · 2017-06-06T23:40:42Z

Modules/_ctypes/_ctypes.c

+  since the endianness may need to be swapped to a non-native one
+  later on.
+ */
+char *


This function should be declared static.

pitrou · 2017-06-06T23:44:53Z

Lib/ctypes/test/test_pep3118.py

+        # Since some types may be aliases to each other, look up the
+        # struct code based on the actual ctypes type
+        tp = code_map[ctypes_code][0]
+        struct_code = code_map[tp._type_][1][sizeof(tp)]


I don't understand this line. Can you show an example where tp._type_ is necessary?

I must say I don't understand it either anymore. It may be leftover from refactoring.
Since the tests seem to pass, I now simplified it.

Ah no, it was due to a int/long failure on windows:

====================================================================== FAIL: test_native_types (ctypes.test.test_pep3118.Test) ---------------------------------------------------------------------- Traceback (most recent call last): File "C:\projects\cpython\lib\ctypes\test\test_pep3118.py", line 56, in test_native_types normalize(replace_type_codes(fmt))) AssertionError: '<l' != '<i' - <l + <i

Added explanatory comment in the test.

pitrou · 2017-06-06T23:45:51Z

Modules/_ctypes/_ctypes.c

+    case 'i': pep_code = 'q'; break;
+    case 'I': pep_code = 'Q'; break;
+#else
+# error


It would be nice to add a non-empty error message, for example "SIZEOF_INT is not one of the expected values".

pitrou · 2017-06-06T23:46:06Z

Modules/_ctypes/_ctypes.c

+    case 'l': pep_code = 'q'; break;
+    case 'L': pep_code = 'Q'; break;
+#else
+# error


pitrou · 2017-06-06T23:46:11Z

Modules/_ctypes/_ctypes.c

+#elif SIZEOF__BOOL == 8
+    case '?': pep_code = 'Q'; break;
+#else
+# error


pv · 2017-06-22T22:50:52Z

Sorry for the delay, updated the PR based on comments.

pv · 2017-08-26T06:45:29Z

ping?

pitrou · 2017-08-26T08:58:50Z

Sorry for the delay.

pitrou · 2017-08-26T08:59:48Z

Lib/ctypes/test/test_pep3118.py

+        #
+        # Example: on Windows c_long translates to '<i', not '<l'.
+        tp = code_map[ctypes_code][0]
+        struct_code = code_map[tp._type_][1][sizeof(tp)]


I still find this difficult to read and understand. Can't you rewrite the unit tests and the native_types array to get rid of the double indirection?

pitrou · 2017-08-26T09:00:38Z

Lib/ctypes/test/test_pep3118.py

@@ -111,7 +146,11 @@ class Complete(Structure):
 #
 # This table contains format strings as they look on little endian
 # machines.  The test replaces '<' with '>' on big endian machines.
+# The type codes written below are the internal ctypes type codes,


Internal ctypes type codes are not even documented, are they?

pitrou · 2017-08-26T09:03:41Z

Thanks for persisting. I am not really happy about the way this hacks around the current structure of the tests. It adds a lot of complication that seems pointless given the task at hand (we're just checking a mapping, right?).

pv · 2017-08-26T09:35:44Z

A static translation table does not exist, because the size of the types, and also which type code is produced for some of the integer types when multiple equivalent possibilities exist, is platform and possibly compiler dependent. If you are suggesting to write all possibilities explicitly, you have to write different tables for win32, win64, linux32, etc. Can you specify more precisely what you object to: - using replace in strings to produce appropriate codes rather than writing them explicitly, - not hardcoding codes for equivalent integer types, - something else? 26.8.2017 11.04 "Antoine Pitrou" <notifications@github.com> kirjoitti:

…

Thanks for persisting. I am not really happy about the way this hacks around the current structure of the tests. It adds a lot of complication that seems pointless given the task at hand (we're just checking a mapping, right?). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5qKVqObam9UqEqb2IfRqEGfrycROks5sb9-AgaJpZM4L-Rci> .

pv · 2017-08-26T09:37:40Z

The internal types codes are not documented. But do you have better suggestions how to write the strings? They cannot be written in PEP3118 format because the produced strings are platform etc dependent. 26.8.2017 11.04 "Antoine Pitrou" <notifications@github.com> kirjoitti:

…

Thanks for persisting. I am not really happy about the way this hacks around the current structure of the tests. It adds a lot of complication that seems pointless given the task at hand (we're just checking a mapping, right?). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5qKVqObam9UqEqb2IfRqEGfrycROks5sb9-AgaJpZM4L-Rci> .

pv · 2017-08-26T09:43:45Z

This of course was ignored previously in the tests, because the ctypes implementation of this stuff was just wrong. 26.8.2017 11.37 "Pauli Virtanen" <pav@iki.fi> kirjoitti: The internal types codes are not documented. But do you have better suggestions how to write the strings? They cannot be written in PEP3118 format because the produced strings are platform etc dependent. 26.8.2017 11.04 "Antoine Pitrou" <notifications@github.com> kirjoitti:

…

Thanks for persisting. I am not really happy about the way this hacks around the current structure of the tests. It adds a lot of complication that seems pointless given the task at hand (we're just checking a mapping, right?). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5qKVqObam9UqEqb2IfRqEGfrycROks5sb9-AgaJpZM4L-Rci> .

pitrou · 2017-08-26T16:49:17Z

My main gripe here is that you shouldn't need code_map at all, let alone the complicated double indirection inside that table. Just add/replace whatever information is missing to native_types.

Also we don't care at all about ctypes internal code types, so the tests would be clearer if they didn't involve them.

pv · 2017-08-26T17:08:51Z

The expected pep strings are platform-dependent. Do you agree with that statement? Do you mean the entries in the native_types table should be translate_typecodes("...") calls in the table itself. Or, do you mean code_map should be replaced by a slew of if statemebts encoding the platform dependence? 26.8.2017 19.49 "Antoine Pitrou" <notifications@github.com> kirjoitti:

…

My main gripe here is that you shouldn't need code_map at all, let alone the complicated double indirection inside that table. Just add/replace whatever information is missing to native_types. Also we don't care at all about ctypes internal code types, so the tests would be clearer if they didn't involve them. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5i6dyAcsuUqZDCKqIIkcgKtizybgks5scEyagaJpZM4L-Rci> .

pv · 2017-08-26T17:13:13Z

I think I need railroad track now :) Could you give an example how you would like the test for the pep3118 format string for c_long to look like? 26.8.2017 20.08 "Pauli Virtanen" <pav@iki.fi> kirjoitti: The expected pep strings are platform-dependent. Do you agree with that statement? Do you mean the entries in the native_types table should be translate_typecodes("...") calls in the table itself. Or, do you mean code_map should be replaced by a slew of if statemebts encoding the platform dependence? 26.8.2017 19.49 "Antoine Pitrou" <notifications@github.com> kirjoitti:

…

My main gripe here is that you shouldn't need code_map at all, let alone the complicated double indirection inside that table. Just add/replace whatever information is missing to native_types. Also we don't care at all about ctypes internal code types, so the tests would be clearer if they didn't involve them. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5i6dyAcsuUqZDCKqIIkcgKtizybgks5scEyagaJpZM4L-Rci> .

pitrou · 2017-08-26T17:44:21Z

You are already encoding platform-dependent information (the size mappings) in code_map? Why don't you fold that into native_types?

pv · 2017-08-26T17:48:52Z

native_types and the other tables contain also compound types where these type codes appear. To me the present approach appeared the simplest solution without code repetition. 26.8.2017 20.13 "Pauli Virtanen" <pav@iki.fi> kirjoitti:

…

I think I need railroad track now :) Could you give an example how you would like the test for the pep3118 format string for c_long to look like? 26.8.2017 20.08 "Pauli Virtanen" ***@***.***> kirjoitti: The expected pep strings are platform-dependent. Do you agree with that statement? Do you mean the entries in the native_types table should be translate_typecodes("...") calls in the table itself. Or, do you mean code_map should be replaced by a slew of if statemebts encoding the platform dependence? 26.8.2017 19.49 "Antoine Pitrou" ***@***.***> kirjoitti: > My main gripe here is that you shouldn't need code_map at all, let alone > the complicated double indirection inside that table. Just add/replace > whatever information is missing to native_types. > > Also we don't care at all about ctypes internal code types, so the tests > would be clearer if they didn't involve them. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#31 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AACI5i6dyAcsuUqZDCKqIIkcgKtizybgks5scEyagaJpZM4L-Rci> > . >

pv · 2017-08-26T21:30:45Z

Ok, once more then, without magic. Is this what you wanted?

pitrou · 2017-08-28T08:53:19Z

Thank you! :-) I find this much better. In particular, it's explicit that the exact type code does not depend only on the type size but also on whether c_int aliases c_long, for example (which is a weird oddity, but I guess it's not this PR's responsability to change that).

One additional thing: can you add a NEWS entry? We nowadays use the blurb CLI tool for that.

pv · 2017-08-28T11:43:20Z

It's here: https://travis-ci.org/python/cpython/jobs/269157668
The build passes with success regardless.

pitrou · 2017-08-28T11:57:25Z

Hmm... are you sure you updated master before rebasing?

pv · 2017-08-28T12:07:36Z

See the pushed commit. It's parent is a30f6d4, which is the current python master. 28.8.2017 14.57 "Antoine Pitrou" <notifications@github.com> kirjoitti:

…

Hmm... are you sure you updated master before rebasing? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACI5tR7nhHuhdygAJlPE6t0pEI26cNrks5scqsxgaJpZM4L-Rci> .

pitrou · 2017-08-28T12:08:46Z

I see. Well, it passed on AppVeyor anyway, so I'm going to merge. Thanks for persisting in this!

…_int (pythonGH-31) Ctypes currently produces wrong pep3118 type codes for several types. E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms, but it should be "<q" instead for sizeof(c_long) == 8 The problem is that the '<>' endian specification in the struct syntax also turns on the "standard size" mode, which makes type characters have a platform-independent meaning, which does not match with the codes used internally in ctypes. The struct module format syntax also does not allow specifying native-size non-native-endian items. This commit adds a converter function that maps the internal ctypes codes to appropriate struct module standard-size codes in the pep3118 format strings. The tests are modified to check for this. (cherry picked from commit 07f1658)

…_int (pythonGH-31) Ctypes currently produces wrong pep3118 type codes for several types. E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms, but it should be "<q" instead for sizeof(c_long) == 8 The problem is that the '<>' endian specification in the struct syntax also turns on the "standard size" mode, which makes type characters have a platform-independent meaning, which does not match with the codes used internally in ctypes. The struct module format syntax also does not allow specifying native-size non-native-endian items. This commit adds a converter function that maps the internal ctypes codes to appropriate struct module standard-size codes in the pep3118 format strings. The tests are modified to check for this.. (cherry picked from commit 07f1658)

…_int (GH-31) (#3241) Ctypes currently produces wrong pep3118 type codes for several types. E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms, but it should be "<q" instead for sizeof(c_long) == 8 The problem is that the '<>' endian specification in the struct syntax also turns on the "standard size" mode, which makes type characters have a platform-independent meaning, which does not match with the codes used internally in ctypes. The struct module format syntax also does not allow specifying native-size non-native-endian items. This commit adds a converter function that maps the internal ctypes codes to appropriate struct module standard-size codes in the pep3118 format strings. The tests are modified to check for this. (cherry picked from commit 07f1658)

…_int (GH-31) (#3242) [2.7] bpo-10746: Fix ctypes PEP 3118 type codes for c_long, c_bool, c_int (GH-31) Ctypes currently produces wrong pep3118 type codes for several types. E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms, but it should be "<q" instead for sizeof(c_long) == 8 The problem is that the '<>' endian specification in the struct syntax also turns on the "standard size" mode, which makes type characters have a platform-independent meaning, which does not match with the codes used internally in ctypes. The struct module format syntax also does not allow specifying native-size non-native-endian items. This commit adds a converter function that maps the internal ctypes codes to appropriate struct module standard-size codes in the pep3118 format strings. The tests are modified to check for this.. (cherry picked from commit 07f1658)

…ython#31) Ctypes currently produces wrong pep3118 type codes for several types. E.g. memoryview(ctypes.c_long()).format gives "<l" on 64-bit platforms, but it should be "<q" instead for sizeof(c_long) == 8 The problem is that the '<>' endian specification in the struct syntax also turns on the "standard size" mode, which makes type characters have a platform-independent meaning, which does not match with the codes used internally in ctypes. The struct module format syntax also does not allow specifying native-size non-native-endian items. This commit adds a converter function that maps the internal ctypes codes to appropriate struct module standard-size codes in the pep3118 format strings. The tests are modified to check for this.

There were cases where data["commit"]["committer"] is null.

Closes python#31

31: Only warn in file init r=ltratt a=nanjekyejoannah Fixes python#29 Co-authored-by: Joannah Nanjekye <jnanjeky@unb.ca>

the-knights-who-say-ni added the CLA not signed label Feb 11, 2017

zware removed the CLA not signed label Feb 14, 2017

the-knights-who-say-ni added the CLA signed label Feb 14, 2017

pitrou requested changes Jun 6, 2017

View reviewed changes

pv force-pushed the bpo-10746 branch 2 times, most recently from d43d02d to eed2ee1 Compare June 22, 2017 23:02

pv force-pushed the bpo-10746 branch from eed2ee1 to 54f9123 Compare July 28, 2017 23:27

pv force-pushed the bpo-10746 branch from 54f9123 to b3435a0 Compare August 26, 2017 06:42

pitrou reviewed Aug 26, 2017

View reviewed changes

pv force-pushed the bpo-10746 branch from f9bf424 to 179b1de Compare August 26, 2017 21:24

pitrou approved these changes Aug 28, 2017

View reviewed changes

bedevere-bot added the awaiting merge label Aug 28, 2017

pitrou merged commit 07f1658 into python:master Aug 28, 2017

Mariatta removed the awaiting merge label Aug 29, 2017

eric-wieser mentioned this pull request Sep 2, 2017

dtype and struct integer format characters are deceptively similar, yet different numpy/numpy#9642

Open

eric-wieser mentioned this pull request May 24, 2018

BUG: gh-10882 may have broken ctypes compatibility in older versions of python numpy/numpy#11150

Closed

mdboom mentioned this pull request Aug 25, 2022

Assert and incorrect error message when loading source file containing invalid UTF-8 #96268

Closed

ziegenbalg mentioned this pull request Nov 30, 2022

double free in io.TextIOWrapper #72573

Closed

jaraco pushed a commit that referenced this pull request Dec 2, 2022

Check if payload contains "committer" info. (GH-31)

70317f4

There were cases where data["commit"]["committer"] is null.

jaraco pushed a commit to jaraco/cpython that referenced this pull request Feb 17, 2023

Support text open(), though not by default (python#32)

8475c05

Closes python#31

Fidget-Spinner referenced this pull request in pylbbv/pylbbv May 27, 2023

Fix: BB_TEST_ITER wrong type propagation (#31)

66143f9

nanjekyejoannah added a commit to nanjekyejoannah/cpython that referenced this pull request Jul 28, 2023

Merge python#31

d888130

31: Only warn in file init r=ltratt a=nanjekyejoannah Fixes python#29 Co-authored-by: Joannah Nanjekye <jnanjeky@unb.ca>

gvanrossum mentioned this pull request Aug 22, 2023

heap-use-after-free in _PyFunction_LookupByVersion #108253

Closed

LinanV mentioned this pull request Oct 23, 2023

Objects/typeobject.c: No such file or directory. #111203

Closed

stasos24 mentioned this pull request Oct 24, 2023

Modules/cjkcodecs/_codecs_iso2022.c - read out of bounds #101180

Closed

williamhu020 mentioned this pull request Nov 5, 2023

Use the API C of 'Py-NewInterpreterFromConfig' to exit unexpectedly in multiple threads. #111751

Closed

kcatss mentioned this pull request Nov 15, 2023

Use-after-free in unregister() of atexit module #112127

Open

kcatss mentioned this pull request Jan 14, 2024

crash in long_vectorcall in longobject.c #114050

Closed

kcatss mentioned this pull request Feb 20, 2024

Use After Free at _heapreplace_max #115706

Closed

ngoldbaum mentioned this pull request Sep 23, 2024

Crash running PyO3 tests with --test-threads=1000 #124375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-10746: ctypes: Fix PEP 3118 type codes for c_long, c_bool, c_int #31

bpo-10746: ctypes: Fix PEP 3118 type codes for c_long, c_bool, c_int #31

pv commented Feb 11, 2017 •

edited by bedevere-bot

Loading

the-knights-who-say-ni commented Feb 11, 2017

codecov bot commented Feb 11, 2017

pv commented Feb 14, 2017 via email

pitrou Jun 6, 2017

pitrou Jun 6, 2017

pv Jun 22, 2017

pv Jun 22, 2017 •

edited

Loading

pitrou Jun 6, 2017

pitrou Jun 6, 2017

pitrou Jun 6, 2017

pv commented Jun 22, 2017

pv commented Aug 26, 2017

pitrou commented Aug 26, 2017

pitrou Aug 26, 2017

pitrou Aug 26, 2017

pitrou commented Aug 26, 2017

pv commented Aug 26, 2017 via email

pv commented Aug 26, 2017 via email

pv commented Aug 26, 2017 via email

pitrou commented Aug 26, 2017

pv commented Aug 26, 2017 via email

pv commented Aug 26, 2017 via email

pitrou commented Aug 26, 2017

pv commented Aug 26, 2017 via email •

edited

Loading

pv commented Aug 26, 2017

pitrou commented Aug 28, 2017

pv commented Aug 28, 2017

pitrou commented Aug 28, 2017

pv commented Aug 28, 2017 via email

pitrou commented Aug 28, 2017

bpo-10746: ctypes: Fix PEP 3118 type codes for c_long, c_bool, c_int #31

bpo-10746: ctypes: Fix PEP 3118 type codes for c_long, c_bool, c_int #31

Conversation

pv commented Feb 11, 2017 • edited by bedevere-bot Loading

the-knights-who-say-ni commented Feb 11, 2017

codecov bot commented Feb 11, 2017

Codecov Report

pv commented Feb 14, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pv Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pv commented Jun 22, 2017

pv commented Aug 26, 2017

pitrou commented Aug 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Aug 26, 2017

pv commented Aug 26, 2017 via email

pv commented Aug 26, 2017 via email

pv commented Aug 26, 2017 via email

pitrou commented Aug 26, 2017

pv commented Aug 26, 2017 via email

pv commented Aug 26, 2017 via email

pitrou commented Aug 26, 2017

pv commented Aug 26, 2017 via email • edited Loading

pv commented Aug 26, 2017

pitrou commented Aug 28, 2017

pv commented Aug 28, 2017

pitrou commented Aug 28, 2017

pv commented Aug 28, 2017 via email

pitrou commented Aug 28, 2017

pv commented Feb 11, 2017 •

edited by bedevere-bot

Loading

pv Jun 22, 2017 •

edited

Loading

pv commented Aug 26, 2017 via email •

edited

Loading