Bleadperl v5.19.6-171-g437e3a7 breaks ROBIN/Want-0.21.tar.gz #13470

p5pRT · 2013-12-15T04:22:43Z

Migrated from rt.perl.org#120792 (status was 'resolved')

Searchable as RT120792$

p5pRT · 2013-12-15T04:22:43Z

From @andk

git bisect

commit 437e3a7
Author: Matthew Horsfall <WolfSage@gmail.com>
Date: Wed Dec 11 18:28:21 2013 -0500

Optimise out PUSHMARK/RETURN if return is the last statement in a sub.

diagnostics

Test Summary Report
-------------------
t/assign.t (Wstat: 65280 Tests: 6 Failed: 0)
Non-zero exit status: 255
Parse errors: Bad plan. You planned 10 tests but ran 6.
Files=8, Tests=142, 1 wallclock secs ( 0.04 usr 0.01 sys + 0.08 cusr 0.02 csys = 0.15 CPU)

perl -V

Summary of my perl5 (revision 5 version 19 subversion 7) configuration:
Commit id: 437e3a7
Platform:
osname=linux, osvers=3.11-2-amd64, archname=x86_64-linux
uname='linux k83 3.11-2-amd64 #1 smp debian 3.11.10-1 (2013-12-04) x86_64 gnulinux '
config_args='-Dprefix=/home/sand/src/perl/repoperls/installed-perls/perl/v5.19.6-171-g437e3a7/165a -Dmyhostname=k83 -Dinstallusrbinperl=n -Uversiononly -Dusedevel -des -Ui_db -Uuseithreads -Uuselongdouble -DDEBUGGING=-g'
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 -g',
cppflags='-fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
ccversion='', gccversion='4.8.2', 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/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib
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.17'
Dynamic Linking:
dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
cccdlflags='-fPIC', lddlflags='-shared -O2 -g -L/usr/local/lib -fstack-protector'

Characteristics of this binary (from libperl):
Compile-time options: HAS_TIMES PERLIO_LAYERS PERL_DONT_CREATE_GVSV
PERL_HASH_FUNC_ONE_AT_A_TIME_HARD PERL_MALLOC_WRAP
PERL_NEW_COPY_ON_WRITE PERL_PRESERVE_IVUV
PERL_USE_DEVEL USE_64_BIT_ALL USE_64_BIT_INT
USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE
USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_PERLIO
USE_PERL_ATOF
Built under linux
Compiled at Dec 13 2013 18:53:48
@INC:
/home/sand/src/perl/repoperls/installed-perls/perl/v5.19.6-171-g437e3a7/165a/lib/site_perl/5.19.7/x86_64-linux
/home/sand/src/perl/repoperls/installed-perls/perl/v5.19.6-171-g437e3a7/165a/lib/site_perl/5.19.7
/home/sand/src/perl/repoperls/installed-perls/perl/v5.19.6-171-g437e3a7/165a/lib/5.19.7/x86_64-linux
/home/sand/src/perl/repoperls/installed-perls/perl/v5.19.6-171-g437e3a7/165a/lib/5.19.7
.

--
andreas

p5pRT · 2013-12-15T13:47:41Z

From @wolfsage

On Sat, Dec 14, 2013 at 11:22 PM, Andreas J. Koenig via RT
<perlbug-followup@perl.org> wrote:

t/assign.t (Wstat: 65280 Tests: 6 Failed: 0)
Non-zero exit status: 255
Parse errors: Bad plan. You planned 10 tests but ran 6.
Files=8, Tests=142, 1 wallclock secs ( 0.04 usr 0.01 sys + 0.08 cusr 0.02 csys = 0.15 CPU)

Some background:

$ ./perl -Ilib assign.t
1..10
ok 1
ok 2
ok 3
ok 4
ok 5
ok 6
Died at assign.t line 42.

assign.t:

32: sub backstr :lvalue {
33: if (want('LVALUE')) {
34: carp("Not in ASSIGN context") unless want('ASSIGN');
35: my $a = want('ASSIGN');
36: $_[0] = reverse $a;
37: lnoreturn;
38: }
39: else {
40: rreturn scalar reverse $_[0];
41: }
42: die; return;
43: }

lnoreturn / rreturn from Want.pm:

sub rreturn (@) {
if (want_lvalue(1)) {
croak "Can't rreturn in lvalue context";
}
double_return();
return wantarray ? @_ : $_[$#_];
}

sub lnoreturn () {
if (!want_lvalue(1) || !want_assign(1)) {
croak "Can't lnoreturn except in ASSIGN context";
}
double_return();
return disarm_temp(my $undef);
}

double_return from Want.xs:

void
double_return()
PREINIT:
PERL_CONTEXT *ourcx, *cx;
PPCODE:
ourcx = upcontext(aTHX_ 0);
cx = upcontext(aTHX_ 1);
if (!cx)
Perl_croak(aTHX_ "Can't return outside a subroutine");

ourcx->cx_type = CXt_NULL;
CvDEPTH(ourcx->blk_sub.cv)--;
#if HAS_RETSTACK
if (PL_retstack_ix > 0)
--PL_retstack_ix;
#endif

return;

(My knowledge in this area is very limited, apologies if this makes no sense)

What it looks like to me is happening is double_return pops a stack
frame, effectively putting the return calls in the sub above lnoreturn
and rreturn.

My optimisation unfortunately removes the returns (since they're at
the end of the lnoreturn / rreturn subs) and so the test gets treated
like this:

32: sub backstr :lvalue {
33: if (want('LVALUE')) {
34: carp("Not in ASSIGN context") unless want('ASSIGN');
35: my $a = want('ASSIGN');
36: $_[0] = reverse $a;
37: undef; # Falls through to die now
38: }
39: else {
40: scalar reverse $_[0]; # Falls through to die now
41: }
42: die; return;
43: }

What I don't know is if Want is violating some rules with regards to
the Perl internals and doing something it shouldn't...

This patch does however fix the issue since the extra scope prevents
the optimisation from working. (That is, until we some day make this
optimisation smarter):

--- Want.pm 2012-02-29 13:54:14.000000000 -0500
+++ Want.pm 2013-12-15 08:43:55.000000000 -0500
@@ -169,7 +169,7 @@
croak "Can't rreturn in lvalue context";
}
double_return();
- return wantarray ? @_ : $_[$#_];
+ { return wantarray ? @_ : $_[$#_]; }
}

sub lnoreturn () {
@@ -177,7 +177,7 @@
croak "Can't lnoreturn except in ASSIGN context";
}
double_return();
- return disarm_temp(my $undef);
+ { return disarm_temp(my $undef); }
}

# Some naughty people were relying on these internal methods.

-- Matthew Horsfall (alh)

p5pRT · 2013-12-15T13:47:41Z

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

p5pRT · 2013-12-15T14:17:06Z

From perl5-porters@perl.org

Matthew Horsfall wrote:

What it looks like to me is happening is double_return pops a stack
frame, effectively putting the return calls in the sub above lnoreturn
and rreturn.

Yes, it sets things up in such a way that pp_return will pop one stack
further than it would have.

Because pp_leavesub assumes there is only one stack frame to pop, it
does not even notice.

What I don't know is if Want is violating some rules with regards to
the Perl internals and doing something it shouldn't...

Why else does it exist? :-)

This patch does however fix the issue since the extra scope prevents
the optimisation from working.

I think that patch should work. Will you do the honours of sub-
mitting it?

(That is, until we some day make this
optimisation smarter):

Applying the optimisation in those cases where the return is nested
inside blocks would result in *more* op calls, because of all the
pp_leave()s, etc. I think pp_return/last/goto, etc., are the fastest
way to exit multiple scopes.

p5pRT · 2013-12-15T14:33:11Z

From @wolfsage

On Sun, Dec 15, 2013 at 9:16 AM, Father Chrysostomos <sprout@cpan.org> wrote:

Matthew Horsfall wrote:

What I don't know is if Want is violating some rules with regards to
the Perl internals and doing something it shouldn't...

Why else does it exist? :-)

Heheh, fair point.

This patch does however fix the issue since the extra scope prevents
the optimisation from working.

I think that patch should work. Will you do the honours of sub-
mitting it?

Done. For now I've filed a pull request, I'll make an RT bug if he
doesn't notice it in a week or so.

robinhouston/Want#1

I've taken this the ticket for now.

Thanks,

-- Matthew Horsfall (alh)

p5pRT · 2013-12-16T16:48:18Z

From @wolfsage

The module author has accepted the patch and pushed a new version:

http://search.cpan.org/~robin/Want-0.22/

Closing.

-- Matthew Horsfall (alh)

p5pRT · 2013-12-16T16:48:19Z

@wolfsage - Status changed from 'open' to 'resolved'

p5pRT · 2014-05-02T17:44:10Z

From @andk

"Matthew Horsfall via RT" <perlbug-followup@perl.org> writes:

The module author has accepted the patch and pushed a new version:

http://search.cpan.org/~robin/Want-0.22/

Closing.

Maybe to be reopened. Another affected module is JJORE/overload-eval-0.10.tar.gz

--
andreas

p5pRT · 2014-05-06T10:09:19Z

From @iabyn

On Fri, May 02, 2014 at 07:43:28PM +0200, Andreas Koenig wrote:

"Matthew Horsfall via RT" <perlbug-followup@perl.org> writes:

The module author has accepted the patch and pushed a new version:

http://search.cpan.org/~robin/Want-0.22/

Closing.

Maybe to be reopened. Another affected module is JJORE/overload-eval-0.10.tar.gz

I see test failures with that module prior to that commit.

--
Technology is dominated by two types of people: those who understand what
they do not manage, and those who manage what they do not understand.

p5pRT · 2014-05-06T17:56:58Z

From @andk

"Dave Mitchell via RT" <perlbug-followup@perl.org> writes:

Closing.

Maybe to be reopened. Another affected module is JJORE/overload-eval-0.10.tar.gz

I see test failures with that module prior to that commit.

Why so terse? At cpantesters? URL? Commit ID?

--
andreas

p5pRT · 2014-05-07T04:56:16Z

From @andk

Andreas Koenig <andreas.koenig.7os6VVqR@franz.ak.mind.de> writes:

"Dave Mitchell via RT" <perlbug-followup@perl.org> writes:

Closing.

Maybe to be reopened. Another affected module is JJORE/overload-eval-0.10.tar.gz

I see test failures with that module prior to that commit.

Why so terse? At cpantesters? URL? Commit ID?

There is no red below the 5.19.7 row in the matrix:

http://matrix.cpantesters.org/?dist=overload-eval%200.10

--
andreas

p5pRT · 2014-05-07T11:18:24Z

From @iabyn

On Wed, May 07, 2014 at 06:55:38AM +0200, Andreas Koenig wrote:

Andreas Koenig <andreas.koenig.7os6VVqR@franz.ak.mind.de> writes:

"Dave Mitchell via RT" <perlbug-followup@perl.org> writes:

Closing.

Maybe to be reopened. Another affected module is JJORE/overload-eval-0.10.tar.gz

I see test failures with that module prior to that commit.

Why so terse? At cpantesters? URL? Commit ID?

There is no red below the 5.19.7 row in the matrix:

http://matrix.cpantesters.org/?dist=overload-eval%200.10

Sorry, I meant that I downloaded the module locally, and tried building
with blead and saw failures. Then tried with a slightly older 5.19.6 perl
binary (the first release older than the commit in question) and still saw
failures (different ones, but failures still).

But having tried to reproduce it just now, I'm no longer seeing test
failures on 5.19.6, so maybe I did something wrong earlier.

I'll look into this further.

--
Spock (or Data) is fired from his high-ranking position for not being able
to understand the most basic nuances of about one in three sentences that
anyone says to him.
-- Things That Never Happen in "Star Trek" #19

p5pRT · 2014-05-21T10:10:04Z

From @iabyn

On Wed, May 07, 2014 at 12:17:48PM +0100, Dave Mitchell wrote:

On Wed, May 07, 2014 at 06:55:38AM +0200, Andreas Koenig wrote:

Andreas Koenig <andreas.koenig.7os6VVqR@franz.ak.mind.de> writes:

Maybe to be reopened. Another affected module is JJORE/overload-eval-0.10.tar.gz

TL;DR: its not a regression from 5.18.

The commit in question (which just optimises 'return expr' at the end of a
sub into 'expr') turns out to be a red herring.

After that commit, you get an assertion failure in t/basic.t due to a
freed var being accessed. The code in question is a perl sub which is
executing the overloaded eval, by eventually doing the actual eval:

sub wantarray {
no overload::eval;
...;
return eval shift;
}

The offending commit logically converts the sub into

sub wantarray {
no overload::eval;
...;
eval shift;
}

which causes the assertion failure. *But*: you get this failure
on perls prior to that commit if you remove the 'return' keyword from the
test file. So this commit didn't break anything, fortunately, it just
flagged an existing issue.

As to what the underlying issue is, I'm not exactly sure. The basic thing
is that if you eval a string, and the generated code ends with a const op
- for example, eval '5;', which compiles to const(5); leaveeval - and if the
eval is in void context, then the const SV is pushed on the stack, and
leaveval doesn't bother making a copy of it (since we're in void
context), then the optree is deleted, including the OP_CONST, which frees
the const SV, leaving a freed SV on the stack:

$ p -Dst -e'eval "5;"'

....

(-e:1) entereval
=>
((eval 1):0) nextstate
=>
((eval 1):1) const(IV(5))
=> IV(5)
((eval 1):1) leaveeval
=> (FREED)
(-e:1) leave

Normally this is isn't an issue, but with overload::eval allowing you to
create a perl-level wrapper around the real eval call, that freed SV is
surviving long enough to cause assertion failures.

Whether that's an issue for overload::eval or perl, I don't know.
However, since I've confirmed that it isn't a 5.20 regression (I can get
the same assertion failure in 5.18.2 by removing 'return' from t/basic.t),
I'll let someone else worry about it.

--
The Enterprise is involved in a bizarre time-warp experience which is in
some way unconnected with the Late 20th Century.
-- Things That Never Happen in "Star Trek" #14

p5pRT closed this as completed Dec 16, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bleadperl v5.19.6-171-g437e3a7 breaks ROBIN/Want-0.21.tar.gz #13470

Bleadperl v5.19.6-171-g437e3a7 breaks ROBIN/Want-0.21.tar.gz #13470

p5pRT commented Dec 15, 2013

p5pRT commented Dec 15, 2013

p5pRT commented Dec 15, 2013

p5pRT commented Dec 15, 2013

p5pRT commented Dec 15, 2013

p5pRT commented Dec 15, 2013

p5pRT commented Dec 16, 2013

p5pRT commented Dec 16, 2013

p5pRT commented May 2, 2014

p5pRT commented May 6, 2014

p5pRT commented May 6, 2014

p5pRT commented May 7, 2014

p5pRT commented May 7, 2014

p5pRT commented May 21, 2014

Bleadperl v5.19.6-171-g437e3a7 breaks ROBIN/Want-0.21.tar.gz #13470

Bleadperl v5.19.6-171-g437e3a7 breaks ROBIN/Want-0.21.tar.gz #13470

Comments

p5pRT commented Dec 15, 2013

p5pRT commented Dec 15, 2013

From @andk

p5pRT commented Dec 15, 2013

From @wolfsage

p5pRT commented Dec 15, 2013

p5pRT commented Dec 15, 2013

From perl5-porters@perl.org

p5pRT commented Dec 15, 2013

From @wolfsage

p5pRT commented Dec 16, 2013

From @wolfsage

p5pRT commented Dec 16, 2013

p5pRT commented May 2, 2014

From @andk

p5pRT commented May 6, 2014

From @iabyn

p5pRT commented May 6, 2014

From @andk

p5pRT commented May 7, 2014

From @andk

p5pRT commented May 7, 2014

From @iabyn

p5pRT commented May 21, 2014

From @iabyn