Issue 19537: Fix misalignment in fastsearch_memchr_1char

sizeof(foo) is never a good approximation for the alignment of foo

Current code assumes that PyUnicode_DATA() is aligned to PyUnicode_KIND() bytes. If this is not true on some platform it will be easer to add a padding than rewrite a code. A lot of code depends on this assumption. See also issue14422.

I afraid that proposed patch may slow down a search.

I don't understand in which concrete situation the current code could be wrong. The start of the unicode string should always be aligned, due to how unicode objects are allocated.

(gdb) p sizeof(PyASCIIObject)
$1 = 22

Andreas Schwab <report@bugs.python.org> wrote:
> (gdb) p sizeof(PyASCIIObject)
> $1 = 22

m68k again? ;)

There is nothing wrong with that.

> (gdb) p sizeof(PyASCIIObject)
> $1 = 22

If the only issue is that the size should be 24 instead of 22,
could we perhaps add an unused uint16_t to the struct just
for this architecture?

What is sizeof(PyUnicodeObject)?

(gdb) p sizeof(PyUnicodeObject)
$1 = 38

Antoine, if sizeof(PyUnicodeObject) is 38, the start of UCS4 unicode string is not aligned. This should be fixed.

> Antoine, if sizeof(PyUnicodeObject) is 38, the start of UCS4 unicode
> string is not aligned. This should be fixed.

Probably. Does anyone want to propose a patch?

How about adding explicit padding to the bitfield in PyASCIIObject?

> How about adding explicit padding to the bitfield in PyASCIIObject?

That sounds ok to me, but it must be explicitly for 68k (or use a
sufficiently smart scheme not to affect already aligned architectures).

If you compile Python with GCC, we can maybe try something with __attribute__ ((aligned (sizeof(void *)))) attribute. The attribute can be used on a structure field. The problem is that we don't care of the alignment of header attributes, only of data, but data is not a field but the data just after the structure.

Or is it possible to GCC to get a structure size aligned on 4 bytes?

For the explicit padding: how do you compute the size of the padding?

How about disabling the fast-path in FASTSEARCH if data is not aligned?

(You might get non-aligned if even if structure is correctly aligned, it may happen if the memory allocator does not align memory blocks. I don't know if Python may get "unaligned" memory blocks.)

I think that adding __attribute__ ((aligned (sizeof(void *)))) to the PyObject type (or to the ob_base field) is enough.

If first byte of structure is aligned, then first byte past the structure should be aligned too.

The attached patch has been tested on {i586,x86_64,ppc,ppc64,ppc64le,armv6hl,armv7hl,aarch64,m68k}-suse-linux.

Are unnamed struct fields actually valid C?

Yes, §6.7.2.1:

A bit-field declaration with no declarator, but only a colon and a width, indicates an unnamed bit-field.

With a footnote:

105) An unnamed bit-field structure member is useful for padding to conform to externally imposed layouts.


Unnamed struct members are valid, too:

There may be unnamed padding at the end of a structure or union.

New changeset 004ae1472a43 by Antoine Pitrou in branch '3.4':
Issue #19537: Fix PyUnicode_DATA() alignment under m68k.  Patch by Andreas Schwab.
http://hg.python.org/cpython/rev/004ae1472a43

New changeset 0128b25068de by Antoine Pitrou in branch 'default':
Issue #19537: Fix PyUnicode_DATA() alignment under m68k.  Patch by Andreas Schwab.
http://hg.python.org/cpython/rev/0128b25068de

Ok, I've committed the patch, then. Thank you, Andreas.

What is the size of the PyASCIIObject on x86/x64 with and without the
patch? GCC and Visual Studio provide attributes to align structures.

> What is the size of the PyASCIIObject on x86/x64 with and without the
> patch?

Haven't tried on x86, but on x86-64 it's the same. If it changes it will
probably get detected by the sys.getsizeof() tests.

2014-03-24 0:30 GMT+01:00 Antoine Pitrou <report@bugs.python.org>:
>> What is the size of the PyASCIIObject on x86/x64 with and without the
>> patch?
>
> Haven't tried on x86, but on x86-64 it's the same. If it changes it will
> probably get detected by the sys.getsizeof() tests.

test_sys has been modified:

-        asciifields = "nnbP"
+        asciifields = "nn4bP"

It would like to know if objects are bigger with the change or not.

Look at the commits, not the patch.

> Look at the commits, not the patch.

Ah! You wrote "Ok, I've committed the patch". Ok, your commit is fine. I would prefered a named field (called "padding"), but it's fine ;-)

Martin, Larry msg214639 states "Fix PyUnicode_DATA() alignment under m68k.", your comments please.

Mark Lawrence, stop playing off committers against each other. This is
outright trolling.

Please do not threaten me, I've been told that I'm to be ignored, and I now observe that committers seem to be sneaking patches into the source control system when the platform is unsupported.  I'll stop placing comments on appropriate issues if and only if I get satisfactory answers to my questions, which so far are conspicuous by their absence.

stage: needs patch -> resolved

versions: + Python 3.4