[perl #118055] miniperl fails with SIGBUS on sparc (usethreads+use64bitint)

[Niko Tyni]

# New Ticket Created by Niko Tyni
# Please include the string: [perl #118055]
# in the subject line of all future correspondence about this issue.
# <URL: https://rt.perl.org:443/rt3/Ticket/Display.html?id=118055 >


This is a bug report for perl from Niko Tyni <nt...@debian.org>,
generated with the help of perlbug 1.39 running under perl 5.18.0.


-----------------------------------------------------------------
[ Sorry about the timing, it took a week for our sparc build server to get
round to building 5.18.0-RC1, and we got the result just hours after
the 5.18.0 release. ]


5.18.0 build fails on sparc with -Dusethreads -Duse64bitint.
miniperl gets a SIGBUS with just about any non-trivial program.

./miniperl -w -Ilib -MExporter -e '<?>' || /usr/bin/make minitest
./miniperl -Ilib make_patchnum.pl
make[1]: *** [lib/Config_git.pl] Bus error

An example backtrace:
Starting program: /home/ntyni/perl/miniperl -e s/a/b/
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/sparc-linux-gnu/libthread_db.so.1".

Program received signal SIGBUS, Bus error.
0x0002ffcc in Perl_newPMOP (my_perl=0x26e008, type=33, flags=0) at op.c:4458
4458 pmop->op_pmoffset = av_len(PL_regex_padav);
(gdb) bt
#0 0x0002ffcc in Perl_newPMOP (my_perl=0x26e008, type=33, flags=0) at op.c:4458
#1 0x0008c56c in S_scan_subst (my_perl=0x26e008, start=0x28c8f1 "/a/b/\n") at toke.c:9650
#2 0x0008421c in Perl_yylex (my_perl=0x26e008) at toke.c:8353
#3 0x000957e8 in Perl_yyparse (my_perl=0x26e008, gramtype=258) at perly.c:341
#4 0x00018294 in S_parse_body (my_perl=0x26e008, env=0x0, xsinit=0x4cfa4 <xs_init>) at perl.c:2309
#5 0x00016d94 in perl_parse (my_perl=0x26e008, xsinit=0x4cfa4 <xs_init>, argc=3, argv=0xffffdd04,
env=0x0) at perl.c:1626
#6 0x0004ce68 in main (argc=3, argv=0xffffdd04, env=0xffffdd14) at miniperlmain.c:111

This is also
http://bugs.debian.org/708792
and a full build log is at
https://buildd.debian.org/status/fetch.php?pkg=perl&arch=sparc&ver=5.18.0~rc1-1&stamp=1368891868

Perlbugging this with a non-threaded build. All the tests pass with that.
The actual Configure arguments used for testing were

./Configure -Doptimize="-g -O0" -des -Dusethreads -Duse64bitint

and both -Dusethreads -Duse64bitint are needed to trigger it.

Seen with gcc 4.4, 4.6 and 4.8, at both -O0 and -O2.
Also tried -DDEBUGGING with no visible difference.

Bisecting says the first bad commit is

commit 8be227ab5eaa23f2d21fd15f70190e494496dcbe
Author: Father Chrysostomos <spr...@cpan.org>
Date: Sat Jun 23 09:54:31 2012 -0700

CV-based slab allocation for ops

-----------------------------------------------------------------
---
Flags:
category=core
severity=high
---
Site configuration information for perl 5.18.0:

Configured by ntyni at Sun May 19 16:39:10 UTC 2013.

Summary of my perl5 (revision 5 version 18 subversion 0) configuration:
Commit id: a9acda3b5f74585852a57b51b724804ac586cb0b
Platform:
osname=linux, osvers=2.6.32-5-sparc64-smp, archname=sparc-linux-64int
uname='linux smetana 2.6.32-5-sparc64-smp #1 smp mon feb 25 02:19:08 utc 2013 sparc gnulinux '
config_args='-Doptimize=-g -O0 -des -Duse64bitint'
hint=recommended, useposix=true, d_sigaction=define
useithreads=undef, usemultiplicity=undef
useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
use64bitint=define, use64bitall=undef, uselongdouble=undef
usemymalloc=n, bincompat5005=undef
Compiler:
cc='cc', ccflags ='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
optimize='-g -O0',
cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
ccversion='', gccversion='4.6.3', gccosandvers=''
intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=87654321
d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
alignbytes=8, prototype=define
Linker and Libraries:
ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
libpth=/usr/local/lib /lib/sparc-linux-gnu /lib/../lib /usr/lib/sparc-linux-gnu /usr/lib/../lib /lib /usr/lib /lib64 /usr/lib64
libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
libc=, so=so, useshrplib=false, libperl=libperl.a
gnulibc_version='2.13'
Dynamic Linking:
dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
cccdlflags=' -fPIC', lddlflags='-shared -g -O0 -L/usr/local/lib -fstack-protector'

Locally applied patches:


---
@INC for perl 5.18.0:
lib
/usr/local/lib/perl5/site_perl/5.18.0/sparc-linux-64int
/usr/local/lib/perl5/site_perl/5.18.0
/usr/local/lib/perl5/5.18.0/sparc-linux-64int
/usr/local/lib/perl5/5.18.0
.

---
Environment for perl 5.18.0:
HOME=/home/ntyni
LANG=en_US.UTF-8
LANGUAGE (unset)
LC_CTYPE=fi_FI.UTF-8
LD_LIBRARY_PATH (unset)
LOGDIR (unset)
PATH=/usr/local/bin:/usr/bin:/bin:/usr/bin/X11:/usr/games
PERL_BADLANG (unset)
SHELL=/bin/zsh

[Leon Timmermans]

This smells like an alignment issue.

> Bisecting says the first bad commit is
>
> commit 8be227ab5eaa23f2d21fd15f70190e494496dcbe
> Author: Father Chrysostomos <spr...@cpan.org>
> Date: Sat Jun 23 09:54:31 2012 -0700
>
> CV-based slab allocation for ops

That makes perfect sense with my theory. Probably that allocator
should do some padding to 64-bit boundaries.

Leon

[Nicholas Clark]

On Mon, May 20, 2013 at 05:11:38AM +0200, Leon Timmermans wrote:
> On Sun, May 19, 2013 at 9:50 PM, Niko Tyni <perlbug-...@perl.org> wrote:
> > [ Sorry about the timing, it took a week for our sparc build server to get
> > round to building 5.18.0-RC1, and we got the result just hours after
> > the 5.18.0 release. ]
> >
> >
> > 5.18.0 build fails on sparc with -Dusethreads -Duse64bitint.
> > miniperl gets a SIGBUS with just about any non-trivial program.

> > 4458 pmop->op_pmoffset = av_len(PL_regex_padav);
> > (gdb) bt
> > #0 0x0002ffcc in Perl_newPMOP (my_perl=0x26e008, type=33, flags=0) at op.c:4458
> > #1 0x0008c56c in S_scan_subst (my_perl=0x26e008, start=0x28c8f1 "/a/b/\n") at toke.c:9650
> > #2 0x0008421c in Perl_yylex (my_perl=0x26e008) at toke.c:8353
> > #3 0x000957e8 in Perl_yyparse (my_perl=0x26e008, gramtype=258) at perly.c:341
> > #4 0x00018294 in S_parse_body (my_perl=0x26e008, env=0x0, xsinit=0x4cfa4 <xs_init>) at perl.c:2309
> > #5 0x00016d94 in perl_parse (my_perl=0x26e008, xsinit=0x4cfa4 <xs_init>, argc=3, argv=0xffffdd04,
> > env=0x0) at perl.c:1626
> > #6 0x0004ce68 in main (argc=3, argv=0xffffdd04, env=0xffffdd14) at miniperlmain.c:111
>
> This smells like an alignment issue.

Agree. Been caught out by these before.

> > Bisecting says the first bad commit is
> >
> > commit 8be227ab5eaa23f2d21fd15f70190e494496dcbe
> > Author: Father Chrysostomos <spr...@cpan.org>
> > Date: Sat Jun 23 09:54:31 2012 -0700
> >
> > CV-based slab allocation for ops
>
> That makes perfect sense with my theory. Probably that allocator
> should do some padding to 64-bit boundaries.

I think it needs to be to the alignment of the largest thing in an OP.
Which on a 32 bit sparc with 64 bit IVs is (currently) 64 bits:


struct pmop {
BASEOP
OP * op_first;
OP * op_last;
#ifdef USE_ITHREADS
IV op_pmoffset;
#else
REGEXP * op_pmregexp; /* compiled expression */
#endif


But really, that "IV" should be something else. "STRLEN", I think, or even
just size_t, because it's being used as an array index, and arrays can't
be 64 bits large on a 32 bit system.

We can't change that for 5.18.x. But as well as the alignment thing, we
should fix it for the future.

Nicholas Clark

[Peter Martini]

On Sun, May 19, 2013 at 3:50 PM, Niko Tyni <perlbug-...@perl.org> wrote:
> # New Ticket Created by Niko Tyni
> # Please include the string: [perl #118055]
> # in the subject line of all future correspondence about this issue.
> # <URL: https://rt.perl.org:443/rt3/Ticket/Display.html?id=118055 >
>
>
> This is a bug report for perl from Niko Tyni <nt...@debian.org>,
> generated with the help of perlbug 1.39 running under perl 5.18.0.
>
>
> -----------------------------------------------------------------
> [ Sorry about the timing, it took a week for our sparc build server to get
> round to building 5.18.0-RC1, and we got the result just hours after
> the 5.18.0 release. ]
>
>
> 5.18.0 build fails on sparc with -Dusethreads -Duse64bitint.
> miniperl gets a SIGBUS with just about any non-trivial program.
>
> ./miniperl -w -Ilib -MExporter -e '<?>' || /usr/bin/make minitest
> ./miniperl -Ilib make_patchnum.pl
> make[1]: *** [lib/Config_git.pl] Bus error

Interesting - I don't get the same result when building with those
args on Solaris 10/gcc:

-bash-3.2$ gcc --version
gcc (GCC) 3.4.3 (csl-sol210-3_4-branch+sol_rpath)
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

-bash-3.2$ uname -a
SunOS sunsmoke 5.10 Generic_147440-01 sun4v sparc SUNW,Sun-Fire-T1000

I'm rebuilding fresh now to see if I can get it to choke, and will
send the full -V output and test results when I'm done.

[Peter Martini]

Finished smoking on my Solaris 10 / SPARC box, and the same config
arguments - -Doptimize=-g -O0 -des -Dusethreads -Duse64bitint - work
there. (See below)

I'll see if I can get debian installed and running on this same
machine so I can duplicate the SIGBUS, and then compare the two. I'll
try and get a semirecent gcc version to test with on Solaris as well.

>>> Results and perl -V <<<

All tests successful.
u=104.41 s=13.91 cu=9818.08 cs=277.63 scripts=2324 tests=685517

Summary of my perl5 (revision 5 version 18 subversion 0) configuration:
Commit id: a9acda3b5f74585852a57b51b724804ac586cb0b
Platform:

osname=solaris, osvers=2.10, archname=sun4-solaris-thread-multi-64int
uname='sunos sunsmoke 5.10 generic_147440-01 sun4v sparc
sunw,sun-fire-t1000 '
config_args='-Doptimize=-g -O0 -des -Dusethreads -Duse64bitint'
hint=recommended, useposix=true, d_sigaction=define
useithreads=define, usemultiplicity=define

useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
use64bitint=define, use64bitall=undef, uselongdouble=undef
usemymalloc=n, bincompat5005=undef
Compiler:

cc='cc', ccflags ='-D_REENTRANT -fno-strict-aliasing -pipe
-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -DPERL_USE_SAFE_PUTENV',
optimize='-g -O0',
cppflags='-D_REENTRANT -fno-strict-aliasing -pipe'
ccversion='', gccversion='3.4.3
(csl-sol210-3_4-branch+sol_rpath)', gccosandvers='solaris2.10'

intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=87654321
d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
ivtype='long long', ivsize=8, nvtype='double', nvsize=8,
Off_t='off_t', lseeksize=8
alignbytes=8, prototype=define
Linker and Libraries:

ld='cc', ldflags =' '
libpth=/usr/lib /usr/ccs/lib
libs=-lsocket -lnsl -ldl -lm -lpthread -lc
perllibs=-lsocket -lnsl -ldl -lm -lpthread -lc
libc=/lib/libc.so, so=so, useshrplib=false, libperl=libperl.a
gnulibc_version=''
Dynamic Linking:
dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags=' '
cccdlflags='-fPIC', lddlflags='-G'


Characteristics of this binary (from libperl):
Compile-time options: HAS_TIMES MULTIPLICITY PERLIO_LAYERS
PERL_DONT_CREATE_GVSV
PERL_HASH_FUNC_ONE_AT_A_TIME_HARD
PERL_IMPLICIT_CONTEXT PERL_MALLOC_WRAP
PERL_PRESERVE_IVUV PERL_SAWAMPERSAND
PERL_USE_SAFE_PUTENV USE_64_BIT_INT USE_ITHREADS
USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE
USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_PERLIO
USE_PERL_ATOF USE_REENTRANT_API
Built under solaris
Compiled at May 20 2013 21:17:14
@INC:
lib
/opt/lib/perl5/site_perl/5.18.0/sun4-solaris-thread-multi-64int
/opt/lib/perl5/site_perl/5.18.0
/opt/lib/perl5/5.18.0/sun4-solaris-thread-multi-64int
/opt/lib/perl5/5.18.0
/opt/lib/perl5/site_perl/5.16.2
/opt/lib/perl5/site_perl/5.8.9
/opt/lib/perl5/site_perl
.

[Nicholas Clark]

On Mon, May 20, 2013 at 08:19:42AM +0100, Nicholas Clark wrote:
> On Mon, May 20, 2013 at 05:11:38AM +0200, Leon Timmermans wrote:
> > On Sun, May 19, 2013 at 9:50 PM, Niko Tyni <perlbug-...@perl.org> wrote:

> > > Bisecting says the first bad commit is
> > >
> > > commit 8be227ab5eaa23f2d21fd15f70190e494496dcbe
> > > Author: Father Chrysostomos <spr...@cpan.org>
> > > Date: Sat Jun 23 09:54:31 2012 -0700
> > >
> > > CV-based slab allocation for ops
> >
> > That makes perfect sense with my theory. Probably that allocator
> > should do some padding to 64-bit boundaries.
>
> I think it needs to be to the alignment of the largest thing in an OP.
> Which on a 32 bit sparc with 64 bit IVs is (currently) 64 bits:
>
>
> struct pmop {
> BASEOP
> OP * op_first;
> OP * op_last;
> #ifdef USE_ITHREADS
> IV op_pmoffset;
> #else
> REGEXP * op_pmregexp; /* compiled expression */
> #endif

I can replicate this on one of the sparc linux machines on the GCC compile
farm. For -Duse64bitint:

sizeof(struct op) = 20 __alignof__(struct op) = 4
sizeof(struct unop) = 24 __alignof__(struct unop) = 4
sizeof(struct binop) = 28 __alignof__(struct binop) = 4
sizeof(struct logop) = 28 __alignof__(struct logop) = 4
sizeof(struct listop) = 28 __alignof__(struct listop) = 4
sizeof(struct pmop) = 56 __alignof__(struct pmop) = 8
sizeof(struct svop) = 24 __alignof__(struct svop) = 4
sizeof(struct padop) = 24 __alignof__(struct padop) = 4
sizeof(struct pvop) = 24 __alignof__(struct pvop) = 4
sizeof(struct loop) = 40 __alignof__(struct loop) = 4
sizeof(struct cop) = 48 __alignof__(struct cop) = 4


I get identical output from a mips machine, but it doesn't BUS ERROR.
So I infer that mips is not as strict on alignment of 64 bit integers.
I don't know if there are architectures other than sparc which are strict
about alignment.


I'm thinking about the least worst way to fix this. I notice that

1) almost nothing on CPAN uses the macros NewOp() and NewOpSz()
2) nothing on CPAN cheats and calls Perl_Slab_Alloc() directly

