On Mon, Dec 10, 2012 at 11:21 PM, Antoine Pitrou solipsis@pitrou.netwrote:
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