bpo-46906: Add PyFloat_Pack8() to the C API #31657

vstinner · 2022-03-03T10:46:56Z

Add new functions to pack and unpack C double (serialize and
deserialize):

PyFloat_Pack2()
PyFloat_Pack4()
PyFloat_Pack8()
PyFloat_Unpack2()
PyFloat_Unpack4()
PyFloat_Unpack8()

Document these functions and add unit tests.

Rename functions and move them from the internal C API to the public
C API:

_PyFloat_Pack2() => PyFloat_Pack2()
_PyFloat_Pack4() => PyFloat_Pack4()
_PyFloat_Pack8() => PyFloat_Pack8()
_PyFloat_Unpack2() => PyFloat_Unpack2()
_PyFloat_Unpack4() => PyFloat_Unpack4()
_PyFloat_Unpack8() => PyFloat_Unpack8()

Replace the "unsigned char*" type with "char*" which is more common
and easy to use.

https://bugs.python.org/issue46906

vstinner · 2022-03-03T10:47:12Z

cc @methane @mdickinson

Doc/c-api/float.rst

arhadthedev · 2022-03-03T11:33:45Z

Modules/_ctypes/cfield.c

@@ -1057,10 +1056,10 @@ f_set_sw(void *ptr, PyObject *value, Py_ssize_t size)
    if (x == -1 && PyErr_Occurred())
        return NULL;
 #ifdef WORDS_BIGENDIAN


Can the le argument of PyFloat_{Un}Pack* be replaced with three-value endianness ( big, little, and native)? The native option would allow to move all #ifdef WORDS_BIGENDIAN fiddling inside the (up)pack functions.

If this parameter is changed, it will be way harder to provide this function in pythoncapi_compat to older Python versions: python/pythoncapi-compat#26 And it maybe become more complicated to write a single code base supporting old and new Python versions.

It's easy to implement the "native" behavior by checking if your system uses little endian or not with your preferred way.

marshal uses little endian

struct supports big and little endian

pickle uses big endian

ctypes supports big and little endian

msgpack uses big endian

So far, I didn't see a module using the "native endian". For a serialization function, it's not convenient.

I wrote #31832 to suggest using PY_BIG_ENDIAN macro to use the native endian.

Lib/test/test_float.py

Doc/c-api/float.rst

bedevere-bot · 2022-03-03T13:10:27Z

🤖 New build scheduled with the buildbot fleet by @vstinner for commit a02f8e9ec0c570411ce862dc2d42004cc833aeb9 🤖

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

vstinner · 2022-03-03T13:10:54Z

I launch a job on buildbots to see how tests go on various architectures and platforms.

vstinner · 2022-03-03T13:47:51Z

So far, tests pass on:

OS: Windows, macOS, FreeBSD, Linux (Debian, Fedora, Gentoo, RHEL8, SLES)
Arch: x86 (32 bit, x87?, little endian), x86-64 (64 bit, SSE?, little endian), ARM64 (little endian), PPC64 (big endian), s390x (big endian)

vstinner · 2022-03-03T16:06:59Z

Hum, I'm not sure if the PR is related to the USAN failure.

AMD64 Arch Linux Usan PR failed:

test_ints (ctypes.test.test_bitfields.C_Test) ... ok
/buildbot/buildarea/pull_request.pablogsal-arch-x86_64.clang-ubsan/build/Modules/_ctypes/cfield.c:554:5: runtime error: shift exponent 18446744073709551614 is too large for 16-bit type 'short'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /buildbot/buildarea/pull_request.pablogsal-arch-x86_64.clang-ubsan/build/Modules/_ctypes/cfield.c:554:5 in

It's a test on the h format code:

#define LOW_BIT(x)  ((x) & 0xFFFF)
#define NUM_BITS(x) ((x) >> 16)