So I infer that no code out there calls the slab allocator for arbitrary
sized things, and that the only thing requested which needs 8 byte alignment
(on some platforms) will have a size of 56.

So I'm wondering if that's the way to go for maint-5.18

Whilst it's cleaner, I'm wary of the idea of aligning everything to 8
(for all platforms where sizeof(IV) > sizeof(void *)) because it will
penalise ARM and x86 for BASOPs, BINOPs and LOGOPs, which are fairly
common (I believe).

Nicholas Clark

[ольга крыжановская]

All RISC architectures prefer the natural alignment of data types,
i.e. 64bit must be 64bit aligned, 32bit must be 32bit aligned, and so
on.
However, there are extra load and store instructions which are
slower, but do not trigger SIGBUS. gcc on some platforms and
architectures prefers the slow instructions (or byte-by-byte
loads/stores) for better compatibility, but on others it uses the
fast, but more tricky load/store instructions which require natural
alignment.

On Solaris/SPARC the Sun Studio compiler always uses the fast
instructions which always require the natural alignment. gcc on SPARC,
depending on version, uses the fast instructions when it thinks its
safe to use them and falls back to the slow byte-per-byte loads
otherwise.
MIPS has the same requirements, but the gcc compiler creates crap
which is always safe, at the costs of performance.

Olga

--
, _ _ ,
{ \/`o;====- Olga Kryzhanovska -====;o`\/ }
.----'-/`-/ olga.kry...@gmail.com \-`\-'----.
`'-..-| / http://twitter.com/fleyta \ |-..-'`
/\/\ Solaris/BSD//C/C++ programmer /\/\
`--` `--`

[Andy Dougherty]

On Tue, 21 May 2013, Nicholas Clark wrote:

> On Mon, May 20, 2013 at 08:19:42AM +0100, Nicholas Clark wrote:
> > On Mon, May 20, 2013 at 05:11:38AM +0200, Leon Timmermans wrote:
> > > On Sun, May 19, 2013 at 9:50 PM, Niko Tyni <perlbug-...@perl.org> wrote:
>
> > > > Bisecting says the first bad commit is
> > > >
> > > > commit 8be227ab5eaa23f2d21fd15f70190e494496dcbe
> > > > Author: Father Chrysostomos <spr...@cpan.org>
> > > > Date: Sat Jun 23 09:54:31 2012 -0700
> > > >
> > > > CV-based slab allocation for ops
> > >
> > > That makes perfect sense with my theory. Probably that allocator
> > > should do some padding to 64-bit boundaries.
> >
> > I think it needs to be to the alignment of the largest thing in an OP.
> > Which on a 32 bit sparc with 64 bit IVs is (currently) 64 bits:
> >
> >
> > struct pmop {
> > BASEOP
> > OP * op_first;
> > OP * op_last;
> > #ifdef USE_ITHREADS
> > IV op_pmoffset;
> > #else
> > REGEXP * op_pmregexp; /* compiled expression */
> > #endif
>
> I can replicate this on one of the sparc linux machines on the GCC compile
> farm. For -Duse64bitint:

I was able to replicate it on an old Sparc Solaris system. It compiles
successfully with gcc-3.4.3, but fails with gcc-4.1.0 and gcc-4.6.0.

> 1) almost nothing on CPAN uses the macros NewOp() and NewOpSz()
> 2) nothing on CPAN cheats and calls Perl_Slab_Alloc() directly
>
> So I infer that no code out there calls the slab allocator for arbitrary
> sized things, and that the only thing requested which needs 8 byte alignment
> (on some platforms) will have a size of 56.
>
> So I'm wondering if that's the way to go for maint-5.18

Yes, I suspect so.

>
> Whilst it's cleaner, I'm wary of the idea of aligning everything to 8
> (for all platforms where sizeof(IV) > sizeof(void *)) because it will
> penalise ARM and x86 for BASOPs, BINOPs and LOGOPs, which are fairly
> common (I believe).

It could perhaps be protected by #ifdef __sparc__/#endif conditionals.

I may be able to look at this late next week.

--
Andy Dougherty doug...@lafayette.edu

[Nicholas Clark]

On Tue, May 21, 2013 at 05:02:38PM -0400, Andy Dougherty wrote:
> On Tue, 21 May 2013, Nicholas Clark wrote:

> > I can replicate this on one of the sparc linux machines on the GCC compile
> > farm. For -Duse64bitint:
>
> I was able to replicate it on an old Sparc Solaris system. It compiles
> successfully with gcc-3.4.3, but fails with gcc-4.1.0 and gcc-4.6.0.

Interesting. So newer gcc is generating instructions that require alignment,
and older gcc is not. And "old Sparc Solaris" meant that newer Sparc Solaris
didn't (ie newer hardware can do misaligned reads?)

