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

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

Closed
p5pRT opened this issue Dec 15, 2013 · 13 comments
Closed

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

p5pRT opened this issue Dec 15, 2013 · 13 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Dec 15, 2013

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

Searchable as RT120792$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 15, 2013

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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 15, 2013

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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 15, 2013

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 15, 2013

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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 15, 2013

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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 16, 2013

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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Dec 16, 2013

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

@p5pRT p5pRT closed this Dec 16, 2013
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 2, 2014

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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 6, 2014

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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 6, 2014

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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 7, 2014

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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 7, 2014

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

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented May 21, 2014

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

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

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.