Skip to content
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

Panic in pure-Perl code with vanilla perl-5.16.0 from perlbrew #12315

Open
p5pRT opened this issue Aug 1, 2012 · 25 comments
Open

Panic in pure-Perl code with vanilla perl-5.16.0 from perlbrew #12315

p5pRT opened this issue Aug 1, 2012 · 25 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 1, 2012

Migrated from rt.perl.org#114372 (status was 'open')

Searchable as RT114372$

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2012

From @shlomif

Created by @shlomif

This Bash script causes a panic with vanilla perl-5.16.0 (installed
using perlbrew - without threads) on Mageia Linux 3/Cauldron on x86-64​:

<<<<
#!/bin/bash
echo -e '[2,36,29,83]\n[30,81,55,15,68,26,71,29,6,46]\n[16,66,42,76,59]\n[65,47,65,47,74,33,41,38,75,59]\n[64,55,39,26,33,66]\n' |
  perl -pe '@​h{@​x=sort{$a<=>$b}/\d+/g}=reverse@​x;s#\d+#$h{$&}#g'

The code does not make a lot of sense, but it was a starting point for working
on this Perl Golf challenge​:

http​://golf.shinh.org/p.rb?MaxMinSwap

When running it I get​:

shlomif@​telaviv1​:~$ bash reproduce.bash
[,,,]
[,,,26,,15,,6,29,]
[,,,,]
[,33,,33,,47,41,47,,]
panic​: attempt to copy freed scalar 81b2f8 at -e line 1, <> line 5.

Further lines are not processed.

Please look into fixing it.

Perl Info

Flags:
    category=core
    severity=high

Site configuration information for perl 5.16.0:

Configured by shlomif at Wed Aug  1 11:45:44 IDT 2012.

Summary of my perl5 (revision 5 version 16 subversion 0) configuration:
   
  Platform:
    osname=linux, osvers=3.5.0-desktop-1.mga3, archname=x86_64-linux
    uname='linux telaviv1.shlomifish.org 3.5.0-desktop-1.mga3 #1 smp sat jul 28 00:29:28 utc 2012 x86_64 x86_64 x86_64 gnulinux '
    config_args='-de -Dprefix=/home/shlomif/apps/perl/perlbrew/perls/perl-5.16.0 -Aeval:scriptdir=/home/shlomif/apps/perl/perlbrew/perls/perl-5.16.0/bin'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, 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='-O2',
    cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.7.1', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='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/../lib64 /usr/lib/../lib64 /lib /usr/lib /lib64 /usr/lib64 /usr/local/lib64
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=/lib/libc-2.16.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.16'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

Locally applied patches:
    


@INC for perl 5.16.0:
    /home/shlomif/apps/perl/modules/lib/perl5/site_perl/5.12.2
    /home/shlomif/apps/perl/modules/lib/perl5/site_perl/5.14.2
    /home/shlomif/apps/perl/modules/lib/perl5/site_perl/5.16.0
    /home/shlomif/apps/perl/modules/lib/site_perl/5.12.2
    /home/shlomif/apps/perl/modules/lib/site_perl/5.14.2
    /home/shlomif/apps/perl/modules/lib/site_perl/5.16.0
    /home/shlomif/apps/perl/modules/lib/perl5/5.12.2
    /home/shlomif/apps/perl/modules/lib/perl5/5.14.2
    /home/shlomif/apps/perl/modules/lib/perl5/5.16.0
    /home/shlomif/apps/perl/perlbrew/perls/perl-5.16.0/lib/site_perl/5.16.0/x86_64-linux
    /home/shlomif/apps/perl/perlbrew/perls/perl-5.16.0/lib/site_perl/5.16.0
    /home/shlomif/apps/perl/perlbrew/perls/perl-5.16.0/lib/5.16.0/x86_64-linux
    /home/shlomif/apps/perl/perlbrew/perls/perl-5.16.0/lib/5.16.0
    .


Environment for perl 5.16.0:
    HOME=/home/shlomif
    LANG=en_GB.UTF-8
    LANGUAGE=en_GB:en
    LC_ADDRESS=en_US.UTF-8
    LC_COLLATE=en_US.UTF-8
    LC_CTYPE=en_US.UTF-8
    LC_IDENTIFICATION=en_GB.UTF-8
    LC_MEASUREMENT=en_GB.UTF-8
    LC_MESSAGES=en_US.UTF-8
    LC_MONETARY=en_US.UTF-8
    LC_NAME=en_GB.UTF-8
    LC_NUMERIC=en_GB.UTF-8
    LC_PAPER=en_US.UTF-8
    LC_SOURCED=1
    LC_TELEPHONE=en_US.UTF-8
    LC_TIME=en_GB.UTF-8
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/shlomif/apps/perl/perlbrew/bin:/home/shlomif/apps/perl/perlbrew/perls/perl-5.16.0/bin:/home/shlomif/apps/apache-maven/apache-maven-2.1.0//bin:/home/shlomif/Download/unpack/graphics/fop/fop-0.93:/home/shlomif/apps/perl/modules/local/bin:/home/shlomif/apps/perl/modules/bin:/home/shlomif/apps/latemp/bin:/home/shlomif/apps/test/quadpres/bin:/home/shlomif/bin:/home/shlomif/apps/apache-maven/apache-maven-2.1.0//bin:/bin:/usr/bin:/usr/games:/usr/lib64/qt4/bin
    PERL5LIB=/home/shlomif/apps/perl/modules/lib/perl5/site_perl/5.12.2:/home/shlomif/apps/perl/modules/lib/perl5/site_perl/5.14.2:/home/shlomif/apps/perl/modules/lib/perl5/site_perl/5.16.0:/home/shlomif/apps/perl/modules/lib/site_perl/5.12.2:/home/shlomif/apps/perl/modules/lib/site_perl/5.14.2:/home/shlomif/apps/perl/modules/lib/site_perl/5.16.0:/home/shlomif/apps/perl/modules/lib/perl5/5.12.2:/home/shlomif/apps/perl/modules/lib/perl5/5.14.2:/home/shlomif/apps/perl/modules/lib/perl5/5.16.0
    PERLBREW_MANPATH=/home/shlomif/apps/perl/perlbrew/perls/perl-5.16.0/man
    PERLBREW_PATH=/home/shlomif/apps/perl/perlbrew/bin:/home/shlomif/apps/perl/perlbrew/perls/perl-5.16.0/bin
    PERLBREW_PERL=perl-5.16.0
    PERLBREW_ROOT=/home/shlomif/apps/perl/perlbrew
    PERLBREW_VERSION=0.44
    PERL_AUTOINSTALL=--skipdeps --alldeps
    PERL_BADLANG (unset)
    PERL_MM_USE_DEFAULT=1
    SHELL=/bin/bash


-- 
-----------------------------------------------------------------
Shlomi Fish       http://www.shlomifish.org/
Understand what Open Source is - http://shlom.in/oss-fs

There was one Napoleon, one George Washington, and one me!
    — Big Boy Caprice in
       http://en.wikiquote.org/wiki/Dick_Tracy_%281990_film%29

Please reply to list if it's a mailing list post - http://shlom.in/reply .

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2012

From @nwc10

On Wed Aug 01 12​:00​:05 2012, shlomif@​shlomifish.org wrote​:

shlomif@​telaviv1​:~$ bash reproduce.bash
[,,,]
[,,,26,,15,,6,29,]
[,,,,]
[,33,,33,,47,41,47,,]
panic​: attempt to copy freed scalar 81b2f8 at -e line 1, <> line 5.

Further lines are not processed.

Interesting test case. It's not a new bug​:

$ echo -e
'[2,36,29,83]\n[30,81,55,15,68,26,71,29,6,46]\n[16,66,42,76,59]\n[65,47,65,47,74,33,41,38,75,59]\n[64,55,39,26,33,66]\n'
| perl5.8.9 -pe '@​h{@​x=sort{$a<=>$b}/\d+/g}=reverse@​x;s#\d+#$h{$&}#g'
[,,,]
[,,,26,,15,,6,29,]
[,,,,]
[,33,,33,,47,41,47,,]
panic​: attempt to copy freed scalar 1008195d0 to 1008195c0 at -e line 1,
<> line 5.

(earlier versions seem to SEGV instead of panicing)

I suspect that the old contents of @​x are getting freed whilst they
are on the stack as a result of the reverse @​x, causing the panic.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2012

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Aug 1, 2012

From dennis.kaarsemaker@booking.com

On wo, 2012-08-01 at 12​:09 -0700, Nicholas Clark via RT wrote​:

On Wed Aug 01 12​:00​:05 2012, shlomif@​shlomifish.org wrote​:

shlomif@​telaviv1​:~$ bash reproduce.bash
[,,,]
[,,,26,,15,,6,29,]
[,,,,]
[,33,,33,,47,41,47,,]
panic​: attempt to copy freed scalar 81b2f8 at -e line 1, <> line 5.

Further lines are not processed.

Interesting test case. It's not a new bug​:

$ echo -e
'[2,36,29,83]\n[30,81,55,15,68,26,71,29,6,46]\n[16,66,42,76,59]\n[65,47,65,47,74,33,41,38,75,59]\n[64,55,39,26,33,66]\n'
| perl5.8.9 -pe '@​h{@​x=sort{$a<=>$b}/\d+/g}=reverse@​x;s#\d+#$h{$&}#g'
[,,,]
[,,,26,,15,,6,29,]
[,,,,]
[,33,,33,,47,41,47,,]
panic​: attempt to copy freed scalar 1008195d0 to 1008195c0 at -e line 1,
<> line 5.

(earlier versions seem to SEGV instead of panicing)

I suspect that the old contents of @​x are getting freed whilst they
are on the stack as a result of the reverse @​x, causing the panic.

