struct.pack of floats in non-native endian order

[richardh2003]

BPO	705836
Nosy	@tim-one, @facundobatista, @mdickinson, @tiran
Files	705836.patch 705836_v2.patch: Updated patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/mdickinson'
closed_at = <Date 2008-03-14.14:24:51.636>
created_at = <Date 2003-03-18.20:17:11.000>
labels = ['extension-modules']
title = 'struct.pack of floats in non-native endian order'
updated_at = <Date 2008-03-14.14:24:51.612>
user = 'https://bugs.python.org/richardh2003'

bugs.python.org fields:

activity = <Date 2008-03-14.14:24:51.612>
actor = 'mark.dickinson'
assignee = 'mark.dickinson'
closed = True
closed_date = <Date 2008-03-14.14:24:51.636>
closer = 'mark.dickinson'
components = ['Extension Modules']
creation = <Date 2003-03-18.20:17:11.000>
creator = 'richardh2003'
dependencies = []
files = ['9240', '9637']
hgrepos = []
issue_num = 705836
keywords = ['patch']
message_count = 14.0
messages = ['15183', '15184', '15185', '15186', '15187', '15188', '60170', '60255', '60256', '60262', '60265', '62087', '63405', '63528']
nosy_count = 6.0
nosy_names = ['tim.peters', 'collinwinter', 'facundobatista', 'mark.dickinson', 'richardh2003', 'christian.heimes']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue705836'
versions = ['Python 2.6', 'Python 3.0']

[richardh2003]

Python version 2.1 (2.2.1 behaves the same).
Windows 2000 and RH Linux 8.0

This was run on an Intel platform.

>>> v = 7.0 + .1 + .1 + .1 + .1 + .1 + .1 + .1 + .1 + .1 
+ .1
>>> v
7.9999999999999964
>>> struct.pack( "f", v )
'\x00\x00\x00A'
>>> struct.pack( ">f", v )
'@\x80\x00\x00'

These two should be the same byte pattern (only
reversed)!

>>> struct.unpack( ">f", '@\x80\x00\x00' )
(4.0,)

!!!!!

[tim-one]

Logged In: YES
user_id=31435

Yuck. It's a bug in not accounting for that rounding can
spill over the original bit width. structmodule's pack_float()
and pack_double() both have this flaw, although the one in
pack_double() is much subtler. A similar cut-and-paste
bug is in cPicke's save_float(). I'll fix all this.

Note: while "<f"'s result should be the byte-reversal
of ">f"'s, there's no defined relationship between either of
those and plain "f". "f" is platform-native in all respects. "<f"
and ">f" force an IEEE-like encoding, even on non-IEEE
platforms.

[tim-one]

Logged In: YES
user_id=31435

Boosted priority because this is an especially bad kind of
bug: mysterious, subtle, and very rare ("most" in-range
floats pack under "<f" and ">f" without problems; a problem
requires that a carry out of the 25th most-significant-bit
propagate thru a solid string of 24 1 bits).

For 2.2 I expect to check in a quick hack. In 2.3 this code
needs refactoring (structmodule and cPickle shouldn't have
duplicated this delicate code)

[richardh2003]

Logged In: YES
user_id=737060

Thanks for getting back to me.

Your comment regarding IEEE formats is very interesting, I
didn't know about this behaviour.

[tim-one]

Logged In: YES
user_id=31435

Fixed. In the 2.2 branch:

Lib/test/test_struct.py; new revision: 1.14.12.1
Misc/NEWS; new revision: 1.337.2.4.2.68
Modules/cPickle.c; new revision: 2.73.2.1.2.4
Modules/structmodule.c; new revision: 2.51.8.2

For 2.3:

Lib/test/test_struct.py; new revision: 1.17
Misc/NEWS; new revision: 1.700
Modules/cPickle.c; new revision: 2.141
Modules/structmodule.c; new revision: 2.58

[collinwinter]

Reopening. The test case committed in r31892 is broken, specifically the part that checks for an OverflowError towards the end: the TestFailed exception is only instanced, never raised. Actually raising the TestFailed instance causes the test to fail.

[facundobatista]

A lot of water passed around this bridge, but I don't know if this is fixed:

In the trunk right, now:

>>> v = 7.0 + .1 + .1 + .1 + .1 + .1 + .1 + .1 + .1 + .1 + .1
>>> v
7.9999999999999964
>>> p = struct.pack( ">f", v )
>>> p
'A\x00\x00\x00'
>>> struct.unpack(">f", p)
(8.0,)

[mdickinson]

It's a little odd: the relevant code is in floatobject.c, in _PyFloat_Pack4. The
issue is what happens when a Python float (stored internally as a platform double)
is packed as an IEEE-754 single-precision float. The current code doesn't behave
consistently across platforms:

• if the platform float format is unknown, the code turns a Python float into an
  IEEE-754 float anyway, and goes to great lengths to raise OverflowError when the
  Python float is too large to fit into an IEEE single-precision float.

• if the platform float format is recognised as IEEE-754, then a C cast is used
  to turn the Python float (stored as a double) into a single-precision float; for
  something that's too large for single precision, the natural result of that
  conversion is an IEEE-754 infinity. I'm not 100% sure that the C standard (even
  C99) actually guarantees this, or whether there's a danger of floating-point
  exceptions being raised here.

I think the behaviour should be consistent: either always raise OverflowError or
always return the 4-byte sequence corresponding to an infinity. I'm not sure
which. From the test-suite, it's clear that the original intention was that
OverflowError should be raised. On the other hand, 98.22% of Python users (i.e.
those on IEEE-754 platforms) have been living happily with the 'output an
infinity' behaviour for the last 5 years without noticing. And we've been moving
towards exposing infinities and NaNs more within Python.

One the OverflowError/infinity decision is made, this is a relatively easy fix,
and I'd be happy to take care of it.

Christian, do you have any thoughts on this issue?

[mdickinson]

Aha: the �C99 standard, section 6.3.1.5, says:

When a double is demoted to float, a long double is demoted to double or
float, or a value being represented in greater precision and range than required by its
semantic type (see 6.3.1.8) is explicitly converted to its semantic type [...] if the value
being converted is outside the range of values that can be represented, the behavior is
undefined.

So the current code likely has different behaviours even on different IEEE-754 platforms.

[mdickinson]

Here's a patch that fixes the test that Collin mentioned to reflect what's actually been
happening for the last nearly 5 years, and changes _PyFloat_Pack4 and _PyFloat_Pack8, as
follows.

When packing a float that's too large for the destination format (e.g. pack(">f", 1e39)):

• before the patch, _PyFloat_Pack* gives an OverflowError on non-IEEE-754 platforms and
  an IEEE infinity on IEEE-754 platforms.

• after the patch, _PyFloat_Pack* gives an IEEE infinity on all platforms.

This patch doesn't fix the problem that the cast from double to float on IEEE machines
involves potentially undefined behaviour; I think that should be considered a separate
issue.

[tiran]

Sounds like a good idea to me. Although all major platforms support IEEE
Python should guarantee a stable behavior on all platforms.

The lines should be a safe replacement of the downcast:
double fromd;
float tof;
tof = abs(fromd) >= FLT_MAX ? FLT_MAX : fromd;
tof = (float)copysign((double)tof, fromd);

By the way the release notes of Python should mention the our work on
better IEEE-754 and numeric support in bold letters. "Python 2.6 - now
with even better support for professional math in the core". It's a good
advertisement. :)

[mdickinson]

I'm stealing this issue from Tim, and downgrading the priority to normal
(it was the original bug that was high priority).

[mdickinson]

Coming back to this, I think that it actually *is* clear what
struct(">f", large_float) should do: it should raise OverflowError.
This fits in well with the general philosophy that Python (at least
tries to) follow for floating-point exceptions.

The attached patch (705836_v2.patch) fixes this. All tests pass with
this patch applied.

I'll leave this around for a few days in case anyone wants to comment on
it; then I plan to apply it to the trunk.

[mdickinson]

Fixed in r61383.