#define GET_BITFIELD(v, size)                                           \
    if (NUM_BITS(size)) {                                               \
        v <<= (sizeof(v)*8 - LOW_BIT(size) - NUM_BITS(size));           \
        v >>= (sizeof(v)*8 - NUM_BITS(size));                           \

static PyObject *
h_get(void *ptr, Py_ssize_t size)
{
    short val;
    memcpy(&val, ptr, sizeof(val));
    GET_BITFIELD(val, size); // <==== HERE
    return PyLong_FromLong((long)val);
}

static struct fielddesc formattable[] = {
    ...
    { 'h', h_set, h_get, NULL, h_set_sw, h_get_sw},
    ...
};

vstinner · 2022-03-03T16:34:58Z

AMD64 Arch Linux Usan PR failed

Oh, I fact 3 tests failed: test_ctypes, test_faulthandler, test_hashlib.

But it's unrelated to the PR, UBSan is also failing without this PR. I created: https://bugs.python.org/issue46913

mdickinson

I've left a few comments, mostly on the docs.

Doc/c-api/float.rst

mdickinson · 2022-03-05T11:00:45Z

Doc/c-api/float.rst

+attempting to unpack a bytes string containing an IEEE INF or NaN will raise an
+exception.
+
+On non-IEEE platforms with more precision, or larger dynamic range, than IEEE


Presumably once we're assuming IEEE 754 for CPython, all this text can be dropped?

My plan is to first fix the Python 3.11 "regression" by exposing these functions as public C API functions. And then deprecate or remove support for non-IEEE 754 platforms.

Require IEEE 754 support is tracked by https://bugs.python.org/issue46917

Doc/c-api/float.rst

Add new functions to pack and unpack C double (serialize and deserialize): * PyFloat_Pack2() * PyFloat_Pack4() * PyFloat_Pack8() * PyFloat_Unpack2() * PyFloat_Unpack4() * PyFloat_Unpack8() Document these functions and add unit tests. Rename functions and move them from the internal C API to the public C API: * _PyFloat_Pack2() => PyFloat_Pack2() * _PyFloat_Pack4() => PyFloat_Pack4() * _PyFloat_Pack8() => PyFloat_Pack8() * _PyFloat_Unpack2() => PyFloat_Unpack2() * _PyFloat_Unpack4() => PyFloat_Unpack4() * _PyFloat_Unpack8() => PyFloat_Unpack8() Replace the "unsigned char*" type with "char*" which is more common and easy to use.

vstinner · 2022-03-10T08:33:24Z

@mdickinson: I addressed your comments. Since the documentation is quite long and complicated, I decided to remove it from Include/cpython/floatobject.h.

vstinner · 2022-03-11T23:12:23Z

Thanks everyone for your valuable reviews! I merged my PR.

There is likely room for enhancement on the doc, but it will be easier to read it once it will be rendered as HTML online at: https://docs.python.org/dev/c-api/float.html First, I only moved the existing doc from comments to the Python doc. This PR was a great opportunity to update the outdated doc and fix it!

Dropping support for non-IEEE 754 floating pointer formats is now tracked by https://bugs.python.org/issue46917

vstinner · 2022-03-11T23:40:12Z

Functions added to pythoncapi_compat: python/pythoncapi-compat@6d2c96e

vstinner requested a review from tiran as a code owner March 3, 2022 10:46

bedevere-bot added the awaiting core review label Mar 3, 2022

the-knights-who-say-ni added the CLA signed label Mar 3, 2022

vstinner mentioned this pull request Mar 3, 2022

Add PyFloat_Pack8() python/pythoncapi-compat#26

Merged

kumaraditya303 reviewed Mar 3, 2022

View reviewed changes

Doc/c-api/float.rst Outdated Show resolved Hide resolved

arhadthedev reviewed Mar 3, 2022

View reviewed changes

kumaraditya303 reviewed Mar 3, 2022

View reviewed changes

Lib/test/test_float.py Outdated Show resolved Hide resolved

kumaraditya303 reviewed Mar 3, 2022

View reviewed changes

Lib/test/test_float.py Outdated Show resolved Hide resolved

kumaraditya303 reviewed Mar 3, 2022

View reviewed changes

Lib/test/test_float.py Outdated Show resolved Hide resolved

methane reviewed Mar 3, 2022

View reviewed changes

Doc/c-api/float.rst Outdated Show resolved Hide resolved

vstinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 3, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 3, 2022

methane approved these changes Mar 3, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Mar 3, 2022

mdickinson reviewed Mar 5, 2022

View reviewed changes

vstinner added 2 commits March 10, 2022 09:28

Address Mark's review

9371cde

vstinner merged commit 882d809 into python:main Mar 11, 2022

bedevere-bot removed the awaiting merge label Mar 11, 2022

vstinner deleted the float_pack branch March 11, 2022 23:10

vstinner mentioned this pull request May 11, 2022

bpo-46906: Move _PyFloat_{Pack,Unpack}{4,8} from internal to cpython #31649

Closed

ghost mentioned this pull request Nov 17, 2023

python3.11 fixes xnd-project/libxnd#48

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-46906: Add PyFloat_Pack8() to the C API #31657

bpo-46906: Add PyFloat_Pack8() to the C API #31657

vstinner commented Mar 3, 2022 •

edited by bedevere-bot

Loading

vstinner commented Mar 3, 2022

arhadthedev Mar 3, 2022

vstinner Mar 3, 2022

vstinner Mar 12, 2022

bedevere-bot commented Mar 3, 2022

vstinner commented Mar 3, 2022

vstinner commented Mar 3, 2022

vstinner commented Mar 3, 2022

vstinner commented Mar 3, 2022

mdickinson left a comment

mdickinson Mar 5, 2022

vstinner Mar 10, 2022

vstinner Mar 10, 2022

vstinner commented Mar 10, 2022

vstinner commented Mar 11, 2022

vstinner commented Mar 11, 2022

bpo-46906: Add PyFloat_Pack8() to the C API #31657

bpo-46906: Add PyFloat_Pack8() to the C API #31657

Conversation

vstinner commented Mar 3, 2022 • edited by bedevere-bot Loading

vstinner commented Mar 3, 2022

arhadthedev Mar 3, 2022

Choose a reason for hiding this comment

vstinner Mar 3, 2022

Choose a reason for hiding this comment

vstinner Mar 12, 2022

Choose a reason for hiding this comment

bedevere-bot commented Mar 3, 2022

vstinner commented Mar 3, 2022

vstinner commented Mar 3, 2022

vstinner commented Mar 3, 2022

vstinner commented Mar 3, 2022

mdickinson left a comment

Choose a reason for hiding this comment

mdickinson Mar 5, 2022

Choose a reason for hiding this comment

vstinner Mar 10, 2022

Choose a reason for hiding this comment

vstinner Mar 10, 2022

Choose a reason for hiding this comment

vstinner commented Mar 10, 2022

vstinner commented Mar 11, 2022

vstinner commented Mar 11, 2022

vstinner commented Mar 3, 2022 •

edited by bedevere-bot

Loading