Actually, the reverse seems unrelated (or I've uncovered another bug). I
managed to reduce the testcase to​:

./perl -e '@​x = (1,2); @​h{@​x=sort}=@​x'

--
Dennis Kaarsemaker, Systems Architect
Booking.com
Herengracht 597, 1017 CE Amsterdam
Tel external +31 (0) 20 715 3409
Tel internal (7207) 3409

@p5pRT
Copy link
Author

p5pRT commented Aug 2, 2012

From @iabyn

On Wed, Aug 01, 2012 at 09​:57​:06PM +0200, Dennis Kaarsemaker wrote​:

Actually, the reverse seems unrelated (or I've uncovered another bug). I
managed to reduce the testcase to​:

./perl -e '@​x = (1,2); @​h{@​x=sort}=@​x'

The sort's unrelated too. It can be reduced further to​:

  @​x = (1,2); @​h{@​x=()}=@​x;

Looks like a classic stack-not-refcounted bug. The elements of @​x are
pushed onto the stack, ready for the main assignment; then they are freed
by the @​x=(), leaving freed elements on the stack.

--
"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"

@p5pRT
Copy link
Author

p5pRT commented Sep 4, 2012

From @cpansprout

On Wed Aug 01 12​:00​:05 2012, shlomif@​shlomifish.org wrote​:

Please look into fixing it.

I plan to eventually, but it will require rewriting thousands of lines
of code.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 5, 2012

From @shlomif

On Mon, 03 Sep 2012 21​:49​:34 -0700
"Father Chrysostomos via RT" <perlbug-followup@​perl.org> wrote​:

On Wed Aug 01 12​:00​:05 2012, shlomif@​shlomifish.org wrote​:

Please look into fixing it.

I plan to eventually, but it will require rewriting thousands of lines
of code.

well, thanks for your willingness, and it sucks that it will require so much
work. That put aside, I wonder why the fix is so non-trivial and will require
such extensive changes, given that Perl has such large test coverage.

Regards,

  Shlomi Fish

--


Shlomi Fish http​://www.shlomifish.org/
Why I Love Perl - http​://shlom.in/joy-of-perl

And the top story for today​: wives live longer than husbands because they are
not married to women.
  — Colin Mochrie in Whose Line is it, Anyway?

Please reply to list if it's a mailing list post - http​://shlom.in/reply .

@p5pRT
Copy link
Author

p5pRT commented Sep 5, 2012

From @cpansprout

On Wed Sep 05 05​:13​:40 2012, shlomif@​shlomifish.org wrote​:

On Mon, 03 Sep 2012 21​:49​:34 -0700
"Father Chrysostomos via RT" <perlbug-followup@​perl.org> wrote​:

On Wed Aug 01 12​:00​:05 2012, shlomif@​shlomifish.org wrote​:

Please look into fixing it.

I plan to eventually, but it will require rewriting thousands of
lines
of code.

well, thanks for your willingness, and it sucks that it will require
so much
work. That put aside, I wonder why the fix is so non-trivial and will
require
such extensive changes, given that Perl has such large test coverage.

The stack, used to pass arguments between operators, does not hold a
reference count on its items. That’s why this fails​:

@​x = (1,2); @​{ @​x = (); \@​y } = @​x;

The RHS of = is evaluated first, so the second statement puts the
elements of @​x on to the stack, then the LHS is evaluated, which pushes
@​y on to the stack, while freeing elements already on it. Then the
freed elements on the RHS are assigned to @​y, but sv_setsv_flags (the
internal function for copying scalars) notices they are freed and dies
with a panic.

Simply making the stack ref-counted would break XS modules. So we would
have to provide two stacks, and switch over to the ‘compatibility’ stack
whenever calling XSUBs. The same may have to apply to custom ops.

All Perl operators take care of stack manipulation themselves. So they
would all need to be changed. Presumably we would have to create a new
set of macros to manipulate the ‘new stack’. Then all uses of the old
macros will have to switch over to the new--not a trivial task, as the
new macros would be used differently. Everything would have to be
audited carefully to avoid memory leaks or double frees.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 5, 2012

From @nwc10

On Wed Sep 05 09​:01​:14 2012, sprout wrote​:

Simply making the stack ref-counted would break XS modules. So we would
have to provide two stacks, and switch over to the ‘compatibility’ stack
whenever calling XSUBs. The same may have to apply to custom ops.

At least for XSUBs that weren't flagged as expecting the "new stack"
semantics. Which would be all of them initially.

(Yes, I can't see a simpler solution to this problem than the one you
propose)

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Sep 5, 2012

From @tsee

On 09/05/2012 06​:01 PM, Father Chrysostomos via RT wrote​:

Simply making the stack ref-counted would break XS modules. So we would
have to provide two stacks, and switch over to the ‘compatibility’ stack
whenever calling XSUBs. The same may have to apply to custom ops.

And making the stack refcounted would be a major, major slow-down. Let's
not forget that.

--Steffen

@p5pRT
Copy link
Author

p5pRT commented Sep 5, 2012

From @nwc10

On Wed, Sep 05, 2012 at 07​:18​:54PM +0200, Steffen Mueller wrote​:

And making the stack refcounted would be a major, major slow-down. Let's
not forget that.

I wouldn't assume that without actually testing it. Certainly not
"major major". There is currently a lot of to-and-fro with the mortals stack
to ensure that SVs on the (main) stack are referenced somewhere.

All that would be abolished with a properly reference counted stack. So it's
not purely a speed loss.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Sep 5, 2012

From @Leont

On Wed, Sep 5, 2012 at 10​:38 PM, Nicholas Clark <nick@​ccl4.org> wrote​:

I wouldn't assume that without actually testing it. Certainly not
"major major". There is currently a lot of to-and-fro with the mortals stack
to ensure that SVs on the (main) stack are referenced somewhere.

All that would be abolished with a properly reference counted stack. So it's
not purely a speed loss.

Data point​: python has a refcounted stack, and that doesn't seem to
have mortally wounded its performance.

Leon

@p5pRT
Copy link
Author

p5pRT commented Sep 6, 2012

From @Leont

On Wed, Sep 5, 2012 at 6​:01 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

Simply making the stack ref-counted would break XS modules. So we would
have to provide two stacks, and switch over to the ‘compatibility’ stack
whenever calling XSUBs. The same may have to apply to custom ops.

All Perl operators take care of stack manipulation themselves. So they
would all need to be changed. Presumably we would have to create a new
set of macros to manipulate the ‘new stack’. Then all uses of the old
macros will have to switch over to the new--not a trivial task, as the
new macros would be used differently. Everything would have to be
audited carefully to avoid memory leaks or double frees.

I suspect we can provide a compatibility API that DWIMs on both
systems for most common usage patterns. It would still be a lot of
work, but it could easy such a transition.

Leon

@p5pRT
Copy link
Author

p5pRT commented Sep 8, 2012

From @iabyn

On Thu, Sep 06, 2012 at 03​:09​:44PM +0200, Leon Timmermans wrote​:

On Wed, Sep 5, 2012 at 6​:01 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

Simply making the stack ref-counted would break XS modules. So we would
have to provide two stacks, and switch over to the ‘compatibility’ stack
whenever calling XSUBs. The same may have to apply to custom ops.

All Perl operators take care of stack manipulation themselves. So they
would all need to be changed. Presumably we would have to create a new
set of macros to manipulate the ‘new stack’. Then all uses of the old
macros will have to switch over to the new--not a trivial task, as the
new macros would be used differently. Everything would have to be
audited carefully to avoid memory leaks or double frees.

I suspect we can provide a compatibility API that DWIMs on both
systems for most common usage patterns. It would still be a lot of
work, but it could easy such a transition.

If we have a refcounted stack, allowing existing XS subs to operate
is fairly trivial. If an XS sub isn't marked as being 'refcounted stack
aware', we simply memcopy the existing stack frame to make a new frame
that contains the same SV pointers, but not refcount-imcremented.
After the XSUB returns, we decrement the refcounts of the all the
SVs in the original stack frame, then shuffle down the returned list,
obliterating the original stack frame.

So the only extra cost to support an old-style XSUB over a new-style one
is two block copies​: one an array of pointers equal to the size of the arg
list, and one equal to the size of the return list.

--
But Pity stayed his hand. "It's a pity I've run out of bullets",
he thought. -- "Bored of the Rings"

@p5pRT
Copy link
Author

p5pRT commented Jul 16, 2016

From @cpansprout

I have been thinking more about this lately.

On Sat Sep 08 05​:01​:55 2012, davem wrote​:

On Thu, Sep 06, 2012 at 03​:09​:44PM +0200, Leon Timmermans wrote​:

I suspect we can provide a compatibility API that DWIMs on both
systems for most common usage patterns. It would still be a lot of
work, but it could easy such a transition.

E.g., a POPs that mortalizes the value when removing it from the stack, instead of freeing it. It would still behave as expected, though it would be less efficient.

But having it work only for the ‘most common usage patterns’ is not acceptable, considering that all these macros have long been part of the public API.

If we have a refcounted stack, allowing existing XS subs to operate
is fairly trivial. If an XS sub isn't marked as being 'refcounted stack
aware', we simply memcopy the existing stack frame to make a new frame
that contains the same SV pointers, but not refcount-imcremented.
After the XSUB returns, we decrement the refcounts of the all the
SVs in the original stack frame, then shuffle down the returned list,
obliterating the original stack frame.

Yes, it can be made to work trivially for XSUBs. But what about custom pp functions? There seem to be more and more modules these days that use them, so we do need some way to have things work under both systems.

What is particularly difficult is that the same .xs file may have pp functions and XSUBs. If POPs in an XSUB is old-style, it has to be defined as (*sp--). pp functions have to use whichever stack perl itself uses, so that would be sv_2mortal(*sp--) on a newer perl.

How can we have POPs defined both ways in the same file?

Alternatively, if we use new macro names, then the pp function would have to use rPOPs on newer perls and POPs on older perls, which is a major API breakage.

Another thought I had is that a final pass when compiling a sub could wrap custom pp functions with a wrapper that copies things to a new stack and copies them back, just as we would do for XSUBs. I don’t like it, but it may be the only way. Though I still don’t think it would work for custom pp functions that wrap perl’s own pp functions.

I’m stuck.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 16, 2016

From zefram@fysh.org

Father Chrysostomos via RT wrote​:

Yes, it can be made to work trivially for XSUBs. But what about custom
pp functions? There seem to be more and more modules these days that
use them, so we do need some way to have things work under both systems.

There aren't too many at the moment. If an incompatible change is
needed, I'm willing to update all of mine for the new way. We can hold
the authors of pp functions to higher standards than general XS authors.
But if the pp interface changes incompatibly, we should change the name
of the op_ppaddr struct member, to catch the modules that haven't updated.
(Same deal as with op_sibling.)

How can we have POPs defined both ways in the same file?

Wrong question. pp functions require more update than just different
macro definitions, because they may access the stack directly or do
other things that rely on actually knowing the refcounting scheme.
Given the need for more substantial revision, the new stack interface
could perfectly well use new macro names.

Though if you're concerned about mixing pp styles in one file, the
same issue arises with just xsubs. It doesn't seem a good idea for the
"I know about the refcounted stack" flag to be per module; that breaks
moving an xsub definition between XS files. It should be a per-xsub
flag in the source.

Alternatively, if we use new macro names, then the pp function would
have to use rPOPs on newer perls and POPs on older perls, which is a
major API breakage.

The simpler pp functions and xsub bodies, which work equivalently on
either pp style, can just do

  #ifdef rPOPs
  # define myPOPs rPOPs
  #else
  # define myPOPs POPs
  #endif

and xsub source can likewise have a flag state saying that the code
works either way.

Another thought I had is that a final pass when compiling a sub could
wrap custom pp functions with a wrapper that copies things to a new stack
and copies them back, just as we would do for XSUBs.

No, you can't do that, at least not the way you would for an xsub. The
portion of the stack that an xsub is liable to operate on is objectively
delimited by the top mark at the moment it is called, so you'd only need
to copy that part of the stack back and forth, and it must pop exactly
one mark. A pp function has no such restriction​: it might not pop a
mark, might even push one, might use arguments beyond a mark, and so on.
So to provide compatibility to an old pp functions you'd need to copy
the whole stack back and forth, which sounds much worse.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Jul 16, 2016

From @cpansprout

On Fri Jul 15 20​:54​:22 2016, zefram@​fysh.org wrote​:

Father Chrysostomos via RT wrote​:

Yes, it can be made to work trivially for XSUBs. But what about custom
pp functions? There seem to be more and more modules these days that
use them, so we do need some way to have things work under both systems.

There aren't too many at the moment. If an incompatible change is
needed, I'm willing to update all of mine for the new way. We can hold
the authors of pp functions to higher standards than general XS authors.
But if the pp interface changes incompatibly, we should change the name
of the op_ppaddr struct member, to catch the modules that haven't updated.
(Same deal as with op_sibling.)

Thank you for your response.

For op_sibling, we provided a transitional period in which defining PERL_OP_PARENT would flag which modules needed updated, without them breaking just yet.

A simple define will not be practical, because perl’s own pp functions would be littered with #ifdefs.

The only way I can think of providing a transitional phase is to

#define op_ppaddr op_rppaddr /* or whatever we call it */

and provide a define that disables that. Affected modules will leak memory, but it may not be the end of the world. We will have a define to catch them.

dSP would have to be redefined to use the refcounted stack. eupxs uses something other than dSP for the old stack.

It’s ugly. It probably breaks some XS modules that do dSP on their own.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 16, 2016

From zefram@fysh.org

Father Chrysostomos via RT wrote​:

How can we have POPs defined both ways in the same file?

Like this...

Core headers define macros to operate both ways based on a switch​:

  #define POPs (PERL_STACK_LOCALLY_REFCOUNTED ? \
  sv_2mortal(*sp--) : (*sp--))
  #define PUSHs(s) (PERL_STACK_LOCALLY_REFCOUNTED ? \
  (*++sp = SvREFCNT_inc(s)) : (*++sp = (s)))
  #define TOPs (*sp) /* same either way */

Core headers also define a macro saying how the stack really behaves in
the core​:

  #define PERL_STACK_GLOBALLY_REFCOUNTED 1 /* or 0 */

Code that needs to be portable to older Perl versions then has a reserve
definition​:

  #ifndef PERL_STACK_GLOBALLY_REFCOUNTED
  # define PERL_STACK_GLOBALLY_REFCOUNTED 0
  #endif

There's then some dance around redefining PERL_STACK_LOCALLY_REFCOUNTED.
I'm not sure whether the `normal' state for it should be, but it's
clear how it needs to be defined for the compilation of any pp code.
For pp code that must operate on the native stack, including the whole
core and probably all custom op pp functions, we define​:

  #define PERL_STACK_LOCALLY_REFCOUNTED PERL_STACK_GLOBALLY_REFCOUNTED

When compiling an XS definition of an xsub, how to define it depends
on declarations made in the source. By default, the xsub code is only
expected to be correct for an uncounted stack, so we define​:

  #define PERL_STACK_LOCALLY_REFCOUNTED 0

and if the core stack is refcounted then we also invoke the magic to
copy the stack back and forth between the two conventions. If the xsub
code declares that it can operate with either stack type, then we have
the switch macro defined as for a native pp function, and do not apply
any copying magic.

On Perl versions predating this mechanism, with the above reserve
definition of PERL_STACK_GLOBALLY_REFCOUNTED in place, both of the above
settings of PERL_STACK_LOCALLY_REFCOUNTED are equivalent, which is just
as well because the POPs et al macros don't pay attention to it and
always act as if it is set that way.

Theoretically we could also have xsub code that will only operate with
a refcounted stack. To do this we define the switch macro to 1, and
if the core stack is not refcounted then we invoke the inverse stack
copying magic. If this is to be portable to core versions predating
the switch, the XS framework would have to redefine POPs et al in the
switch-sensitive way, and the stack copying magic would have to come
from the XS framework rather than a core feature.

With the switch in place, the usual way to write pp functions would be
to cope with both styles, not to write only for a refcounted stack.
Most pp functions wouldn't need any edit from their current form,
because POPs et al are conceptually equivalent across both styles.
Code that performs custom stack work, such as bulk copying between an
AV and the stack, would sometimes need conditional code, guarded by
"if(PERL_STACK_LOCALLY_REFCOUNTED)" or equivalent. (The switch macro
can also be used in a #if, but that's more cumbersome than a true C
conditional.)

-zefram

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2016

From perl5-porters@perl.org

Zefram wrote​:

Father Chrysostomos via RT wrote​:

E.g., a POPs that mortalizes the value when removing it from the stack,
instead of freeing it. It would still behave as expected, though it
would be less efficient.

If you're going to mortalise everything that ever goes on the stack,
there's no need to have the reference be non-mortal in the phase when
it's actually on the stack. You may as well mortalise the reference when
pushing it onto the stack, immediately after incrementing the refcount.

