-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix misalignment in fastsearch_memchr_1char #63736
Comments
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 bpo-14422. 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) |
Andreas Schwab <report@bugs.python.org> wrote:
m68k again? ;) |
There is nothing wrong with that. |
If the only issue is that the size should be 24 instead of 22, |
What is sizeof(PyUnicodeObject)? |
(gdb) p sizeof(PyUnicodeObject) |
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? |
That sounds ok to me, but it must be explicitly for 68k (or use a |
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:
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': New changeset 0128b25068de by Antoine Pitrou in branch 'default': |
Ok, I've committed the patch, then. Thank you, Andreas. |
What is the size of the PyASCIIObject on x86/x64 with and without the |
Haven't tried on x86, but on x86-64 it's the same. If it changes it will |
2014-03-24 0:30 GMT+01:00 Antoine Pitrou <report@bugs.python.org>:
test_sys has been modified:
+ asciifields = "nn4bP" It would like to know if objects are bigger with the change or not. |
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 |
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. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: