On Tue, 11 Dec 2012 08:16:27 +0100
Antoine Pitrou <solipsis@pitrou.net> wrote:

> On Tue, 11 Dec 2012 03:05:19 +0100 (CET)
> gregory.p.smith <python-checkins@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).
>
> > 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.

Ok, I hadn't seen your proposal. I find it reasonable:

�A more correct non-hacky alternative if any alignment issues are still
found would be to use a compiler specific alignment declaration on the
structure and determine which value to use at configure time.�


However, the commit is still problematic, and I think it should be
reverted. We can't remove the alignment hack just because it seems to
be useless on x86(-64).

I didn't remove it. �I made it match what our memory allocator is already doing.

Thanks for reviewing commits in such detail BTW. �I do appreciate it.
">

(original) (raw)


On Mon, Dec 10, 2012 at 11:21 PM, Antoine Pitrou <solipsis@pitrou.net> wrote:

On Tue, 11 Dec 2012 08:16:27 +0100
Antoine Pitrou <solipsis@pitrou.net> wrote:

\> On Tue, 11 Dec 2012 03:05:19 +0100 (CET)
\> gregory.p.smith <python-checkins@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).
\>
\> > 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.

Ok, I hadn't seen your proposal. I find it reasonable:

�A more correct non-hacky alternative if any alignment issues are still
found would be to use a compiler specific alignment declaration on the
structure and determine which value to use at configure time.�


However, the commit is still problematic, and I think it should be
reverted. We can't remove the alignment hack just because it seems to
be useless on x86(-64).

I didn't remove it. �I made it match what our memory allocator is already doing.

Thanks for reviewing commits in such detail BTW. �I do appreciate it.

BTW, I didn't notice your replies until now because you didn't include me in the to/cc list on the responses. �Please do that if you want a faster response. :)

-gps