POPs would be defined as sv_2mortal(...), but it would be a backward-compatibility macro. I would not expect much (if any) code in core
to use it.

I only brought up POPs because it was the easiest macro I could rede-
fine off the top of my head. :-)

This only needs to be done when there isn't already a reference that
will definitely live long enough. It doesn't actually change the
stack protocol, because the stack doesn't own any references, and
so there's no compatibility problem. I previously proposed this
in <mid​:20151109211329.GH13455@​fysh.org>, which you can find at
<http​://www.nntp.perl.org/group/perl.perl5.porters/2015/11/msg232508.html>.

Dave immediately objected to my proposal the first time round, on the
grounds that the mortals stack significantly degrades performance.
Presumably he'll have the same objection to the plan of having the
stack own refs which then get mortalised when removed from the stack,
because this makes even heavier use of mortalisation than my proposal.

It makes sense to avoid the mortals stack, because by using it we are
pushing SVs on to *two* stacks at the same time, (almost) doubling the
amount of stack-fiddling.

Any stack refcounting scheme that is to be superior to my proposal will
have to avoid Dave's objection by generally not mortalising things coming
off the stack, thus achieving superior performance to all schemes that
mortalise a lot. However, this would mean some serious rewriting of
pp functions, losing source compatibility in even the simplest cases.

I think it is acceptable to have compatibility macros that use mortal-
isation, to preserve source compatibility, but switch things over to a
new set of macros wherever possible.

Code popping anything off the stack would own the counted ref that the
stack had, and can't decrement the refcount until it's finished with
that operand. A delayed decrement of refcount would be quite intrusive,
and it means a lot of tracking of references. But most pp functions also
have the possibility of a non-local exit, during which the reference
has to be cleaned up.

Good point.

Mortalisation would be the obvious solution,
but that's what we're trying to avoid. The solution would be to leave
the operands on the stack (the stack owning the counted ref) during an
operation, only popping them off (and immediately decrementing refcount)
when they're finished with.

That was one of the ideas I had in mind.

Quite a change in stack discipline.

Which is why this bug, known about for decades, has gone unfixed. :-(

I wonder how the performance of decrementing refcounts every time the
stack is popped compares to the mortals stack. There's some rather
obvious similarity between the two, in terms of the kinds of operations
being performed at the low level. Dave?

As I said above, I think it is just a matter of having one stack hold-
ing the SVs, instead of two.

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2016

From zefram@fysh.org

Father Chrysostomos wrote​:

POPs would be defined as sv_2mortal(...), but it would be a
backward-compatibility macro. I would not expect much (if any) code
in core to use it.

Any really extensive change in stack usage like this is bound to introduce
more bugs, quite apart from being really difficult to perform at all.
This isn't sounding like a winning approach.

If we were to introduce more mortal refs now, that wouldn't preclude
a later change to stack refcounting. We could immediately fix the
refcounting bugs at some performance cost, and then the potential
stack refcounting change would be a performance improvement, rather
than a necessary bug fix. Given our failure so far to organise such
a wide-ranging change, I'd be happier if it weren't so essential and
weren't preventing us fixing the actual bugs. How would you feel about
this scenario?

If we go with mortalisation, we can make some performance improvements
to the process of mortalisation. Calling sv_2mortal() for each element
of a bulk array would be inefficient, but a batch version of the function
(void sv_batch_2mortal(SV**, size_t)) could do one set of stack handling
for the whole lot, with a bulk memcpy() to put all the scalars on the
mortals stack in one go. There may also be some gain to be made by
having a single function that performs sv_2mortal(SvREFCNT_inc()),
merging the logic of the two operations.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2016

From @cpansprout

On Sun Jul 17 11​:19​:22 2016, zefram@​fysh.org wrote​:

Father Chrysostomos wrote​:

POPs would be defined as sv_2mortal(...), but it would be a
backward-compatibility macro. I would not expect much (if any) code
in core to use it.

Any really extensive change in stack usage like this is bound to introduce
more bugs, quite apart from being really difficult to perform at all.
This isn't sounding like a winning approach.

That seems to imply ‘Don’t do it’.

If we were to introduce more mortal refs now, that wouldn't preclude
a later change to stack refcounting. We could immediately fix the
refcounting bugs at some performance cost, and then the potential
stack refcounting change would be a performance improvement,

Which seems to imply ‘Do do it’. :-)

So you are saying don’t do it now, do it later. But that does not avoid the fact that it is bound to introduce more bugs.

Surely I am misunderstanding you.

rather
than a necessary bug fix. Given our failure so far to organise such
a wide-ranging change, I'd be happier if it weren't so essential and
weren't preventing us fixing the actual bugs. How would you feel about
this scenario?

I am not sure I understand your argument about our failure to organise it. Procrastination is not necessarily the lack of organisation. It merely indicates that nobody competent enough cared enough actually to do anything about it.

If we could fix the bugs easily and the optimise, that would be nice. But it may be entail a lot of work twice over. See below.

If we go with mortalisation, we can make some performance improvements
to the process of mortalisation. Calling sv_2mortal() for each element
of a bulk array would be inefficient, but a batch version of the function
(void sv_batch_2mortal(SV**, size_t)) could do one set of stack handling
for the whole lot, with a bulk memcpy() to put all the scalars on the
mortals stack in one go. There may also be some gain to be made by
having a single function that performs sv_2mortal(SvREFCNT_inc()),
merging the logic of the two operations.

We would also need to do it for rv2sv, rv2gv, the refaliasing ops (or padsv), and maybe a whole list of other ops. A careful review of the various cases of failure reported over the years may help.

We currently use the mortals stack in many places where it is not needed most of the time, but we do it for the sake of edge cases. It may be worth making the stack track reference counts just for that reason. Benchmarking will tell.

My arguments above may sound as though I want to make the stack keep reference counts come hell or high water, but I am actually ambivalent.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2016

From zefram@fysh.org

Father Chrysostomos via RT wrote​:

That seems to imply "Don't do it".

It's not really a normative statement. It's more an assessment of
where we are​: it looks like we're not actually going to manage to do it
acceptably any time soon.

So you are saying don't do it now, do it later. But that does not
avoid the fact that it is bound to introduce more bugs.

True, but my plan would change the context in which we do it later.
Actually, the first effect, by fixing the real bugs, would be to remove
most of the impetus for a refcounting change, thus making us less likely
to ultimately do it. The tradeoff it offered would be quite different.
But with the pressure of bugfixing off, we'd be less likely to do it
in a slightly buggy way. We'd be free to spend as long as necessary
working out the best transitional plan and reviewing all the code for
correctness. So, if we do eventually do it, this would increase the
chance of us doing it *correctly*.

We would also need to do it for rv2sv, rv2gv, the refaliasing ops (or
padsv), and maybe a whole list of other ops. A careful review of the
various cases of failure reported over the years may help.

Yes, an awful lot of ops would need this added. But if we miss some,
that's just an incomplete bugfix, it doesn't make anything worse.
The bugs in RT are a decent place to start​: for each we can work out
what op type put the things on the stack, add extra refs to that op, and
thus fix that actual reported bug. The report also becomes a test case.
So we can quite easily clear up all the ones that people have actually
run into. We should also do a comprehensive review of all op types,
which will be relative drudge work and fix bugs less rapidly, and that's
the phase in which we might miss some needed fixes.

My arguments above may sound as though I want to make the stack keep
reference counts come hell or high water, but I am actually ambivalent.

I'm likewise open-minded regarding how we keep things on the stack alive;
I'm not opposed to stack refcounting per se. I'm concerned for the
maintainability issues that arise for all the relevant code.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2016

From zefram@fysh.org

I wrote​:

There's then some dance around redefining PERL_STACK_LOCALLY_REFCOUNTED.
I'm not sure whether the `normal' state for it should be,

Let's flesh this out a bit more. The safe way is that the normal state is
to have the macro undefined. That way, any code using POPs and friends
by default won't compile. This won't affect by-the-book XS code (see
below); this default applies to custom pp functions and hand-written
xsubs. Given that we break compilation of the pp functions themselves,
we don't need to change the name of the op_ppaddr struct member.

I'll illustrate the process with a simple case of a custom pp function
that performs dyadic integer addition. Today this function would likely
be written thus​:

  static OP *THX_pp_int_add(pTHX)
  {
  dSP;
  SV *right = POPs, *left = POPs, *result;
  PUTBACK;
  result = sv_2mortal(newSViv(SvIV(left) + SvIV(right)));
  SPAGAIN;
  PUSHs(result);
  PUTBACK;
  return NORMAL;
  }

When the refcounted-stack-capable Perl version comes out, this function
stops compiling. To restore it, it needs to be made capable of handling
both stack styles, and marked as such. A quick examination shows that
its stack usage is conventional enough to be covered by the compatibility
definitions of the stack macros, so all we need for the first updated
version is to locally define the switch macro​:

  #define PERL_STACK_LOCALLY_REFCOUNTED PERL_STACK_GLOBALLY_REFCOUNTED
  static OP *THX_pp_int_add(pTHX)
  {
  dSP;
  SV *right = POPs, *left = POPs, *result;
  PUTBACK;
  result = sv_2mortal(newSViv(SvIV(left) + SvIV(right)));
  SPAGAIN;
  PUSHs(result);
  PUTBACK;
  return NORMAL;
  }
  #undef PERL_STACK_LOCALLY_REFCOUNTED

Note that if we did not have the rather cautious PUTBACK/SPAGAIN around
the core operation then there would be a problem around non-local exits
that might arise from SvIV(). With the refcounted stack, it's vitally
important to reliably PUTBACK the stack pointer after pushing or popping
anything, before any possibility of a non-local exit. Previously that
wasn't necessary, because losing the stack pointer didn't affect what
references would be destroyed on unwinding. If the original version
had not had the PUTBACK, therefore, it would be necessary to add it in
the course of this update. (The SPAGAIN is technically superfluous,
but included for logical consistency with the PUTBACK.)

This version using the compatibility macros is relatively inefficient
because of the mortalisation. We can improve this by shifting to a
manner of stack usage that is oriented towards the refcounted stack.
In doing so we probably still want to maintain compatibility to the
uncounted style of stack. So we rewrite thus​:

  #define PERL_STACK_LOCALLY_REFCOUNTED PERL_STACK_GLOBALLY_REFCOUNTED
  static OP *THX_pp_int_add(pTHX)
  {
  dSP;
  SV *right = TOPs, *left = TOPm1s;
  SV *result = newSViv(SvIV(left) + SvIV(right));
  #if PERL_STACK_LOCALLY_REFCOUNTED
  SvREFCNT_dec(left);
  SvREFCNT_dec(right);
  #else
  sv_2mortal(result);
  #endif
  sp--;
  *sp = result;
  PUTBACK;
  return NORMAL;
  }
  #undef PERL_STACK_LOCALLY_REFCOUNTED

Safety for non-local exits is maintained by leaving the operands on the
stack during the actual operation, so the later replacement of operands
by result happens in a different way. This kind of stack operation
works fine with an uncounted stack too; the only conditional part is
the refcount handling.

I've written out the stack pointer and refcount manipulation explicitly,
but we'd probably want to make writing this sort of thing easier by
introducing another set of macros. Just as PUSHs and POPs are intended
for code written for the uncounted stack but provide compatibility
to both kinds of stack, so the new macros can be intended for code
oriented towards the counted stack but still provide compatibility.
Something like​:

  #define POPdiscard ((void) (PERL_STACK_LOCALLY_REFCOUNTED ? \
  SvREFCNT_dec(*sp--) : sp--))
  #define PUSHowned(s) ((void) (*++sp = PERL_STACK_LOCALLY_REFCOUNTED ? \
  (s) : sv_2mortal(s)))

PUSHowned() is quite close to the compatibility version of the existing
mPUSHs(). I think they're not identical, though​: since something pushed
with mPUSHs() might be popped off in a non-mortalising way, PUSHowned()
doesn't guarantee to make the scalar live as long as mPUSHs() does.
I think on the refcounted stack mPUSHs() needs to create a mortal
reference in addition to the non-mortal ref that will be owned by
the stack.

We'd also want a parameterised version of TOPm1s, because we're going
to be indexing a lot deeper into the stack without popping anything.

With these macros, our pp function becomes​:

  #define PERL_STACK_LOCALLY_REFCOUNTED PERL_STACK_GLOBALLY_REFCOUNTED
  static OP *THX_pp_int_add(pTHX)
  {
  dSP;
  SV *right = TOPs, *left = TOPm1s;
  SV *result = newSViv(SvIV(left) + SvIV(right));
  POPdiscard;
  POPdiscard;
  PUSHowned(result);
  PUTBACK;
  return NORMAL;
  }
  #undef PERL_STACK_LOCALLY_REFCOUNTED

This is almost as readable as the original. Although to maintain
compatibility to earlier Perl versions the file containing it
(or ppport.h) will need to acquire reserve definitions of
PERL_STACK_GLOBALLY_REFCOUNTED, POPdiscard, and PUSHowned().

In theory one could also write pp functions for one or other stack style
without an attempt at compatibility. One would then add an assertion
beside the function​:

  STATIC_ASSERT_GLOBAL(PERL_STACK_LOCALLY_REFCOUNTED);

For regular XS code we're going to have a compatibility mechanism so
that an update isn't required. But we want to make an update possible.
We'll need a new per-sub XS declaration to say which stack styles the code
is good for, defaulting to only uncounted stacks. Most XS subs don't
do any explicit stack manipulation, leaving it all to the XS framework.
Those subs can be quickly identified and marked, with just one extra
line, as OK for either kind of stack. The XS framework will locally
#define PERL_STACK_LOCALLY_REFCOUNTED for the sub depending on the
declared compatibility.

A problem arises with the stack manipulation code generated by the XS
framework, for handling arguments and return values, where they interact
with explicit stack manipulation in the XS source. Currently the options
for XS writers build in some knowledge that the position of the stack
pointer is a bit arbitrary, not affecting the lifetimes of the scalars
in question. This doesn't sit well with wanting to shift from POPs/PUSHs
semantics to TOPs/POPdiscard/PUSHowned semantics. Need more work here.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Aug 12, 2016

From @cpansprout

On Sat Jul 16 15​:31​:33 2016, zefram@​fysh.org wrote​:

Father Chrysostomos via RT wrote​:

How can we have POPs defined both ways in the same file?

Like this...

Core headers define macros to operate both ways based on a switch​:

\#define POPs \(PERL\_STACK\_LOCALLY\_REFCOUNTED ? \\
sv\_2mortal\(\*sp\-\-\) : \(\*sp\-\-\)\)
\#define PUSHs\(s\) \(PERL\_STACK\_LOCALLY\_REFCOUNTED ? \\
\(\*\+\+sp = SvREFCNT\_inc\(s\)\) : \(\*\+\+sp = \(s\)\)\)
\#define TOPs \(\*sp\) /\* same either way \*/

How would you suggest we handle call_sv and similar functions? Should we stop the macro wrapper from being generated by regen/embed.pl and define something like this manually​:

  #define call_sv(a,b) \
  call_sv_rstack(a,b,PERL_STACK_LOCALLY_REFCOUNTED)

where the _rstack version takes a bool parameter?

But what do we do with calls that explicitly use a Perl_ prefix? Should they just be assumed to be using the non-rc stack? Should Perl_call_sv be deprecated in favour of the macro, so that any code using it will warn at compile time?

I can see several ways of doing this. I would like the opinion of others.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Aug 12, 2016

From zefram@fysh.org

Father Chrysostomos via RT wrote​:

How would you suggest we handle call_sv and similar functions?

That's tricky. I reckon we can't get away with defining call_sv()
different ways. We'd need the actual function to know which kind of stack
it's been called from and handle it accordingly. Of course there's no
problem in having a per-flag stack to say which type it is​: we'll need
that anyway for unwinding. But it's quite a significant complication
to call_sv() and all other functions of that nature.

-zefram

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants