homepage

On m68k this assert triggers in Python/unicodeobject.c:4658 because m68k architectures can align on 16bit boundaries.
assert(!((size_t) dest & LONG_PTR_MASK));
I'm not sure of the wider implications in Python as to how to rectify.

We don't have a m86k test box and I don't think we support this platform either.

I'm willing to help fix, but there are m68k emulators out there which I guess suffice for a test box.

TBH, I don't think we should support this platform officially.
Is that processor still in use (e.g. in embedded systems)?

Wikipedia says "derivative processors are still widely used in embedded applications." in m68k article.

Freescale (ex-Motorola) ColdFire architecture is alive and doing well. And I know people still running Motorola 68040 Apple machines.
The problem is not a m68k emulator, but to build all the needed environment on it: OS, compilers, network, etc.
If the original submitter is willing to help...

Which branch or version of Python is that?

As mention in the versions. It is Python 3.3.0.

Ok, could you post the whole C stack trace? (using e.g. gdb)
By the way, this is not about the alignment of m68k architectures: x86 can align on a byte boundary and doesn't trigger the assert.

It must be about pointer alignment, because that's the whole point of the ASSERT.
As for the backtrace, the gdb support on the platform isn't great yet, but here it is....
Breakpoint 1, ascii_decode (start=0x30c5b04 "__len__", end=0x30c5b0b "", 
 dest=0x1a6d592 "�������") at Objects/unicodeobject.c:4658
4658	 assert(!((size_t) dest & LONG_PTR_MASK));
(gdb) bt
#0 ascii_decode (start=0x30c5b04 "__len__", end=0x30c5b0b "", dest=0x1a6d592 "�������")
 at Objects/unicodeobject.c:4658
#1 0x030595a6 in .L4737 () at Objects/unicodeobject.c:4741
#2 0x03044dba in .L2648 () at Objects/unicodeobject.c:1806
#3 0x03096f54 in PyUnicode_InternFromString (cp=0x30c5b04 "__len__")
 at Objects/unicodeobject.c:14284
#4 0x030c69f6 in .L1892 () at Objects/typeobject.c:6090
#5 0x030c6dc8 in add_operators (type=0x33507c8) at Objects/typeobject.c:6244
#6 0x030bfc66 in .L1249 () at Objects/typeobject.c:4182
#7 0x030bfbae in .L1241 () at Objects/typeobject.c:4146
#8 0x02ff62a8 in _Py_ReadyTypes () at Objects/object.c:1576
#9 0x0300e688 in .L60 () at Python/pythonrun.c:301
#10 0x0300ea5c in Py_InitializeEx (install_sigs=1) at Python/pythonrun.c:401
#11 0x0300ea6e in Py_Initialize () at Python/pythonrun.c:407
#12 0x02ff9fca in .L135 () at Modules/main.c:657
#13 0x02ff24be in .L6 () at ./Modules/python.c:90
#14 0x03329d5a in .L76 ()
#15 0x0331731e in .L69 ()

What will happened if you just remove this line?

> It must be about pointer alignment, because that's the whole point of
> the ASSERT.
Indeed, it's about pointer alignment, but it's not about the CPU. It's
about the compiler (or the platform's C ABI).
Apparently the compiler doesn't align struct fields to natural
boundaries like most other compilers do, which means the size of the
PyASCIIObject struct (in unicodeobject.h) ends up not being a multiple
of 4, which in turn means the "dest" pointer (allocated from the end of
that structure) is not 4 byte-aligned either.
However, you can probably safely remove the assert(), since it is there
to warn about misalignment on platforms which *are* alignment-sensitive.
There is another assert() of the same kind in unicodeobject.c, which you
can remove too.
It would be nice if the C source could be improved here, but it's not
obvious which rule to enforce exactly. We want to be lenient if the
misalignment is a product of the compiler's alignment rules, but not if
it's a mistake on our part.
Which compiler is it?

It's GCC 4.6.3.
GCC has the -malign-int but mentions this isn't in best interest of m68k ABI compatibility if it's used.

Oh, and as for pointer alignment, I probably just wasn't clear in the initial report. But we agree on m68k C ABI.

Ok, so we could simply disable the assert on m68k, if you can confirm it works. Do you want to provide a patch? I don't know what the preprocessor conditional should look like.

Perhaps we should disable not only assert, but all optimized code inside #if SIZEOF_LONG <= SIZEOF_VOID_P / #endif block. Benchmarks needed to measure either unaligned writing speedup or slowdown decoding.

@skrah: "I don't think we should support this platform officially."
Please don’t break what works. We have almost complete (about three quarters of roughly 10'000 source packages) Debian unstable working on m68k, with several versions of Python in use. Thanks!
@pitrou: "x86 can align on a byte boundary and doesn't trigger the assert."
That’s because most compilers on i386 do "natural alignment" – in fact, most compilers on most platforms do. "natural alignment" means 4-byte quantities get aligned on 4-byte boundaries, 8-byte quantities on 8-byte boundaries, etc.
On m68k, the lowest alignment for almost all larger-than-a-byte data types is 2 byte, even though that one is strict. This means that, for example, an int is often only 2-byte aligned.
@alanh: "GCC has the -malign-int but mentions this isn't in best interest of m68k ABI compatibility if it's used."
Indeed, because that would break the C and kernel/syscall ABI.
@all: what _precisely_ is the assertion needed to check for?
@pitrou: "since it is there to warn about misalignment on platforms which *are* alignment-sensitive"
Well, m68k is. Just the numbers differ. (It also has int, long and pointer at 32 bit.)
We had a similar issue in the Linux kernel, where it used the lower two bits of an address for flags (urgh...) which could only be solved by using GCC’s __attribute__((__aligned__(4))) on the quantities in question, but that may or may not be the required case here, which is why I’m asking.
I can test any trees on my VMs, but that takes a day or two, of course, at 50-200 BogoMIPS. You can do that yourself by running a VM as well, the Debian Wiki has instructions, if anyone is interested.
Otherwise, it’ll just get tested as soon as it hits Debian (unstable, usually we don’t build experimental packages except on explicit request by the packagers, due to lack of time / horsepower)...
Thanks doko for linking this issue!

> We had a similar issue in the Linux kernel, where it used the lower
> two bits of an address for flags (urgh...) which could only be solved by
> using GCC’s __attribute__((__aligned__(4))) on the quantities in
> question, but that may or may not be the required case here, which is
> why I’m asking.
It is not required since, as you say, m68k only requires 2-byte
alignment. However, as Serhiy said, it may (or may not) be better for
performance. At this point, only people with access to a m68k machine or
VM (and motivated enough :-)) can do the necessary tests and propose a
way forward.
(but, performance notwithstanding, fixing the build should be a simple
matter of silencing the assert with an appropriate #if line)

mirabilos <report@bugs.python.org> wrote:
> Please dont break what works. We have almost complete (about three quarters
> of roughly 10'000 source packages) Debian unstable working on m68k, with
> several versions of Python in use. Thanks!
Are you saying that the complete test suite works on m68k except for this
assert?

@pitrou: As for performance, 2-byte and 4-byte are the same on m68k, given that they usually have RAM which doesn’t benefit from that kind of alignment, and systems that are structured accordingly.
The "best" cpp define I don’t know, but my system defines __m68k__ and Alan would probably be able to say whether this is defined on ColdFire, too.
@skrah: No, I specifically did not say that ☺
But it works pretty damn well.

mirabilos, if you are motivated enough, do the following. Compile two Python executables - one with deleted assert, and second with deleted a block between "#if SIZEOF_LONG <= SIZEOF_VOID_P" and "#endif". Run following microbenchmarks for both executables:
./python -m timeit -s "x=b'A'*10000" "x.decode('ascii')"
./python -m timeit -s "x=b'A'*10000" "x.decode('utf-8')"

Serhiy Storchaka dixit:
>mirabilos, if you are motivated enough, do the following. Compile two
>Python executables - one with deleted assert, and second with deleted
>a block between "#if SIZEOF_LONG <= SIZEOF_VOID_P" and "#endif". Run
>following microbenchmarks for both executables:
>
>./python -m timeit -s "x=b'A'*10000" "x.decode('ascii')"
>./python -m timeit -s "x=b'A'*10000" "x.decode('utf-8')"
>
>----------
>
>_______________________________________
>Python tracker <report@bugs.python.org>
><http://bugs.python.org/issue17237>
>_______________________________________
Thanks, will actually do that, just not before the weekend,
dayjob’s keeping me busy, and I need to be careful to not
burn out myself in the evening too.
Which tree should I build? A release (if so, which)? Or
some CVS branch?
Do note we clock at roughly 1000 pystones for the fastest
machines... yes 1000 not 10000.
bye,
//mirabilos
-- 
FWIW, I'm quite impressed with mksh interactively. I thought it was much
*much* more bare bones. But it turns out it beats the living hell out of
ksh93 in that respect. I'd even consider it for my daily use if I hadn't
wasted half my life on my zsh setup. :-) -- Frank Terbeck in #!/bin/mksh

> Which tree should I build? A release (if so, which)? Or
> some CVS branch?
It doesn't matter.
> Do note we clock at roughly 1000 pystones for the fastest
> machines... yes 1000 not 10000.
It doesn't matter. Only relative values have a meaning. What is faster and on 
how many percent.

And of course run the tests on non-debug builds.

Serhiy Storchaka dixit:
>> Which tree should I build? A release (if so, which)? Or
>> some CVS branch?
>
>It doesn't matter.
Erm, still, which one do I build? Not 3.2 because it obviously
works, at least as packaged in Debian.
bye,
//mirabilos
-- 
Yay for having to rewrite other people's Bash scripts because bash
suddenly stopped supporting the bash extensions they make use of
	-- Tonnerre Lombard in #nosec

> Erm, still, which one do I build? Not 3.2 because it obviously
> works, at least as packaged in Debian.
Any >= 3.3.0.

Hi again,
sorry for being a bit late in following up to this.
Here are the results for the two runs you requested; these were done on the same machine, intermixed (the first one cache-clean, the second and third run subsequently (so that the third run is cache-filled)). I used static builds.
For the "without assert", I get approximately 570 usec per loop consistently:
(590,580), (570,570), (560,580), (570,560)
Interestingly enough, there seems to be no preference either way (ASCII or UTF-8).
For the "without the whole if..endif block", I get mixed results:
(650,540), (690,530), (520,530), (650,540)
Interesting here is that the third run of them both has a lower ASCII than UTF-8 time, the others don’t – but the UTF-8 run is the second in a row, so cache might be of an issue.
Repeating the runs, I get (560,590), (540,530) for the second case,
and (760,570), (590,660) for the first case breaking its "consistently
570 usec" streak. Error of measurement may be large, thus.
Which supports your suspicion that the optimised case may not be needed at all.
Matthias asked me to provide links how to set up an Atari VM with Debian unstable and a clean/minimal unstable chroot, since there’s much use of Ubuntu here who ship ARAnyM (I even filed a Sync Request so that the latest release got a fixed version ☺).
https://wiki.debian.org/Aranym/Quick#download has got pointers for the first part (setting up an ARAnyM VM), and https://wiki.debian.org/M68k/Installing contains the whole documentation (we cannot currently use d-i but people are working on it). http://people.debian.org/~tg/f/20121227/m68k-buildd.tgz is the output of "debootstrap --variant=buildd" and as such should be usable in either cowbuilder or sbuild. Considering that we *only* have unstable available, you may want to be careful when upgrading ;) but an apt-get update should work out of the box (takes a few minutes).
The VMs have 768+14 MiB RAM each in the sample configuration (maxed out), which makes them use a bit less than 1 GiB on the host side. A 3 GHz host CPU is of benefit.

> Which supports your suspicion that the optimised case may not be needed 
> at all.
So we can just skip the assert when __m68k__ is defined?

Could you please repeat the tests with a larger data (1MB or more)?

Antoine: precisely.
Serhiy: sure. The times are now in msec per loop.
I did three subsequent runs, so the second and
third tuple are cache-warm.
Without assert: (89,88), (87,87), (89,86)
Without block : (79,78), (78,78), (79,78)
And in a second run:
Without assert: (87,88), (87,88), (92,87)
Without block : (111,91), (78,85), (79,79)
This means that, again, removing the "optimised"
code speeds up operations (on this particular
slow architecture.

Well, it will be safer to skip this block on such platforms.

file30203/ascii_decode_nonaligned.patch is potentially a nop (the struct being a multiple of, in the m68k case 4, bytes is not an indicator of whether to skip it).
I think we can be bold and put #if !defined(__m68k__) and #endif around the entire block and, should there ever be another architecture with similar issues, whitelist them there.

> I think we can be bold and put #if !defined(__m68k__) and #endif
> around the entire block and, should there ever be another architecture
> with similar issues, whitelist them there.
Agreed.

What is sizeof(PyASCIIObject) on m68k?

Currently 22; it will increase to 24 if a few more bits are added. That’s why I said it’s "fragile" magic.

I’m currently thinking this patch.
(Will need another day or so for compiles to finish, though.)

+ dest is always aligned on common platforms 
+ (if sizeof(PyASCIIObject) is divisible by SIZEOF_LONG). 
Actually, that’s the part that is not true. On m68k, and perfectly
permitted by the C standard, even a 24-byte object has only a
guaranteed alignment of 2 bytes (or one, if it’s a char array)
in the normal case (contains integers and pointers, nothing special).
We patched out this bogus assumption from things like glib already ;)

PyASCIIObject is allocated on heap and should have a maximal alignment enough for every type. If sizeof(PyASCIIObject) % SIZEOF_LONG == 0 then dest is at least long-aligned. Currently sizeof(PyASCIIObject) is 22 on m68k and the optimization is switched off at compile time. When PyASCIIObject will grow to 24 bytes the optimization will switched on and perhaps will have some effect. I prefer checks for features instead of concrete names.

> PyASCIIObject is allocated on heap and should have a maximal alignment
> enough for every type. If sizeof(PyASCIIObject) % SIZEOF_LONG == 0
> then dest is at least long-aligned. Currently sizeof(PyASCIIObject) is
> 22 on m68k and the optimization is switched off at compile time. When
> PyASCIIObject will grow to 24 bytes the optimization will switched on
> and perhaps will have some effect. I prefer checks for features
> instead of concrete names.
This is a bugfix, please let's keep it simple. Checking for __m68k__
ensures that other architectures aren't affected by mistake.

Right, keeping it simple helps in preventing accidents, and the code block looks full of magic enough as-is.
Maybe add a comment block that says:
/*
 * m68k is a bit different from most architectures in that objects
 * do not use "natural alignment" - for example, int and long are
 * only aligned at 2-byte boundaries. Tests have shown that skipping
 * the "optimised version" will even speed up m68k, so we #ifdef
 * for "the odd duck out" here.
 */
Then we have an in-situ documentation point for why that ifdef is there and why m68k is "the odd duck" and this whitelist method is used.

Well, then already too much bikeshedding for such simple fix. Antoine, do you want to commit a fix?

New changeset 0f8022ac88ad by Antoine Pitrou in branch '3.3':
Issue #17237: Fix crash in the ASCII decoder on m68k.
http://hg.python.org/cpython/rev/0f8022ac88ad
New changeset 201ae2d02328 by Antoine Pitrou in branch 'default':
Issue #17237: Fix crash in the ASCII decoder on m68k.
http://hg.python.org/cpython/rev/201ae2d02328 

Ok, I hope I got the fix right :) Thanks mirabilos for the comment suggestion, I used a modified version.

Thanks Antoine!
Now, for "finishing up" this... to follow up on Stefan’s comment... is there any way I can run the testsuite from an installed Python (from the Debian packages)? (I build the packages with disabled testsuite, to get the rest of the system running again, since python3.3 was recently made required and we had never built it before.)
Otherwise I guess I could run "make test" on one of the earlier trees I used for the timing... but that machine is currently building six Linux kernel flavours from the src:linux package and thus will not be available for the next one and a half week or so...

> Now, for "finishing up" this... to follow up on Stefan’s comment... is
> there any way I can run the testsuite from an installed Python (from
> the Debian packages)?
"python -m test" (with any options you might like), but we don't
guarantee that all tests pass on an installed Python. But at least you
will be able to spot any hard crashes :-)

Antoine Pitrou dixit:
>"python -m test" (with any options you might like), but we don't
No, I tried that (as it was the only thing I could find on the
’net as well) on an i386 system and only get:
tglase@tglase:~ $ python2.7 -m test
/usr/bin/python2.7: No module named test.__main__; 'test' is a package and cannot be directly executed
1|tglase@tglase:~ $ python3.2 -m test
/usr/bin/python3.2: No module named test.__main__; 'test' is a package and cannot be directly executed
Same when adding ‘-h’.
bye,
//mirabilos
-- 
Gast: „Ein Bier, bitte!"
Wirt: „Geht auch alkoholfrei?"
Gast: „Geht auch Spielgeld?"

> >"python -m test" (with any options you might like), but we don't
> 
> No, I tried that (as it was the only thing I could find on the
> ’net as well) on an i386 system and only get:
Ah, that's because the system Python install doesn't include the test
suite. Perhaps you have to install an additional package, python-dev
perhaps?
(note, on 2.7, it's "python -m test.regrtest")

Antoine Pitrou dixit:
>(note, on 2.7, it's "python -m test.regrtest")
That indeed does things. So I had mistaken them for the same problem.
>Ah, that's because the system Python install doesn't include the test
>suite. Perhaps you have to install an additional package, python-dev
>perhaps?
tglase@tglase:~ $ l /usr/lib/python2.7/test/
__init__.py pystone.py* regrtest.py* test_support.py
__init__.pyc pystone.pyc regrtest.pyc test_support.pyc
tglase@tglase:~ $ l /usr/lib/python3.2/test/
__init__.py __pycache__/ pystone.py* regrtest.py* support.py
Maybe it’s just not packaged... these are all I can find, and
installing python3.2-dev doesn’t fix it.
Oh well, then it’ll just have to wait. Do you have a preferred
place where I can submit the test results, as it’s getting
very off-topic here?
bye,
//mirabilos
-- 
 "Using Lynx is like wearing a really good pair of shades: cuts out
 the glare and harmful UV (ultra-vanity), and you feel so-o-o COOL."
 -- Henry Nelson, March 1999

> Oh well, then it’ll just have to wait. Do you have a preferred
> place where I can submit the test results, as it’s getting
> very off-topic here?
Well, if everything works fine, you don't have to submit them!
If you get test failures, you can open issues for the individual test
failures.