> > 1) almost nothing on CPAN uses the macros NewOp() and NewOpSz()
> > 2) nothing on CPAN cheats and calls Perl_Slab_Alloc() directly
> >
> > So I infer that no code out there calls the slab allocator for arbitrary
> > sized things, and that the only thing requested which needs 8 byte alignment
> > (on some platforms) will have a size of 56.
> >
> > So I'm wondering if that's the way to go for maint-5.18
>
> Yes, I suspect so.
> >
> > Whilst it's cleaner, I'm wary of the idea of aligning everything to 8
> > (for all platforms where sizeof(IV) > sizeof(void *)) because it will
> > penalise ARM and x86 for BASOPs, BINOPs and LOGOPs, which are fairly
> > common (I believe).
>
> It could perhaps be protected by #ifdef __sparc__/#endif conditionals.

I don't think that this is correct, as I've read that mips can enable SIGBUS
on misaligned reads, hence at least one other architecture potentially needs
this.

> I may be able to look at this late next week.

I hope to have something sooner than that.

Nicholas Clark

[Andy Dougherty]

On Wed, 22 May 2013, Nicholas Clark wrote:

> On Tue, May 21, 2013 at 05:02:38PM -0400, Andy Dougherty wrote:
> > On Tue, 21 May 2013, Nicholas Clark wrote:
>
> > > I can replicate this on one of the sparc linux machines on the GCC compile
> > > farm. For -Duse64bitint:
> >
> > I was able to replicate it on an old Sparc Solaris system. It compiles
> > successfully with gcc-3.4.3, but fails with gcc-4.1.0 and gcc-4.6.0.
>
> Interesting. So newer gcc is generating instructions that require alignment,
> and older gcc is not. And "old Sparc Solaris" meant that newer Sparc Solaris
> didn't (ie newer hardware can do misaligned reads?)

Old just means it's all I have handy. (Specifically UltraSparc 10,
Solaris 8, maybe 15 years old). It can do misaligned writes (this bus
error is on a write command, I think) if instructed to do so, but it's a
different assembly instruction. I suspect the differing reports we've
seen are due to the different gcc versions, not different SPARC hardware.

> > I may be able to look at this late next week.
>
> I hope to have something sooner than that.

That would be great. I can test things sooner, but not create them.
(Though testing is slow -- about 1/2 hour from Configure to SIGBUS!).

--
Andy Dougherty doug...@lafayette.edu

[Niko Tyni]

On Wed, May 22, 2013 at 12:33:28PM +0100, Nicholas Clark wrote:
> On Tue, May 21, 2013 at 05:02:38PM -0400, Andy Dougherty wrote:
> > On Tue, 21 May 2013, Nicholas Clark wrote:
>
> > > I can replicate this on one of the sparc linux machines on the GCC compile
> > > farm. For -Duse64bitint:
> >
> > I was able to replicate it on an old Sparc Solaris system. It compiles
> > successfully with gcc-3.4.3, but fails with gcc-4.1.0 and gcc-4.6.0.
>
> Interesting. So newer gcc is generating instructions that require alignment,
> and older gcc is not. And "old Sparc Solaris" meant that newer Sparc Solaris
> didn't (ie newer hardware can do misaligned reads?)

Earlier cases of similar alignment problems on sparc that I've seen
typically only occurred at gcc -O2, not at -O0. I was somewhat surprised
to see this one crash at -O0 too.

> > > Whilst it's cleaner, I'm wary of the idea of aligning everything to 8
> > > (for all platforms where sizeof(IV) > sizeof(void *)) because it will
> > > penalise ARM and x86 for BASOPs, BINOPs and LOGOPs, which are fairly
> > > common (I believe).
> >
> > It could perhaps be protected by #ifdef __sparc__/#endif conditionals.
>
> I don't think that this is correct, as I've read that mips can enable SIGBUS
> on misaligned reads, hence at least one other architecture potentially needs
> this.

FWIW ARM also has (probably different) alignment requirements, but
unaligned access trapping is somewhat controllable on ARM/Linux through
the sysctl interface at /proc/cpu/alignment. AIUI "fixing" an unaligned
access in the kernel causes a severe performance penalty.

http://jsolano.net/2012/09/06/arm-unaligned-data-access-and-floating-point-in-linux/

I don't think there's a similar sysctl for Sparc / Linux (or Solaris
for that matter.)

> > I may be able to look at this late next week.
>
> I hope to have something sooner than that.

Thanks for looking at this. I'm happy to test anything you come up with.
--
Niko Tyni nt...@debian.org

[Nicholas Clark]

On Wed, May 22, 2013 at 10:39:53PM +0300, Niko Tyni wrote:
> On Wed, May 22, 2013 at 12:33:28PM +0100, Nicholas Clark wrote:

> > I hope to have something sooner than that.

I had an insight on the tram. All sorts of complex hacks are complex...

> Thanks for looking at this. I'm happy to test anything you come up with.

Try this. It's against blead. The comment in the diff hunk will conflict
against v5.18.0:

diff --git a/op.c b/op.c
index 96a06b1..341b087 100644
--- a/op.c
+++ b/op.c
@@ -175,6 +175,19 @@ Perl_Slab_Alloc(pTHX_ size_t sz)
|| (CvSTART(PL_compcv) && !CvSLABBED(PL_compcv)))
return PerlMemShared_calloc(1, sz);

+#if defined(USE_ITHREADS) && IVSIZE > U32SIZE
+ /* Work around a goof with alignment on our part. For sparc32 (and
+ possibly other architectures), if built with -Duse64bitint, the IV
+ op_pmoffset in struct pmop should be 8 byte aligned, but the slab
+ allocator is only providing 4 byte alignment. The real fix is to change
+ the IV to a type the same size as a pointer, such as size_t, but we
+ can't do that without breaking the ABI, which is a no-no in a maint
+ release. So instead, simply allocate struct pmop directly, which will be
+ suitably aligned: */
+ if (sz == sizeof(struct pmop))
+ return PerlMemShared_calloc(1, sz);
+#endif
+
/* While the subroutine is under construction, the slabs are accessed via
CvSTART(), to avoid needing to expand PVCV by one pointer for something
unneeded at runtime. Once a subroutine is constructed, the slabs are

Not massively tested, but does get the (sadly soon to be decommisioned)
GCC compiler farm Sparc machine well past miniperl, and still chugging.

Would be nice to test this with & without -Duse64bitint on as many
architectures as practical. (I can get to x86_64, x86, sparc and mips, but
not tonight)

Nicholas Clark

[Andy Dougherty]

I like that. It's clean and clever, but not too clever.

So far, it built for me on Solaris/SPARC with

gcc-4.6.0 -Dusethreads -Duse64bitint
gcc-4.6.0 -Dusethreads
gcc-4.6.0 -Duse64bitint

On the first two, all tests passed except op/threads.t. Rerunning just
that test with a longer timeout after test 9 passed in both cases.

On the third one, it's in the middle of building extensions. This is slow
going -- Each of the first two variations took 9.5 hours (a little more than
7 hours is for the test suite).

--
Andy Dougherty doug...@lafayette.edu

[Niko Tyni]

On Wed, May 22, 2013 at 01:26:03PM -0700, Nicholas Clark via RT wrote:
> On Wed, May 22, 2013 at 10:39:53PM +0300, Niko Tyni wrote:

> I had an insight on the tram. All sorts of complex hacks are complex...
>
> > Thanks for looking at this. I'm happy to test anything you come up with.

> Would be nice to test this with & without -Duse64bitint on as many
> architectures as practical. (I can get to x86_64, x86, sparc and mips, but
> not tonight)

I tested with v5.18.0 and a "backported" version of the patch. All tests
pass with GCC 4.6.3 and -Dusethreads, both with and without -Duse64bitint,
on these Debian platforms:

armhf (ARM hard-float, 32bit)
ia64 (Itanium, 64bit)
mips (32bit, big endian)
mipsel (32bit, little endian)
powerpc (32bit)
s390 (IBM S/390, 31bit)
s390x (IBM S/390 64bit)
sparc (32bit)

I'm attaching 'perl -V' output for all of those in case someone is curious.
(With outputs from the non-64bitint build and the 64bitint one concatenated.)
--
Niko Tyni nt...@debian.org

[Nicholas Clark]

On Fri, May 24, 2013 at 10:30:09AM +0300, Niko Tyni wrote:
> On Wed, May 22, 2013 at 01:26:03PM -0700, Nicholas Clark via RT wrote:
> > On Wed, May 22, 2013 at 10:39:53PM +0300, Niko Tyni wrote:
>
> > I had an insight on the tram. All sorts of complex hacks are complex...
> >
> > > Thanks for looking at this. I'm happy to test anything you come up with.
>
> > Would be nice to test this with & without -Duse64bitint on as many
> > architectures as practical. (I can get to x86_64, x86, sparc and mips, but
> > not tonight)
>
> I tested with v5.18.0 and a "backported" version of the patch. All tests
> pass with GCC 4.6.3 and -Dusethreads, both with and without -Duse64bitint,
> on these Debian platforms:
>
> armhf (ARM hard-float, 32bit)
> ia64 (Itanium, 64bit)
> mips (32bit, big endian)
> mipsel (32bit, little endian)
> powerpc (32bit)
> s390 (IBM S/390, 31bit)
> s390x (IBM S/390 64bit)
> sparc (32bit)

Thanks. It also passed on everything I tried it on. I've pushed it to blead.

Sadly the two available sparc machines at the GCC compile farm are scheduled
to be decommissioned at the end of June, so I probably won't be able to test
on sparc for much longer. However, it does seem that the kernel bug is fixed.
Previously the test suite had been be able to create an unkillable busy
process. So it looks like it is now "safe" to run a blead smoker on 32 bit
sparc Linux.

Nicholas Clark

[Andy Dougherty]

On Fri, 24 May 2013, Nicholas Clark wrote:

> On Fri, May 24, 2013 at 10:30:09AM +0300, Niko Tyni wrote:
> > On Wed, May 22, 2013 at 01:26:03PM -0700, Nicholas Clark via RT wrote:
> > > On Wed, May 22, 2013 at 10:39:53PM +0300, Niko Tyni wrote:
> >
> > > I had an insight on the tram. All sorts of complex hacks are complex...
> > >
> > > > Thanks for looking at this. I'm happy to test anything you come up with.
> >
> > > Would be nice to test this with & without -Duse64bitint on as many
> > > architectures as practical. (I can get to x86_64, x86, sparc and mips, but
> > > not tonight)
> >
> > I tested with v5.18.0 and a "backported" version of the patch. All tests
> > pass with GCC 4.6.3 and -Dusethreads, both with and without -Duse64bitint,
> > on these Debian platforms:
> >
> > armhf (ARM hard-float, 32bit)
> > ia64 (Itanium, 64bit)
> > mips (32bit, big endian)
> > mipsel (32bit, little endian)
> > powerpc (32bit)
> > s390 (IBM S/390, 31bit)
> > s390x (IBM S/390 64bit)
> > sparc (32bit)
>
> Thanks. It also passed on everything I tried it on. I've pushed it to blead.

Thank you. So far it's working fine for me and I anticipate no problems.
It's just taking a long time to actually finish. I'll report back when it
does.

Meanwhile, what would you think about tweaking it like this:

diff --git a/op.c b/op.c
index 792e8d6..fd69c56 100644
--- a/op.c
+++ b/op.c
@@ -175,7 +175,7 @@ Perl_Slab_Alloc(pTHX_ size_t sz)

|| (CvSTART(PL_compcv) && !CvSLABBED(PL_compcv)))
return PerlMemShared_calloc(1, sz);

-#if defined(USE_ITHREADS) && IVSIZE > U32SIZE
+#if defined(USE_ITHREADS) && IVSIZE > U32SIZE && !defined(__x86_64__)

/* Work around a goof with alignment on our part. For sparc32 (and

possibly other architectures), if built with -Duse64bitint, the IV

op_pmoffset in struct pmop should be 8 byte aligned, but the slab

As far as I am aware, the alignment isn't an issue on x86_64, but there is
a small, but barely measureable, cost to the if(sz == sizeof(struct pmop))
test even in cases where it isn't used. Fiddling a bit with perbench
benchmarks, this modified version of one of the hash tests:

my $i = "abcdefg";
for (1..2000) {
for my $j (1..2000) {
$hash{$i} = $j;
}
$i++;
}

my $k;
foreach $k (keys %hash) {
my $v = $hash{$k};
}

ran slightly quicker on x86_64 (differences were consistently at the 2-3
sigma level as reported by dumbbench) with that suggested tweak.

> Sadly the two available sparc machines at the GCC compile farm are scheduled
> to be decommissioned at the end of June, so I probably won't be able to test
> on sparc for much longer. However, it does seem that the kernel bug is fixed.
> Previously the test suite had been be able to create an unkillable busy
> process. So it looks like it is now "safe" to run a blead smoker on 32 bit
> sparc Linux.

That would be nice. I can continue to test on SPARC, but the machine
isn't online most of the time, and is sometimes needed for other tasks, so
I can't really run a smoker on it.

--
Andy Dougherty doug...@lafayette.edu

[Nicholas Clark]

On Sat, May 25, 2013 at 10:57:47AM -0400, Andy Dougherty wrote:

> It's just taking a long time to actually finish. I'll report back when it
> does.
>
> Meanwhile, what would you think about tweaking it like this:
>
> diff --git a/op.c b/op.c
> index 792e8d6..fd69c56 100644
> --- a/op.c
> +++ b/op.c
> @@ -175,7 +175,7 @@ Perl_Slab_Alloc(pTHX_ size_t sz)
> || (CvSTART(PL_compcv) && !CvSLABBED(PL_compcv)))
> return PerlMemShared_calloc(1, sz);
>
> -#if defined(USE_ITHREADS) && IVSIZE > U32SIZE
> +#if defined(USE_ITHREADS) && IVSIZE > U32SIZE && !defined(__x86_64__)
> /* Work around a goof with alignment on our part. For sparc32 (and
> possibly other architectures), if built with -Duse64bitint, the IV
> op_pmoffset in struct pmop should be 8 byte aligned, but the slab

Good point. But I think that that's more restrictive than it needs to be.
I think that this would be valid:

#if defined(USE_ITHREADS) && IVSIZE > U32SIZE && IVSIZE > PTRSIZE

(but I have not yet tested it)

> As far as I am aware, the alignment isn't an issue on x86_64, but there is
> a small, but barely measureable, cost to the if(sz == sizeof(struct pmop))
> test even in cases where it isn't used. Fiddling a bit with perbench
> benchmarks, this modified version of one of the hash tests:

and would avoid that cost on other 64 bit platforms.

> my $i = "abcdefg";
> for (1..2000) {
> for my $j (1..2000) {
> $hash{$i} = $j;
> }
> $i++;
> }
>
> my $k;
> foreach $k (keys %hash) {
> my $v = $hash{$k};
> }
>
> ran slightly quicker on x86_64 (differences were consistently at the 2-3
> sigma level as reported by dumbbench) with that suggested tweak.

That's a test doing work at runtime, which is affected by a compile-time
change? As best I can work out, there are no OPs allocated at runtime by
that code.

Nicholas Clark

[Andrew Dougherty]

On Sat, 25 May 2013, Nicholas Clark wrote:

> On Sat, May 25, 2013 at 10:57:47AM -0400, Andy Dougherty wrote:
>
> > It's just taking a long time to actually finish. I'll report back when it
> > does.
> >
> > Meanwhile, what would you think about tweaking it like this:
> >
> > diff --git a/op.c b/op.c
> > index 792e8d6..fd69c56 100644
> > --- a/op.c
> > +++ b/op.c
> > @@ -175,7 +175,7 @@ Perl_Slab_Alloc(pTHX_ size_t sz)
> > || (CvSTART(PL_compcv) && !CvSLABBED(PL_compcv)))
> > return PerlMemShared_calloc(1, sz);
> >
> > -#if defined(USE_ITHREADS) && IVSIZE > U32SIZE
> > +#if defined(USE_ITHREADS) && IVSIZE > U32SIZE && !defined(__x86_64__)
> > /* Work around a goof with alignment on our part. For sparc32 (and
> > possibly other architectures), if built with -Duse64bitint, the IV
> > op_pmoffset in struct pmop should be 8 byte aligned, but the slab
>
> Good point. But I think that that's more restrictive than it needs to be.
> I think that this would be valid:
>
> #if defined(USE_ITHREADS) && IVSIZE > U32SIZE && IVSIZE > PTRSIZE

Yes, that would catch the relevant SPARC case (-Duse64bitint on an
otherwise 32-bit build).

> (but I have not yet tested it)
>
> > As far as I am aware, the alignment isn't an issue on x86_64, but there is
> > a small, but barely measureable, cost to the if(sz == sizeof(struct pmop))
> > test even in cases where it isn't used. Fiddling a bit with perbench
> > benchmarks, this modified version of one of the hash tests:
>
> and would avoid that cost on other 64 bit platforms.
>
> > my $i = "abcdefg";
> > for (1..2000) {
> > for my $j (1..2000) {
> > $hash{$i} = $j;
> > }
> > $i++;
> > }
> >
> > my $k;
> > foreach $k (keys %hash) {
> > my $v = $hash{$k};
> > }
> >
> > ran slightly quicker on x86_64 (differences were consistently at the 2-3
> > sigma level as reported by dumbbench) with that suggested tweak.
>
> That's a test doing work at runtime, which is affected by a compile-time
> change? As best I can work out, there are no OPs allocated at runtime by
> that code.

Correct. With current blead on Linux x86_64, every op allocation still
tests

if(sz == sizeof(struct pmop)

and even though the test is always false for that program, the cummulative
cost of doing the test is (just barely) measurable.

Anyway, it works as is. I think your suggested change should be an
improvement. I'll go ahead and run some tests, but it looks like it
should be fine and safe to backport to 5.18.0.

--
Andy Dougherty doug...@lafayette.edu

[Craig A. Berry]

On Sat, May 25, 2013 at 10:04 AM, Nicholas Clark <ni...@ccl4.org> wrote:

#if defined(USE_ITHREADS) && IVSIZE > U32SIZE && IVSIZE > PTRSIZE

FWIW, this would be true on VMS Alpha or Itanium configured with -Duse64bitint -Dusethreads.  Pointers are 32-bit by default.  64-bit pointers can be requested at compile time, but as far as I know, no one's ever added support to Perl for that.  Alignment faults are not fatal, but badly degrade performance, especially on Itanium where more of the fault handling is done by the OS in software.

[Nicholas Clark]

Meaning that right now 5.18.0 built on VMS Alpha or Itanium with
-Duse64bitint will have potentially misaligned pmops, and hence slowdown as
the fixup code is being hit?

Nicholas Clark

[Nicholas Clark]

On Sat, May 25, 2013 at 11:26:40AM -0400, Andrew Dougherty wrote:
> On Sat, 25 May 2013, Nicholas Clark wrote:
>
> > On Sat, May 25, 2013 at 10:57:47AM -0400, Andy Dougherty wrote:
> >
> > > It's just taking a long time to actually finish. I'll report back when it
> > > does.
> > >
> > > Meanwhile, what would you think about tweaking it like this:
> > >
> > > diff --git a/op.c b/op.c
> > > index 792e8d6..fd69c56 100644
> > > --- a/op.c
> > > +++ b/op.c
> > > @@ -175,7 +175,7 @@ Perl_Slab_Alloc(pTHX_ size_t sz)
> > > || (CvSTART(PL_compcv) && !CvSLABBED(PL_compcv)))
> > > return PerlMemShared_calloc(1, sz);
> > >
> > > -#if defined(USE_ITHREADS) && IVSIZE > U32SIZE
> > > +#if defined(USE_ITHREADS) && IVSIZE > U32SIZE && !defined(__x86_64__)
> > > /* Work around a goof with alignment on our part. For sparc32 (and
> > > possibly other architectures), if built with -Duse64bitint, the IV
> > > op_pmoffset in struct pmop should be 8 byte aligned, but the slab
> >
> > Good point. But I think that that's more restrictive than it needs to be.
> > I think that this would be valid:
> >
> > #if defined(USE_ITHREADS) && IVSIZE > U32SIZE && IVSIZE > PTRSIZE
>
> Yes, that would catch the relevant SPARC case (-Duse64bitint on an
> otherwise 32-bit build).

I pushed this tweak as smoke-me/nicholas/rt-118055-1

commit f61a27cade956988ec6c5ff53b8aff3492ca5839
Author: Nicholas Clark <ni...@ccl4.org>
Date: Sun May 26 08:50:17 2013 +0200

Improved struct pmop alignment fix - avoid the slow path on 64 bit systems.

Commit c2a50ddb1bed6576 works round an alignment bug in the slab allocator
for 32 bit systems built with 64 bit IVs. However, the C pre-processor logic
meant that the test path was enabled on true 64 bit systems. It's not needed
there, so improve the logic so that it isn't compiled for platforms where
pointers are 64 bit.

diff --git a/op.c b/op.c
index 792e8d6..c5dbb68 100644

--- a/op.c
+++ b/op.c
@@ -175,7 +175,7 @@ Perl_Slab_Alloc(pTHX_ size_t sz)
|| (CvSTART(PL_compcv) && !CvSLABBED(PL_compcv)))
return PerlMemShared_calloc(1, sz);

-#if defined(USE_ITHREADS) && IVSIZE > U32SIZE

+#if defined(USE_ITHREADS) && IVSIZE > U32SIZE && IVSIZE > PTRSIZE

/* Work around a goof with alignment on our part. For sparc32 (and
possibly other architectures), if built with -Duse64bitint, the IV
op_pmoffset in struct pmop should be 8 byte aligned, but the slab

> Correct. With current blead on Linux x86_64, every op allocation still
> tests
>
> if(sz == sizeof(struct pmop)
>
> and even though the test is always false for that program, the cummulative
> cost of doing the test is (just barely) measurable.

With the change as shown, that code is no longer compiled on any platform with
64 bit pointers.

I tested 4 configurations on Sparc, Mips and Arm Linux, and all were (still)
happy.

Nicholas Clark

[Craig A. Berry]

Sorry, I was responding to that one line of code and not the context of the thread as a whole.  32-bit pointers in a struct will get aligned on 32-bit boundaries unless some other alignment is requested.  32-bit alignment for a 32-bit item is considered natural alignment and so does not incur an alignment fault to access it.  

Just out of paranoia I did a build and run of the test suite for the following two commits with -Dusethreads -D64bitint:

blead 2013-05-27.15:45:01 24ee35539e5d73b5154f5b656396a34acc420d51 v5.19.0-268-g24ee355

smoke-me/nicholas/rt-118055-1 2013-05-26.06:50:17 f61a27cade956988ec6c5ff53b8aff3492ca5839 v5.19.0-238-gf61a27c

and the build and test times on an otherwise-idle Itanium system were identical.

[Nicholas Clark]

That's useful, but what I'd hope for, given that the initial version of the
alignment fix was already committed to blead as 24ee35539e5d73b5~92

So it works on VMS Itanium. Which is good. But I'm sort of curious, is it
slower at 24ee35539e5d73b5~93 (or earlier - eg v5.18.0)

Actually, "state of tests for v5.18.0 on VMS" would be useful to know.

Nicholas Clark

[Andy Dougherty]

On Sat, 25 May 2013, Nicholas Clark wrote:

> #if defined(USE_ITHREADS) && IVSIZE > U32SIZE && IVSIZE > PTRSIZE

I know it's already been pushed and tested elsewhere, but I'm happy to
follow up that it also passed on Solaris 8/SPAARC with these combinations:

gcc-4.6.0

gcc-4.6.0 -Dusethreads -Duse64bitint
gcc-4.6.0 -Dusethreads
gcc-4.6.0 -Duse64bitint

gcc-4.1.0 -Dusethreads -Duse64bitint
gcc-4.1.0 -Dusethreads
gcc-4.1.0 -Duse64bitint

gcc-3.4.3 -Dusethreads -Duse64bitint
gcc-3.4.3 -Dusethreads

cc -Dusethreads
cc -Dusethreads -Duse64bitint

Nicely done. I think this (along with the corresponding comment patches,
just to keep things in sync, ought to be pushed to maint-5.18).

--
Andy Dougherty doug...@lafayette.edu

[Craig A. Berry]

Is it not c2a50dd that would be the cut-off point?  

$ git log --oneline -n500 | grep -C2 c2a50dd

cb1974b Update Pod-Perldoc to CPAN version 3.20

5b9c515 Update Pod-Usage to CPAN version 1.62

c2a50dd Ensure that the IV in struct pmop (for ithreads) is aligned properly.

1a72e16 Update to CPAN-Meta means META.* need regenerating

814e893 Update File-Temp to CPAN version 0.2301

Based on that assumption I built blead@1a72e16 and the full build and run was about 3% faster than later.  There have been so many module updates and other goings-on that I doubt that is a number that means much.  Except it doesn't look like we had pathological alignment behavior before your fix.

Actually, "state of tests for v5.18.0 on VMS" would be useful to know.

 

Working on it. 

[Nicholas Clark]

On Wed, May 29, 2013 at 07:41:41AM -0500, Craig A. Berry wrote:
> On Tue, May 28, 2013 at 3:10 AM, Nicholas Clark <ni...@ccl4.org> wrote:

> > That's useful, but what I'd hope for, given that the initial version of the
> > alignment fix was already committed to blead as 24ee35539e5d73b5~92
> >
> > So it works on VMS Itanium. Which is good. But I'm sort of curious, is it
> > slower at 24ee35539e5d73b5~93 (or earlier - eg v5.18.0)
> >
>
> Is it not c2a50dd that would be the cut-off point?
>
> $ git log --oneline -n500 | grep -C2 c2a50dd
> cb1974b Update Pod-Perldoc to CPAN version 3.20
> 5b9c515 Update Pod-Usage to CPAN version 1.62
> c2a50dd Ensure that the IV in struct pmop (for ithreads) is aligned
> properly.
> 1a72e16 Update to CPAN-Meta means META.* need regenerating
> 814e893 Update File-Temp to CPAN version 0.2301

Yes, silly me.

> Based on that assumption I built blead@1a72e16 and the full build and run
> was about 3% faster than later. There have been so many module updates and
> other goings-on that I doubt that is a number that means much. Except it
> doesn't look like we had pathological alignment behavior before your fix.

Thanks for checking this.


Meanwhile, I've just copied 386de64977e726b5 across to the Itanium on the
HP cluster, and it's got to point of running tests. This one is known, right?

t/comp/parser ................................................. FAILED--unexpected output at test 0

Nicholas Clark

[Craig A. Berry]

On Tue, Jun 11, 2013 at 8:56 AM, Nicholas Clark <ni...@ccl4.org> wrote:

Meanwhile, I've just copied 386de64977e726b5 across to the Itanium on the
HP cluster, and it's got to point of running tests. This one is known, right?

t/comp/parser ................................................. FAILED--unexpected output at test 0

I'm seeing that too, but not when I run it by itself.  I do see newish warnings when running with -w, and compile-time warnings would trigger the "unexpected output at test 0" symptom, but I don't think tests normally run under -w, so I'm puzzled.

[Dave Mitchell]

On Mon, May 27, 2013 at 03:20:40PM +0100, Nicholas Clark wrote:
> I pushed this tweak as smoke-me/nicholas/rt-118055-1
>
> commit f61a27cade956988ec6c5ff53b8aff3492ca5839
> Author: Nicholas Clark <ni...@ccl4.org>
> Date: Sun May 26 08:50:17 2013 +0200
>
> Improved struct pmop alignment fix - avoid the slow path on 64 bit systems.
>
> Commit c2a50ddb1bed6576 works round an alignment bug in the slab allocator
> for 32 bit systems built with 64 bit IVs. However, the C pre-processor logic
> meant that the test path was enabled on true 64 bit systems. It's not needed
> there, so improve the logic so that it isn't compiled for platforms where
> pointers are 64 bit.

I've just cherry-picked c2a50ddb1bed6576 into maint-5.18 as
45d94421d77056cb11b2961462ab1ca53d4bdb65.

However, I'm not sure of the status of the smoke-me/nicholas/rt-118055-1
branch, either for blead or maint.

--
The warp engines start playing up a bit, but seem to sort themselves out
after a while without any intervention from boy genius Wesley Crusher.
-- Things That Never Happen in "Star Trek" #17

[Dominic Hargreaves]

On Sat, Jun 15, 2013 at 06:15:36AM -0700, Dave Mitchell via RT wrote:
> On Mon, May 27, 2013 at 03:20:40PM +0100, Nicholas Clark wrote:
> > I pushed this tweak as smoke-me/nicholas/rt-118055-1
> >
> > commit f61a27cade956988ec6c5ff53b8aff3492ca5839
> > Author: Nicholas Clark <ni...@ccl4.org>
> > Date: Sun May 26 08:50:17 2013 +0200
> >
> > Improved struct pmop alignment fix - avoid the slow path on 64 bit systems.

F

> >
> > Commit c2a50ddb1bed6576 works round an alignment bug in the slab allocator
> > for 32 bit systems built with 64 bit IVs. However, the C pre-processor logic
> > meant that the test path was enabled on true 64 bit systems. It's not needed
> > there, so improve the logic so that it isn't compiled for platforms where
> > pointers are 64 bit.
>
> I've just cherry-picked c2a50ddb1bed6576 into maint-5.18 as
> 45d94421d77056cb11b2961462ab1ca53d4bdb65.
>
> However, I'm not sure of the status of the smoke-me/nicholas/rt-118055-1
> branch, either for blead or maint.

We have applied

http://perl5.git.perl.org/perl.git/commit/f61a27cade956988ec6c5ff53b8aff3492ca5839

(from smoke-me/nicholas/rt-118055-1, although for some reason gitweb
isn't showing that branch up at all), based on the positive feedback on
that patch on this ticket. Can it now be merged to blead and maint-5.18?

Dominic.

[Niko Tyni]

On Sat, Jun 15, 2013 at 02:14:37PM +0100, Dave Mitchell wrote:
> On Mon, May 27, 2013 at 03:20:40PM +0100, Nicholas Clark wrote:
> > I pushed this tweak as smoke-me/nicholas/rt-118055-1
> >
> > commit f61a27cade956988ec6c5ff53b8aff3492ca5839
> > Author: Nicholas Clark <ni...@ccl4.org>
> > Date: Sun May 26 08:50:17 2013 +0200
> >
> > Improved struct pmop alignment fix - avoid the slow path on 64 bit systems.
> >
> > Commit c2a50ddb1bed6576 works round an alignment bug in the slab allocator
> > for 32 bit systems built with 64 bit IVs. However, the C pre-processor logic
> > meant that the test path was enabled on true 64 bit systems. It's not needed
> > there, so improve the logic so that it isn't compiled for platforms where
> > pointers are 64 bit.
>
> I've just cherry-picked c2a50ddb1bed6576 into maint-5.18 as
> 45d94421d77056cb11b2961462ab1ca53d4bdb65.

Thanks to both of you!

> However, I'm not sure of the status of the smoke-me/nicholas/rt-118055-1
> branch, either for blead or maint.

FWIW we applied them both for Debian "experimental" 5.18.0 packages,
and most architectures auto-built OK (with -Duse64bitint -Dusethreads).
Still missing a few builds, including sparc, which is why I've been
holding off announcing "works for us."

I'm pretty sure I tested f61a27cade95698 manually on sparc and it was
fine there.

http://buildd.debian.org/status/package.php?p=perl&suite=experimental
http://buildd.debian-ports.org/status/package.php?p=perl&suite=experimental

(the latter is for architectures that are not yet or no longer part of
Debian actual.)
--
Niko Tyni nt...@debian.org

[Dominic Hargreaves]

The build results for sparc are now in and it the build is succeeding
(with all tests) with both patches from this ticket applied:

https://buildd.debian.org/status/fetch.php?pkg=perl&arch=sparc&ver=5.18.0-3&stamp=1371738826

Dominic

[Dave Mitchell]

I've now cherry-picked the improved fix into maint-5.18.
Does this mean this ticket can be closed?

--
I don't want to achieve immortality through my work...
I want to achieve it through not dying.
-- Woody Allen

[Dominic Hargreaves]

Yes, I believe so - thank you!

Dominic.

[Father Chrysostomos via RT]

Does this remaining issue change that?

On Mon May 20 00:20:46 2013, nicholas wrote:
> struct pmop {
> BASEOP
> OP * op_first;
> OP * op_last;
> #ifdef USE_ITHREADS
> IV op_pmoffset;
> #else
> REGEXP * op_pmregexp; /* compiled expression */
> #endif
>
>
> But really, that "IV" should be something else. "STRLEN", I think, or
> even
> just size_t, because it's being used as an array index, and arrays
> can't
> be 64 bits large on a 32 bit system.

We usually use PADOFFSET for array indices, but whatever.

>
> We can't change that for 5.18.x. But as well as the alignment thing,
> we
> should fix it for the future.

Any reason I shouldn’t change the IV in blead and remove the workaround
from Perl_Slab_Alloc?

--

Father Chrysostomos


---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=118055

[Father Chrysostomos via RT]

I have done that in commit 784e50c.

[Nicholas Clark]

On Sat, Jul 06, 2013 at 06:01:52PM -0700, Father Chrysostomos via RT wrote:
> On Mon Jun 24 18:16:21 2013, sprout wrote:

> > On Mon May 20 00:20:46 2013, nicholas wrote:
> > > struct pmop {
> > > BASEOP
> > > OP * op_first;
> > > OP * op_last;
> > > #ifdef USE_ITHREADS
> > > IV op_pmoffset;
> > > #else
> > > REGEXP * op_pmregexp; /* compiled expression */
> > > #endif
> > >
> > >
> > > But really, that "IV" should be something else. "STRLEN", I think, or
> > > even
> > > just size_t, because it's being used as an array index, and arrays
> > > can't
> > > be 64 bits large on a 32 bit system.
> >
> > We usually use PADOFFSET for array indices, but whatever.
> >
> > >
> > > We can't change that for 5.18.x. But as well as the alignment thing,
> > > we
> > > should fix it for the future.
> >
> > Any reason I shouldn't change the IV in blead and remove the workaround
> > from Perl_Slab_Alloc?
>
> I have done that in commit 784e50c.

Thanks. PADOFFSET looks like a better idea than the other types I suggested.

Sorry for the delay in replying. Been away and too much going on. Trying to
get back on top of things.

Nicholas Clark