unshift to @ISA #14915
Comments
From @VadimPushtaevI got some strange behavior trying to unishift more than one value to @ISA. 5.14 and 5.22 are both affected. Code: use strict; our @ISA; push( @ISA, "A"); # OK output Use of uninitialized value within @ISA in unshift at /home/v.pushtaev/bug.pl line 9. perl -v This is perl 5, version 22, subversion 0 (v5.22.0) built for x86_64-linux Copyright 1987-2015, Larry Wall Perl may be copied only under the terms of either the Artistic License or the Complete documentation for Perl, including FAQ lists, should be found on |
|
The RT System itself - Status changed from 'new' to 'open' |
From zefram@fysh.orgVadim Pushtaev wrote:
This gives the impression that the repeating of a class name within @ISA $ perl -lwe 'unshift @ISA, qw(A B)' 2>&1 | uniq -c -zefram |
From @VadimPushtaevYes, sorry for that typo, it is confusing. |
From [Unknown Contact. See original ticket]Yes, sorry for that typo, it is confusing. |
From @apHi Vadim, * Vadim Pushtaev <perlbug-followup@perl.org> [2015-09-17 15:15]:
actually that error is probably irrelevant. $ perl -we 'unshift @ISA, ""' That is actually correct; there is no bug there. I’d wager the undef warning points to the real problem. The recursive Further, $ perl -we 'push @ISA, undef' 2>&1 | uniq -c Why does trying to push undef generate 100 extra undef warnings? So I wager that there are two bugs here - one is the loop that happens Regards, |
From @VadimPushtaevOn Thu Sep 17 10:37:16 2015, aristotle wrote:
I don't quite understand why it generates any warnings at all. Pushing (and unshifting) undef is totally legit. |
From @ap* Vadim Pushtaev via RT <perlbug-followup@perl.org> [2015-09-18 09:30]:
How so? It only makes sense for @ISA to contain package name strings. |
From @VadimPushtaevOn Fri Sep 18 03:28:24 2015, aristotle wrote:
I mean, there is a problem with inheritance, yes, but not with pushing value into array (even though it's a special array). |
From Eirik-Berg.Hanssen@allverden.noOn Thu, Sep 17, 2015 at 7:36 PM, Aristotle Pagaltzis <pagaltzis@gmx.de>
… yeah … unless behaviour under block eval consitutes a third bug: eirik@purplehat[13:10:10] Yikes. Using block eval, I _can_ push undef (without coercion) and even "" on Using block eval, I _can_ unshift qw/ A B C D / on @ISA, but it results (In either case, push/unshift runs a 101 loop, and when @ISA is actually Eirik |
From @ap* Vadim Pushtaev via RT <perlbug-followup@perl.org> [2015-09-18 12:35]:
But at some point perl will try to use an @ISA entry as a package name
But it is a special array. As soon as you touch it, perl updates various Regards, |
From @rurbanOn Sep 18, 2015, at 12:27 PM, Aristotle Pagaltzis <pagaltzis@gmx.de> wrote:
Exactly. |
From @ilmariReini Urban <reini.urban@gmail.com> writes:
Nobody is disputing that. Can we please get back to the actual problem: -- |
From @leonerdOn Fri, 18 Sep 2015 16:14:34 +0100
Random guess: 'unshift' has to create multiple holes at the start of my @arr = (1,2,3); ==> @arr = ( 1, 2, 3 ) -- leonerd@leonerd.org.uk |
From @VadimPushtaevOn Fri Sep 18 08:24:21 2015, leonerd@leonerd.org.uk wrote:
Look like this. Unshifting actually works outside the "main" package: perl -we 'package Test; our @ISA; unshift @ISA, 1, 2, 3; print @ISA' I still warns but @ISA contains (1,2,3) afterwards. |
From zefram@fysh.orgPaul "LeoNerd" Evans wrote:
Pretty much. pp_unshift() internally performs an av_unshift() followed -zefram |
From @ilmari"Paul \"LeoNerd\" Evans" <leonerd@leonerd.org.uk> writes:
Sounds plausible. If we do it in a package other than "main", so we $ perl -wE 'package Foo; unshift @ISA, qw(A B C); say "@ISA"' 2>&1 | uniq -c $ perl -wE 'package Foo; @ISA = (undef, undef, undef);' 2>&1 | uniq -c There's a separate bug in that storing undef in @main::ISA warns 101 $ perl -wE 'push @ISA, ""' -- |
From zefram@fysh.orgVadim Pushtaev via RT wrote:
The undefs, when treated as class names, are being coerced $ perl -lwe 'sub foo {"main::foo"} package Bar; @ISA = (undef); print Bar->foo' Hence the "recursive inheritance" when it's done in "main". That's -zefram |
From @leonerdOn Fri, 18 Sep 2015 16:38:11 +0100
Oops; sounds like the code to detect and warn against the chance of recursion bug [n]: see 'recursion bug' -- leonerd@leonerd.org.uk |
From @ilmariZefram <zefram@fysh.org> writes:
And the repeated warnings come from Perl_sv_2pv_flags() via -- |
From @ilmariZefram <zefram@fysh.org> writes:
Turns out there is a mechanism to delay the @ISA set magic until all the |
From @ilmari0001-perl-126082-Delay-ISA-magic-while-unshifting.patchFrom 87d12d06e76138f0bf4060edfead498554afce97 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 18 Sep 2015 17:40:01 +0100
Subject: [PATCH] [perl #126082] Delay @ISA magic while unshifting
pp_unshift() first calls av_unshift(), which prepends the the
requisite number of undefs, then calls av_store() for each item.
However, unlike pp_push() it was not setting PL_delaymagic around the
av_store() loop, so when unshifting onto @ISA, its magic would be
triggered while there were still undefs in the array, causig the
following spurious warning:
$ perl -wE 'package Foo; unshift @ISA, qw(A B)'
Use of uninitialized value in unshift at -e line 1.
---
pp.c | 5 +++++
t/op/magic.t | 10 +++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/pp.c b/pp.c
index 9dd3048..0e0cc7e 100644
--- a/pp.c
+++ b/pp.c
@@ -5496,10 +5496,15 @@ PP(pp_unshift)
else {
SSize_t i = 0;
av_unshift(ary, SP - MARK);
+ PL_delaymagic = DM_DELAY;
while (MARK < SP) {
SV * const sv = newSVsv(*++MARK);
(void)av_store(ary, i++, sv);
}
+ if (PL_delaymagic & DM_ARRAY_ISA)
+ mg_set(MUTABLE_SV(ary));
+
+ PL_delaymagic = 0;
}
SP = ORIGMARK;
if (OP_GIMME(PL_op, 0) != G_VOID) {
diff --git a/t/op/magic.t b/t/op/magic.t
index 4a8006d..eefb9cf 100644
--- a/t/op/magic.t
+++ b/t/op/magic.t
@@ -5,7 +5,7 @@ BEGIN
chdir 't' if -d 't';
@INC = '../lib';
require './test.pl';
- plan (tests => 190);
+ plan (tests => 191);
}
# Test that defined() returns true for magic variables created on the fly,
@@ -814,6 +814,14 @@ END
is scalar(keys(%ENV)), 0;
}
+{
+ local @ISA;
+ my $warned = 0;
+ local $SIG{__WARN__} = sub { ++$warned if $_[0] =~ /Use of uninitialized value in unshift/; print "# @_"; };
+ unshift @ISA, qw(A B C);
+ is $warned, 0, '[perl #126082] unshifting onto @ISA doesn\'t trigger set magic for each item';
+}
+
__END__
# Put new tests before the various ENV tests, as they blow %ENV away.
--
2.6.0.rc0.131.gf624c3d
|
From @ilmariI also noticed that pp_push() does: SV *sv; while pp_unshift() just does: SV * const sv = newSVsv(*++MARK); Are they equivalent? If so, can pp_push() be simplfied? If not, does -- |
From @leonerdOn Fri, 18 Sep 2015 17:48:12 +0100
Small thought - does that want to save/restore the old delay? int old_delay = PL_delaymagic; ... PL_delaymagic = old_delay; -- leonerd@leonerd.org.uk |
From @ilmari"Paul \"LeoNerd\" Evans" <leonerd@leonerd.org.uk> writes:
Pl_delaymagic is only used in pp_push (and now pp_unshift) for delaying -- |
From @ap* Reini Urban <reini.urban@gmail.com> [2015-09-18 16:10]:
This ticket is about @ISA, not @INC. |
From @cowensOn Fri, Sep 18, 2015, 12:59 Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> "Paul \"LeoNerd\" Evans" <leonerd@leonerd.org.uk> writes:
Pl_delaymagic is only used in pp_push (and now pp_unshift) for delaying -- Mathisen What about splice? |
From @rurbanOn Sep 19, 2015, at 1:38 AM, Aristotle Pagaltzis <pagaltzis@gmx.de> wrote:
Oops, sorry. Mixed it up. |
From @ilmari"Chas. Owens" <chas.owens@gmail.com> writes:
pp_splice does not use PL_delaymagic, but also does not seem to suffer -- |
From @ilmariilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
A bit more thorough read of the code reveals that it manipulates the -- |
From @ilmariilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
-- |
From @tonycozOn Fri Sep 18 09:59:11 2015, ilmari wrote:
While your patch does the same thing as pp_push, I'm not sure if there are cases where zeroing PL_delaymagic rather than saving/restoring is incorrect. If the value being pushed is tied, and the FETCH handler does a push(@Some::ISA) that will clear PL_delaymagic and so it won't be delayed later. With your patch the following warns for me: use strict; my $x; tie $x, 'F'; unshift @X::ISA, $x, "Z"; package X; package Y; package Z; sub foo { package F; sub TIESCALAR { sub FETCH { package G; package H; So it looks like the existing code in pp_push() is incorrect, as well as the similar code in your patch, and probably other code zeroing PL_delaymagic after use. Also, rather then Paul's save code, we should probably use a SAVE*() macro, so if magic croaks we can recover correctly. Tony |
From @ilmari"Tony Cook via RT" <perlbug-followup@perl.org> writes:
Here's an updated patch that fixes that for both pp_push() and |
From @ilmari0001-perl-126082-Delay-ISA-magic-while-unshifting.patchFrom eea99cfaafb02fcf7e9bedee193ac036a35bca1e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 18 Sep 2015 17:40:01 +0100
Subject: [PATCH] [perl #126082] Delay @ISA magic while unshifting
pp_unshift() first calls av_unshift(), which prepends the the
requisite number of undefs, then calls av_store() for each item.
However, unlike pp_push() it was not setting PL_delaymagic around the
av_store() loop, so when unshifting onto @ISA, its magic would be
triggered while there were still undefs in the array, causig the
following spurious warning:
$ perl -wE 'package Foo; unshift @ISA, qw(A B)'
Use of uninitialized value in unshift at -e line 1.
Also fix pp_push() to save and restore PL_delaymagic instead of
clearing it, so that e.g. unshifting a tied value with FETCH pushing
onto another @ISA doesn't erroneously clear the value from underneath
the unshift.
---
pp.c | 11 +++++++++--
t/op/magic.t | 23 ++++++++++++++++++++++-
2 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/pp.c b/pp.c
index 9dd3048..49a545a 100644
--- a/pp.c
+++ b/pp.c
@@ -5442,6 +5442,8 @@ PP(pp_push)
}
else {
if (SvREADONLY(ary) && MARK < SP) Perl_croak_no_modify();
+ ENTER;
+ SAVEI16(PL_delaymagic);
PL_delaymagic = DM_DELAY;
for (++MARK; MARK <= SP; MARK++) {
SV *sv;
@@ -5453,8 +5455,7 @@ PP(pp_push)
}
if (PL_delaymagic & DM_ARRAY_ISA)
mg_set(MUTABLE_SV(ary));
-
- PL_delaymagic = 0;
+ LEAVE;
}
SP = ORIGMARK;
if (OP_GIMME(PL_op, 0) != G_VOID) {
@@ -5496,10 +5497,16 @@ PP(pp_unshift)
else {
SSize_t i = 0;
av_unshift(ary, SP - MARK);
+ ENTER;
+ SAVEI16(PL_delaymagic);
+ PL_delaymagic = DM_DELAY;
while (MARK < SP) {
SV * const sv = newSVsv(*++MARK);
(void)av_store(ary, i++, sv);
}
+ if (PL_delaymagic & DM_ARRAY_ISA)
+ mg_set(MUTABLE_SV(ary));
+ LEAVE;
}
SP = ORIGMARK;
if (OP_GIMME(PL_op, 0) != G_VOID) {
diff --git a/t/op/magic.t b/t/op/magic.t
index 4a8006d..da7532e 100644
--- a/t/op/magic.t
+++ b/t/op/magic.t
@@ -5,7 +5,7 @@ BEGIN
chdir 't' if -d 't';
@INC = '../lib';
require './test.pl';
- plan (tests => 190);
+ plan (tests => 192);
}
# Test that defined() returns true for magic variables created on the fly,
@@ -681,6 +681,27 @@ END
pass('can read ${^E_NCODING} without blowing up');
is $_, undef, '${^E_NCODING} is undef';
+{
+ my $warned = 0;
+ local $SIG{__WARN__} = sub { ++$warned if $_[0] =~ /Use of uninitialized value in unshift/; print "# @_"; };
+ unshift @RT12608::A::ISA, qw(RT12608::B RT12608::C);
+ is $warned, 0, '[perl #126082] unshifting onto @ISA doesn\'t trigger set magic for each item';
+}
+
+{
+ my $warned = 0;
+ local $SIG{__WARN__} = sub { ++$warned if $_[0] =~ /Use of uninitialized value in unshift/; print "# @_"; };
+
+ my $x; tie $x, 'RT12608::F';
+ unshift @RT12608::X::ISA, $x, "RT12608::Z";
+ is $warned, 0, '[perl #126082] PL_delaymagic correctly/saved restored when pushing/unshifting onto @ISA';
+
+ package RT12608::F;
+ use parent 'Tie::Scalar';
+ sub TIESCALAR { bless {}; }
+ sub FETCH { push @RT12608::G::ISA, "RT12608::H"; "RT12608::Y"; }
+}
+
# ^^^^^^^^^ New tests go here ^^^^^^^^^
SKIP: {
--
2.6.0.rc0.131.gf624c3d
|
From @ilmari
-- |
From @iabynOn Tue, Sep 22, 2015 at 12:06:11PM +0100, Dagfinn Ilmari Mannsåker wrote:
Using the savestack has a quite a performance penalty. Using Porting/bench.pl A: bleadperl with all PL_delaymagic code removed A B C COND_m 4.3 4.1 3.8 Ir_m1 0.1 0.2 0.1 Ir_mm 0.0 0.0 0.0 As you can see, it added about a 20% overhead to the sum of padav(@a); I wonder whether instead we could: * in pp_push() etc just do U16 = old_delay = PL_delaymagic; * then add a 'saved_delaymagic' field to the jmpenv struct, then make That way the var gets restored on both normal and exception scope exits, Or perhaps the whole PL_delaymagic thing needs rethinking, and that PS: Ironically the PL_delaymagic code in pp_push seems to have been added -- |
From @iabynOn Thu, Oct 01, 2015 at 11:50:19AM +0100, Dave Mitchell wrote:
I have now implemented that. Along with your patch, I've just pushed it bench.pl shows that pp_push, pp_unshift and pp_aassign are almost A = blead @a = (1,2) COND_m 2.0 1.8 2.0 Ir_m1 0.0 0.0 0.1 Ir_mm 0.0 0.0 0.0 push @a, 2 COND_m 4.5 4.3 4.9 Ir_m1 0.1 -0.1 0.2 Ir_mm 0.0 0.0 0.0 unshift @a, 2 A B C COND_m 4.9 5.0 4.2 Ir_m1 1.0 1.0 1.1 Ir_mm 0.0 0.0 0.0 -- |
From @ilmariDave Mitchell <davem@iabyn.com> writes:
Thanks for doing the work to fix the performance regression caused by my
Looks good to me. -- |
From @iabynOn Tue, Oct 13, 2015 at 08:10:00PM +0100, Dagfinn Ilmari Mannsåker wrote:
Well, you took the 'correct' approach, just not the fastest :-) Your commit and my follow-up now merged into blead as a68090f optimise save/restore of PL_delaymagic. -- |
|
@iabyn - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for submitting this report. You have helped make Perl better. Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0 |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#126082 (status was 'resolved')
Searchable as RT126082$
The text was updated successfully, but these errors were encountered: