[Python-Dev] cpython: Using 'long double' to force this structure to be worst case aligned is no

[Gregory P. Smith]

On Thu, Dec 13, 2012 at 11:27 PM, Gregory P. Smith <greg at krypto.org> wrote:

>
> On Mon, Dec 10, 2012 at 11:16 PM, Antoine Pitrou <solipsis at pitrou.net>wrote:
>
>> On Tue, 11 Dec 2012 03:05:19 +0100 (CET)
>> gregory.p.smith <python-checkins at python.org> wrote:
>> >   Using 'long double' to force this structure to be worst case aligned
>> is no
>> > longer required as of Python 2.5+ when the gc_refs changed from an int
>> (4
>> > bytes) to a Py_ssize_t (8 bytes) as the minimum size is 16 bytes.
>> >
>> > The use of a 'long double' triggered a warning by Clang trunk's
>> > Undefined-Behavior Sanitizer as on many platforms a long double requires
>> > 16-byte alignment but the Python memory allocator only guarantees 8 byte
>> > alignment.
>> >
>> > So our code would allocate and use these structures with technically
>> improper
>> > alignment.  Though it didn't matter since the 'dummy' field is never
>> used.
>> > This silences that warning.
>> >
>> > Spelunking into code history, the double was added in 2001 to force
>> better
>> > alignment on some platforms and changed to a long double in 2002 to
>> appease
>> > Tru64.  That issue should no loner be present since the upgrade from
>> int to
>> > Py_ssize_t where the minimum structure size increased to 16 (unless
>> anyone
>> > knows of a platform where ssize_t is 4 bytes?)
>>
>> What?? Every 32-bit platform has a 4 bytes ssize_t (and size_t).
>>
>
> No they don't.
>
> size_t and ssize_t exist in large part because they are often larger than
> an int or long on 32bit platforms.  They are 64-bit on Linux regardless of
> platform (i think there is a way to force a compile in ancient mode that
> forces them and the APIs being used to be 32-bit size_t variants but nobody
> does that).
>
>
>> > We can probably get rid of the double and this union hack all together
>> today.
>> > That is a slightly more invasive change that can be left for later.
>>
>> How do you suggest to get rid of it? Some platforms still have strict
>> alignment rules and we must enforce that PyObjects (*) are always
>> aligned to the largest possible alignment, since a PyObject-derived
>> struct can hold arbitrary C types.
>>
>> (*) GC-enabled PyObjects, anyway. Others will be naturally aligned
>> thanks to the memory allocator.
>>
>>
>> What's more, I think you shouldn't be doing this kind of change in a
>> bugfix release. It might break compiled C extensions since you are
>> changing some characteristics of object layout (although you would
>> probably only break those extensions which access the GC header, which
>> is probably not many of them). Resource consumption improvements
>> generally go only into the next feature release.
>>
>
BTW - This change was done on tip only. The comment about this being 'in a
bugfix release' is wrong.

While I personally believe this is needed in all of the release branches I
didn't commit this one there *just in case* there is some weird platform
where this change actually makes a difference. I don't believe such a thing
exists in 2012, but as there is no way that is worth my time for me to find
that out, I didn't put it in a bugfix branch.

-gps


> This isn't a resource consumption improvement.  It is a compilation
> correctness change with zero impact on the generated code or ABI
> compatibility before and after.  The structure, as defined, is was flagged
> as problematic by Clang's undefined behavior sanitizer because it contains
> a 'long double' which requires 16-byte alignment but Python's own memory
> allocator was using an 8 byte boundary.
>
> So changing the definition of the dummy side of the union makes zero
> difference to already compiled code as it (a) doesn't change the
> structure's size and (b) all existing implementations already align these
> on an 8 byte boundary.
>
> -gps
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-dev/attachments/20121213/a4007c79/attachment.html>