unshift to @ISA - Githubissues

p5pRT commented 9 years ago

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

Searchable as RT126082$

p5pRT commented 9 years ago

From @VadimPushtaev

I got some strange behavior trying to unishift more than one value to @ISA.

5.14 and 5.22 are both affected.

Code:

use strict; use warnings;

our @ISA;

push( @ISA\, "A"); # OK unshift(@ISA\, "B"); # OK push( @ISA\, "C"\, "D"); # OK unshift(@ISA\, "D"\, "E"); # Recursive inheritance detected in package 'main'

output

Use of uninitialized value within @ISA in unshift at /home/v.pushtaev/bug.pl line 9. Use of uninitialized value within @ISA in unshift at /home/v.pushtaev/bug.pl line 9. Use of uninitialized value within @ISA in unshift at /home/v.pushtaev/bug.pl line 9. [...] Use of uninitialized value within @ISA in unshift at /home/v.pushtaev/bug.pl line 9. Use of uninitialized value within @ISA in unshift at /home/v.pushtaev/bug.pl line 9. Use of uninitialized value within @ISA in unshift at /home/v.pushtaev/bug.pl line 9. Recursive inheritance detected in package 'main' 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

Perl may be copied only under the terms of either the Artistic License or the GNU General Public License\, which may be found in the Perl 5 source kit.

Complete documentation for Perl\, including FAQ lists\, should be found on this system using "man perl" or "perldoc perl". If you have access to the Internet\, point your browser at http://www.perl.org/\, the Perl Home Page.

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

From zefram@fysh.org

Vadim Pushtaev wrote:

push( @ISA\, "C"\, "D"); # OK unshift(@ISA\, "D"\, "E"); # Recursive inheritance detected in package 'main'

This gives the impression that the repeating of a class name within @ISA might be relevant\, but that's a red herring. Shorter example:

$ perl -lwe 'unshift @ISA\, qw(A B)' 2>&1 | uniq -c 101 Use of uninitialized value in unshift at -e line 1. 1 Recursive inheritance detected in package 'main' at -e line 1.

-zefram

p5pRT commented 9 years ago

From @VadimPushtaev

Yes\, sorry for that typo\, it is confusing.

p5pRT commented 9 years ago

From [Unknown Contact. See original ticket]

Yes\, sorry for that typo\, it is confusing.

p5pRT commented 9 years ago

From @ap

Hi Vadim\,

* Vadim Pushtaev \perlbug\-followup@perl\.org [2015-09-17 15:15]:

unshift(@ISA\, "D"\, "E"); # Recursive inheritance detected in package 'main'

actually that error is probably irrelevant.

$ perl -we 'unshift @ISA\, ""' Recursive inheritance detected in package 'main' at -e line 1.

That is actually correct; there is no bug there.

I’d wager the undef warning points to the real problem. The recursive inheritance error is probably just the result of trying to coerce the undef to a string\, which gives the empty string\, which means the same as 'main'.

Further\,

$ perl -we 'push @ISA\, undef' 2>&1 | uniq -c 101 Use of uninitialized value $ISA[0] in unshift at -e line 1. 1 Recursive inheritance detected in package 'main' at -e line 1.

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 when you try to put undef in @ISA\, and one is that unshifting multiple values\, all defined\, somehow causes perl to try to unshift undef.

Regards\, -- Aristotle Pagaltzis // \<http://plasmasturm.org/>

p5pRT commented 9 years ago

From @VadimPushtaev

On Thu Sep 17 10:37:16 2015\, aristotle wrote:

Further\,

$ perl -we 'push @ISA\, undef' 2>&1 | uniq -c 101 Use of uninitialized value $ISA[0] in unshift at -e line 1. 1 Recursive inheritance detected in package 'main' at -e line 1.

Why does trying to push undef generate 100 extra undef warnings?

I don't quite understand why it generates any warnings at all. Pushing (and unshifting) undef is totally legit.

p5pRT commented 9 years ago

From @ap

* Vadim Pushtaev via RT \perlbug\-followup@perl\.org [2015-09-18 09:30]:

I don't quite understand why it generates any warnings at all. Pushing (and unshifting) undef is totally legit.

How so? It only makes sense for @ISA to contain package name strings. What should inheriting from an undef package mean?

p5pRT commented 9 years ago

From @VadimPushtaev

On Fri Sep 18 03:28:24 2015\, aristotle wrote:

* Vadim Pushtaev via RT \perlbug\-followup@perl\.org [2015-09-18 09:30]:

I don't quite understand why it generates any warnings at all. Pushing (and unshifting) undef is totally legit.

How so? It only makes sense for @ISA to contain package name strings. What should inheriting from an undef package mean?

I mean\, there is a problem with inheritance\, yes\, but not with pushing value into array (even though it's a special array).

p5pRT commented 9 years ago

From Eirik-Berg.Hanssen@allverden.no

On Thu\, Sep 17\, 2015 at 7:36 PM\, Aristotle Pagaltzis \pagaltzis@gmx\.de wrote:

So I wager that there are two bugs here - one is the loop that happens when you try to put undef in @ISA\, and one is that unshifting multiple values\, all defined\, somehow causes perl to try to unshift undef.

… yeah … unless behaviour under block eval consitutes a third bug:

eirik@purplehat[13:10:10]~$ perl -wle 'our(@ISA); $"="\, "; eval { push @ISA\, undef\, undef\, ""}; warn "\@ISA: [@ISA]\n"; warn main->foo' 2>&1 | uniq -c 101 Use of uninitialized value in push at -e line 1. 2 Use of uninitialized value in join or string at -e line 1. 1 @ISA: [\, \, ] 101 Use of uninitialized value in method with known name at -e line 1. 1 Recursive inheritance detected in package 'main' at -e line 1. eirik@purplehat[13:13:12]~$ $ perl -wle 'our(@ISA); $"="\, "; eval { unshift @ISA\, qw/ A B C D /}; warn "\@ISA: [@ISA]\n"; warn main->foo' 2>&1 | uniq -c 101 Use of uninitialized value in unshift at -e line 1. 3 Use of uninitialized value in join or string at -e line 1. 1 @ISA: [A\, \, \, ] 101 Use of uninitialized value in method with known name at -e line 1. 1 Recursive inheritance detected in package 'main' at -e line 1. eirik@purplehat[13:13:27]~$

Yikes.

Using block eval\, I _can_ push undef (without coercion) and even "" on @ISA. Somehow I was expecting either a fatal error or a roll-back to previous @ISA. Not sure if that in itself is a bug; it sure seems less than optimal.

Using block eval\, I _can_ unshift qw/ A B C D / on @ISA\, but it results in ("A"\, undef\, undef\, undef). Somehow. (There's a hint in the number of undefs there\, right?)

(In either case\, push/unshift runs a 101 loop\, and when @ISA is actually used\, it blows up again.)

Eirik

p5pRT commented 9 years ago

From @ap

* Vadim Pushtaev via RT \perlbug\-followup@perl\.org [2015-09-18 12:35]:

On Fri Sep 18 03:28:24 2015\, aristotle wrote:

* Vadim Pushtaev via RT \perlbug\-followup@perl\.org [2015-09-18 09:30]:

I don't quite understand why it generates any warnings at all. Pushing (and unshifting) undef is totally legit.

How so? It only makes sense for @ISA to contain package name strings. What should inheriting from an undef package mean?

I mean\, there is a problem with inheritance\, yes\, but not with pushing value into array

But at some point perl will try to use an @ISA entry as a package name and at that point it must convert the undef to a string\, so then it must warn. Even if @ISA were not otherwise special\, it would still be better for that warning to be triggered the instant you put the useless value on the array\, instead of e.g. 5 source files later when you go to call a method from the package\, because that way it’s much easier to find how the nonsense value got into @ISA.

(even though it's a special array).

But it is a special array. As soon as you touch it\, perl updates various things behind the scenes related to the inheritance graph as a whole. I don’t see any benefit from trying to hide that @ISA is not the same as a regular array. If you need a regular array\, use a regular array.

Regards\, -- Aristotle Pagaltzis // \<http://plasmasturm.org/>

p5pRT commented 9 years ago

From @rurban

On Sep 18\, 2015\, at 12:27 PM\, Aristotle Pagaltzis \pagaltzis@gmx\.de wrote:

* Vadim Pushtaev via RT \perlbug\-followup@perl\.org [2015-09-18 09:30]:

I don't quite understand why it generates any warnings at all. Pushing (and unshifting) undef is totally legit.

How so? It only makes sense for @ISA to contain package name strings. What should inheriting from an undef package mean?

Exactly. undef is wrong. valid types for @INC are PV\, AVREF and CVREF. warning on wrong types is very good.

p5pRT commented 9 years ago

From @ilmari

Reini Urban \reini\.urban@gmail\.com writes:

On Sep 18\, 2015\, at 12:27 PM\, Aristotle Pagaltzis \pagaltzis@gmx\.de wrote:

* Vadim Pushtaev via RT \perlbug\-followup@perl\.org [2015-09-18 09:30]:

I don't quite understand why it generates any warnings at all. Pushing (and unshifting) undef is totally legit.

How so? It only makes sense for @ISA to contain package name strings. What should inheriting from an undef package mean?

Exactly. undef is wrong. valid types for @INC are PV\, AVREF and CVREF. warning on wrong types is very good.

Nobody is disputing that. Can we please get back to the actual problem: the second and subsequent values in unshift(@ISA\, …) become undef\, and then it warns about that 101 times before dying with "Recursive inheritance detected".

-- "I use RMS as a guide in the same way that a boat captain would use a lighthouse. It's good to know where it is\, but you generally don't want to find yourself in the same spot." - Tollef Fog Heen

p5pRT commented 9 years ago

From @leonerd

On Fri\, 18 Sep 2015 16:14:34 +0100 ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) wrote:

Nobody is disputing that. Can we please get back to the actual problem: the second and subsequent values in unshift(@ISA\, …) become undef\, and then it warns about that 101 times before dying with "Recursive inheritance detected".

Random guess: 'unshift' has to create multiple holes at the start of the array so it doesn't suffer O(n^2) behaviour. Maybe it's these holes that perl objects to\, checking each subsequent one after setting individual slots?

my @arr = (1\,2\,3); unshift @arr\, (4\,5\,6);

==> @arr = ( 1\, 2\, 3 ) @arr = ( undef\, undef\, undef\, 1\, 2\, 3 ) @arr = ( 4\, undef\, undef\, 1\, 2\, 3 ) @arr = ( 4\, 5\, undef\, 1\, 2\, 3 ) @arr = ( 4\, 5\, 6\, 1\, 2\, 3 )

-- Paul "LeoNerd" Evans

leonerd@leonerd.org.uk http://www.leonerd.org.uk/ | https://metacpan.org/author/PEVANS

p5pRT commented 9 years ago

From @VadimPushtaev

On Fri Sep 18 08:24:21 2015\, leonerd@leonerd.org.uk wrote:

On Fri\, 18 Sep 2015 16:14:34 +0100 ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) wrote:

Nobody is disputing that. Can we please get back to the actual problem: the second and subsequent values in unshift(@ISA\, …) become undef\, and then it warns about that 101 times before dying with "Recursive inheritance detected".

Random guess: 'unshift' has to create multiple holes at the start of the array so it doesn't suffer O(n^2) behaviour. Maybe it's these holes that perl objects to\, checking each subsequent one after setting individual slots?

my @arr = (1\,2\,3); unshift @arr\, (4\,5\,6);
 ==>  @&#8203;arr = $ 1\, 2\, 3 $
      @&#8203;arr = $ undef\, undef\, undef\, 1\, 2\, 3 $
      @&#8203;arr = $ 4\,     undef\, undef\, 1\, 2\, 3 $
      @&#8203;arr = $ 4\,     5\,     undef\, 1\, 2\, 3 $
      @&#8203;arr = $ 4\,     5\,     6\,     1\, 2\, 3 $

Look like this.

Unshifting actually works outside the "main" package:

perl -we 'package Test; our @ISA; unshift @ISA\, 1\, 2\, 3; print @ISA' Use of uninitialized value within @Test::ISA in unshift at -e line 1. Use of uninitialized value within @Test::ISA in unshift at -e line 1. Use of uninitialized value within @Test::ISA in unshift at -e line 1. 123

I still warns but @ISA contains (1\,2\,3) afterwards.

p5pRT commented 9 years ago

From zefram@fysh.org

Paul "LeoNerd" Evans wrote:

Random guess: 'unshift' has to create multiple holes at the start of the array so it doesn't suffer O(n^2) behaviour.

Pretty much. pp_unshift() internally performs an av_unshift() followed by a bunch of av_store()s. av_unshift() doesn't take parameters for the values to unshift; it always sticks undefs in. (Actually null pointers internally.) The av_store() calls invoke magic on the array.

-zefram

p5pRT commented 9 years ago

From @ilmari

"Paul \"LeoNerd\" Evans" \leonerd@leonerd\.org\.uk writes:

On Fri\, 18 Sep 2015 16:14:34 +0100 ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) wrote:

Nobody is disputing that. Can we please get back to the actual problem: the second and subsequent values in unshift(@ISA\, …) become undef\, and then it warns about that 101 times before dying with "Recursive inheritance detected".

Random guess: 'unshift' has to create multiple holes at the start of the array so it doesn't suffer O(n^2) behaviour. Maybe it's these holes that perl objects to\, checking each subsequent one after setting individual slots?

my @arr = (1\,2\,3); unshift @arr\, (4\,5\,6);
 ==>  @&#8203;arr = $ 1\, 2\, 3 $
      @&#8203;arr = $ undef\, undef\, undef\, 1\, 2\, 3 $
      @&#8203;arr = $ 4\,     undef\, undef\, 1\, 2\, 3 $
      @&#8203;arr = $ 4\,     5\,     undef\, 1\, 2\, 3 $
      @&#8203;arr = $ 4\,     5\,     6\,     1\, 2\, 3 $

Sounds plausible. If we do it in a package other than "main"\, so we don't get the recursive inheritance error\, we get the number of warnings we would expect from that:

$ perl -wE 'package Foo; unshift @ISA\, qw(A B C); say "@ISA"' 2>&1 | uniq -c 3 Use of uninitialized value in unshift at -e line 1. 1 A B C

$ perl -wE 'package Foo; @ISA = (undef\, undef\, undef);' 2>&1 | uniq -c 3 Use of uninitialized value in list assignment at -e line 1.

There's a separate bug in that storing undef in @main::ISA warns 101 times before dying with "Recursive inheritance detected". The latter is arguably correct\, since undef stringifies to ""\, which means main:

$ perl -wE 'push @ISA\, ""' Recursive inheritance detected in package 'main' at -e line 1.

-- - Twitter seems more influential [than blogs] in the 'gets reported in the mainstream press' sense at least. - Matt McLeod - That'd be because the content of a tweet is easier to condense down to a mainstream media article. - Calle Dybedahl

p5pRT commented 9 years ago

From zefram@fysh.org

Vadim Pushtaev via RT wrote:

Unshifting actually works outside the "main" package:

The undefs\, when treated as class names\, are being coerced to the empty string\, which is an irregular name for the "main" package:

$ perl -lwe 'sub foo {"main::foo"} package Bar; @ISA = (undef); print Bar->foo' Use of uninitialized value in list assignment at -e line 1. main::foo

Hence the "recursive inheritance" when it's done in "main". That's another red herring. The problem at hand is really that the class system sees the undefs at all.

-zefram

p5pRT commented 9 years ago

From @leonerd

On Fri\, 18 Sep 2015 16:38:11 +0100 ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) wrote:

There's a separate bug in that storing undef in @main::ISA warns 101 times before dying with "Recursive inheritance detected".

Oops; sounds like the code to detect and warn against the chance of an infinite recursion bug itself suffers an infinite recursion bug.

recursion bug [n]: see 'recursion bug'

-- Paul "LeoNerd" Evans

leonerd@leonerd.org.uk http://www.leonerd.org.uk/ | https://metacpan.org/author/PEVANS

p5pRT commented 9 years ago

From @ilmari

Zefram \zefram@fysh\.org writes:

Vadim Pushtaev via RT wrote:

Unshifting actually works outside the "main" package:

The undefs\, when treated as class names\, are being coerced to the empty string\, which is an irregular name for the "main" package:

$ perl -lwe 'sub foo {"main::foo"} package Bar; @ISA = (undef); print Bar->foo' Use of uninitialized value in list assignment at -e line 1. main::foo

And the repeated warnings come from Perl_sv_2pv_flags() via SvPV_const(keysv\, klen) in hv_common(PL_stashcache\, namesv\, …) which is called by gv_stashsv(sv\, 0)\, which is called by S_mro_get_linear_isa_{dfs\,c3}\, which throw "Recursive inheritance" error after 100 levels of recursion.

-- "A disappointingly low fraction of the human race is\, at any given time\, on fire." - Stig Sandbeck Mathisen

p5pRT commented 9 years ago

From @ilmari

Zefram \zefram@fysh\.org writes:

Paul "LeoNerd" Evans wrote:

Random guess: 'unshift' has to create multiple holes at the start of the array so it doesn't suffer O(n^2) behaviour.

Pretty much. pp_unshift() internally performs an av_unshift() followed by a bunch of av_store()s. av_unshift() doesn't take parameters for the values to unshift; it always sticks undefs in. (Actually null pointers internally.) The av_store() calls invoke magic on the array.

Turns out there is a mechanism to delay the @ISA set magic until all the items have been assigned\, which pp_push() uses\, but not pp_unshift(). The attached patch applies it to the latter too.

p5pRT commented 9 years ago

From @ilmari

0001-perl-126082-Delay-ISA-magic-while-unshifting.patch

```diff From 87d12d06e76138f0bf4060edfead498554afce97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 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 ```

p5pRT commented 9 years ago

From @ilmari

I also noticed that pp_push() does:

SV *sv; if (*MARK) SvGETMAGIC(*MARK); sv = newSV(0); if (*MARK) sv_setsv_nomg(sv\, *MARK); av_store(ary\, AvFILLp(ary)+1\, sv);

while pp_unshift() just does:

SV * const sv = newSVsv(*++MARK); (void)av_store(ary\, i++\, sv);

Are they equivalent? If so\, can pp_push() be simplfied? If not\, does pp_unshift() need fixing?

-- "I use RMS as a guide in the same way that a boat captain would use a lighthouse. It's good to know where it is\, but you generally don't want to find yourself in the same spot." - Tollef Fog Heen

p5pRT commented 9 years ago

From @leonerd

On Fri\, 18 Sep 2015 17:48:12 +0100 ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) wrote:

+ 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;

Small thought - does that want to save/restore the old delay?

int old_delay = PL_delaymagic; PL_delaymagic = DM_DELAY;

...

PL_delaymagic = old_delay;

-- Paul "LeoNerd" Evans

leonerd@leonerd.org.uk http://www.leonerd.org.uk/ | https://metacpan.org/author/PEVANS

p5pRT commented 9 years ago

From @ilmari

"Paul \"LeoNerd\" Evans" \leonerd@leonerd\.org\.uk writes:

Small thought - does that want to save/restore the old delay?

int old_delay = PL_delaymagic; PL_delaymagic = DM_DELAY;

...

PL_delaymagic = old_delay;

Pl_delaymagic is only used in pp_push (and now pp_unshift) for delaying @ISA magic and in pp_aassign for delaying @ISA and uid/gid magic\, so I don't see why that would be necessary.

-- "A disappointingly low fraction of the human race is\, at any given time\, on fire." - Stig Sandbeck Mathisen

p5pRT commented 9 years ago

From @ap

* Reini Urban \reini\.urban@gmail\.com [2015-09-18 16:10]:

On Sep 18\, 2015\, at 12:27 PM\, Aristotle Pagaltzis \pagaltzis@gmx\.de wrote:

* Vadim Pushtaev via RT \perlbug\-followup@perl\.org [2015-09-18 09:30]:

I don't quite understand why it generates any warnings at all. Pushing (and unshifting) undef is totally legit.

How so? It only makes sense for @ISA to contain package name strings. What should inheriting from an undef package mean?

Exactly. undef is wrong. valid types for @INC are PV\, AVREF and CVREF. warning on wrong types is very good.

This ticket is about @ISA\, not @INC.

p5pRT commented 9 years ago

From @cowens

On Fri\, Sep 18\, 2015\, 12:59 Dagfinn Ilmari Mannsåker \ilmari@ilmari\.org wrote:

"Paul \"LeoNerd\" Evans" \leonerd@leonerd\.org\.uk writes:

Small thought - does that want to save/restore the old delay?

int old_delay = PL_delaymagic; PL_delaymagic = DM_DELAY;

...

PL_delaymagic = old_delay;

Pl_delaymagic is only used in pp_push (and now pp_unshift) for delaying @ISA magic and in pp_aassign for delaying @ISA and uid/gid magic\, so I don't see why that would be necessary.

-- "A disappointingly low fraction of the human race is\, at any given time\, on fire." - Stig Sandbeck

Mathisen

What about splice?

p5pRT commented 9 years ago

From @rurban

On Sep 19\, 2015\, at 1:38 AM\, Aristotle Pagaltzis \pagaltzis@gmx\.de wrote:

* Reini Urban \reini\.urban@gmail\.com [2015-09-18 16:10]:

On Sep 18\, 2015\, at 12:27 PM\, Aristotle Pagaltzis \pagaltzis@gmx\.de wrote:

* Vadim Pushtaev via RT \perlbug\-followup@perl\.org [2015-09-18 09:30]:

I don't quite understand why it generates any warnings at all. Pushing (and unshifting) undef is totally legit.

How so? It only makes sense for @ISA to contain package name strings. What should inheriting from an undef package mean?

Exactly. undef is wrong. valid types for @INC are PV\, AVREF and CVREF. warning on wrong types is very good.

This ticket is about @ISA\, not @INC.

Oops\, sorry. Mixed it up.

p5pRT commented 9 years ago

From @ilmari

"Chas. Owens" \chas\.owens@gmail\.com writes:

On Fri\, Sep 18\, 2015\, 12:59 Dagfinn Ilmari Mannsåker \ilmari@ilmari\.org wrote:

Pl_delaymagic is only used in pp_push (and now pp_unshift) for delaying @ISA magic and in pp_aassign for delaying @ISA and uid/gid magic\, so I don't see why that would be necessary.

What about splice?

pp_splice does not use PL_delaymagic\, but also does not seem to suffer from the undef problem of pp_unshift. The code is too complex for me to figure out exactly why this is the case right now.

-- "A disappointingly low fraction of the human race is\, at any given time\, on fire." - Stig Sandbeck Mathisen

p5pRT commented 9 years ago

From @ilmari

ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

"Chas. Owens" \chas\.owens@gmail\.com writes:

On Fri\, Sep 18\, 2015\, 12:59 Dagfinn Ilmari Mannsåker \ilmari@ilmari\.org wrote:

Pl_delaymagic is only used in pp_push (and now pp_unshift) for delaying @ISA magic and in pp_aassign for delaying @ISA and uid/gid magic\, so I don't see why that would be necessary.

What about splice?

pp_splice does not use PL_delaymagic\, but also does not seem to suffer from the undef problem of pp_unshift. The code is too complex for me to figure out exactly why this is the case right now.

A bit more thorough read of the code reveals that it manipulates the contents of AvARRAY directly\, and then calls mg_set() at the end\, so it's from premature @ISA magic triggering from av_store().

-- "The surreality of the universe tends towards a maximum" -- Skud's Law "Never formulate a law or axiom that you're not prepared to live with the consequences of." -- Skud's Meta-Law

p5pRT commented 9 years ago

From @ilmari

ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:

A bit more thorough read of the code reveals that it manipulates the contents of AvARRAY directly\, and then calls mg_set() at the end\, so it's from premature @ISA magic triggering from av_store(). ^ safe

-- - Twitter seems more influential [than blogs] in the 'gets reported in the mainstream press' sense at least. - Matt McLeod - That'd be because the content of a tweet is easier to condense down to a mainstream media article. - Calle Dybedahl

p5pRT commented 9 years ago

From @tonycoz

On Fri Sep 18 09:59:11 2015\, ilmari wrote:

"Paul \"LeoNerd\" Evans" \leonerd@leonerd\.org\.uk writes:

Small thought - does that want to save/restore the old delay?

int old_delay = PL_delaymagic; PL_delaymagic = DM_DELAY;

...

PL_delaymagic = old_delay;

Pl_delaymagic is only used in pp_push (and now pp_unshift) for delaying @ISA magic and in pp_aassign for delaying @ISA and uid/gid magic\, so I don't see why that would be necessary.

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; use warnings;

my $x;

tie $x\, 'F';

unshift @X::ISA\, $x\, "Z";

package X;

package Y;

package Z;

sub foo { }

package F; use parent 'Tie::Scalar';

sub TIESCALAR { bless {}; }

sub FETCH { push @G::ISA\, "H"; "Y"; }

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

p5pRT commented 9 years ago

From @ilmari

"Tony Cook via RT" \perlbug\-followup@perl\.org writes:

On Fri Sep 18 09:59:11 2015\, ilmari wrote:

"Paul \"LeoNerd\" Evans" \leonerd@leonerd\.org\.uk writes:

Small thought - does that want to save/restore the old delay?

int old_delay = PL_delaymagic; PL_delaymagic = DM_DELAY;

...

PL_delaymagic = old_delay;

Pl_delaymagic is only used in pp_push (and now pp_unshift) for delaying @ISA magic and in pp_aassign for delaying @ISA and uid/gid magic\, so I don't see why that would be necessary.

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.

Here's an updated patch that fixes that for both pp_push() and pp_unshift(). pp_aassign() needs fixing similarly\, but that's too convoluted for me to wrap my brain around at the moment.

p5pRT commented 9 years ago

From @ilmari

0001-perl-126082-Delay-ISA-magic-while-unshifting.patch

```diff From eea99cfaafb02fcf7e9bedee193ac036a35bca1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 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 ```

p5pRT commented 9 years ago

From @ilmari

With your patch the following warns for me:

use strict; use warnings;

my $x;

tie $x\, 'F';

unshift @X::ISA\, $x\, "Z";

package X;

package Y;

package Z;

sub foo { }

package F; use parent 'Tie::Scalar';

sub TIESCALAR { bless {}; }

sub FETCH { push @G::ISA\, "H"; "Y"; }

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

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

-- - Twitter seems more influential [than blogs] in the 'gets reported in the mainstream press' sense at least. - Matt McLeod - That'd be because the content of a tweet is easier to condense down to a mainstream media article. - Calle Dybedahl

p5pRT commented 9 years ago

From @iabyn

On Tue\, Sep 22\, 2015 at 12:06:11PM +0100\, Dagfinn Ilmari Mannsåker wrote:

"Tony Cook via RT" \perlbug\-followup@perl\.org writes:

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. Here's an updated patch that fixes that for both pp_push() and pp_unshift(). pp_aassign() needs fixing similarly\, but that's too convoluted for me to wrap my brain around at the moment.

+ ENTER; + SAVEI16(PL_delaymagic); [snip] + LEAVE;

Using the savestack has a quite a performance penalty. Using Porting/bench.pl to measure 'push @a\, 2' after a setup of 'my @a = 1'\, I got the following raw read\, write etc counts:

A: bleadperl with all PL_delaymagic code removed B: bleadperl C: bleadperl plus ENTER/SAVE(PL_delaymagic)/LEAVE

A B C -------- -------- -------- Ir 502.0 510.0 605.0 Dr 148.1 149.1 178.1 Dw 91.8 93.8 113.8 COND 77.3 78.3 87.3 IND 6.3 6.3 7.3

COND_m 4.3 4.1 3.8 IND_m 4.0 4.0 4.0

Ir_m1 0.1 0.2 0.1 Dr_m1 1.4 1.4 0.5 Dw_m1 0.7 0.7 0.7

Ir_mm 0.0 0.0 0.0 Dr_mm 0.0 0.0 0.0 Dw_mm 0.0 0.0 0.0

As you can see\, it added about a 20% overhead to the sum of padav(@a); const(1); push().

I wonder whether instead we could:

* in pp_push() etc just do

U16 = old_delay = PL_delaymagic; PL_delaymagic = ...; ... PL_delaymagic = old_delay;

* then add a 'saved_delaymagic' field to the jmpenv struct\, then make JMPENV_PUSH and JMPENV_POP save and restore PL_delaymagic.

That way the var gets restored on both normal and exception scope exits\, which not having a penalty in hot code like pp_hot and pp_aassign.

Or perhaps the whole PL_delaymagic thing needs rethinking\, and that perhaps there's a better way to do this. Not that I can think of one yet.

PS:

Ironically the PL_delaymagic code in pp_push seems to have been added purely as an efficiency measure\, so that 'push @ISA\, "X"\, "Y" doesn't have to recompute all the MRO stuff twice.

-- Counsellor Troi states something other than the blindingly obvious. -- Things That Never Happen in "Star Trek" #16

p5pRT commented 9 years ago

From @iabyn

On Thu\, Oct 01\, 2015 at 11:50:19AM +0100\, Dave Mitchell wrote:

As you can see\, it added about a 20% overhead to the sum of padav(@a); const(1); push().

I wonder whether instead we could:

* in pp_push() etc just do
U16 = old\_delay = PL\_delaymagic;
PL\_delaymagic = \.\.\.;
\.\.\.
PL\_delaymagic = old\_delay;
* then add a 'saved_delaymagic' field to the jmpenv struct\, then make JMPENV_PUSH and JMPENV_POP save and restore PL_delaymagic.

That way the var gets restored on both normal and exception scope exits\, which not having a penalty in hot code like pp_hot and pp_aassign.

I have now implemented that. Along with your patch\, I've just pushed it for smoking as smoke-me/davem/delaymg.

bench.pl shows that pp_push\, pp_unshift and pp_aassign are almost back at blead levels:

A = blead B = your patch: av_unshift fixed\, ENTER/SAVE/LEAVE added C = smoke-me/davem/delaymg: ENTER etc removed\, pp_aassign fixed

@a = (1\,2) A B C -------- -------- --------- Ir 1007.0 1007.0 1009.0 Dr 307.0 307.0 309.0 Dw 206.0 206.0 207.0 COND 137.0 137.0 137.0 IND 12.0 12.0 12.0

COND_m 2.0 1.8 2.0 IND_m 5.0 5.0 5.0

Ir_m1 0.0 0.0 0.1 Dr_m1 0.0 0.0 0.0 Dw_m1 0.0 0.0 0.0

Ir_mm 0.0 0.0 0.0 Dr_mm 0.0 0.0 0.0 Dw_mm 0.0 0.0 0.0

push @a\, 2 A B C -------- -------- --------- Ir 519.5 614.5 521.5 Dr 151.3 180.3 153.3 Dw 95.3 115.3 96.3 COND 80.2 89.2 80.2 IND 6.3 7.3 6.3

COND_m 4.5 4.3 4.9 IND_m 4.0 4.0 4.0

Ir_m1 0.1 -0.1 0.2 Dr_m1 0.5 0.4 0.2 Dw_m1 0.8 0.9 0.9

Ir_mm 0.0 0.0 0.0 Dr_mm 0.0 0.0 0.0 Dw_mm 0.0 0.0 0.0

unshift @a\, 2

A B C -------- -------- --------- Ir 500.7 601.7 508.7 Dr 147.1 177.1 150.1 Dw 91.6 113.6 94.6 COND 74.4 84.4 75.4 IND 6.3 7.3 6.3

COND_m 4.9 5.0 4.2 IND_m 4.1 4.1 4.1

Ir_m1 1.0 1.0 1.1 Dr_m1 1.2 0.7 0.9 Dw_m1 0.1 0.1 0.2

Ir_mm 0.0 0.0 0.0 Dr_mm 0.1 0.1 0.1 Dw_mm 0.0 0.0 0.0

-- Fire extinguisher (n) a device for holding open fire doors.

p5pRT commented 9 years ago

From @ilmari

Dave Mitchell \davem@iabyn\.com writes:

On Thu\, Oct 01\, 2015 at 11:50:19AM +0100\, Dave Mitchell wrote:
As you can see\, it added about a 20% overhead to the sum of padav(@a); const(1); push().

I wonder whether instead we could:

* in pp_push() etc just do
U16 = old\_delay = PL\_delaymagic;
PL\_delaymagic = \.\.\.;
\.\.\.
PL\_delaymagic = old\_delay;
* then add a 'saved_delaymagic' field to the jmpenv struct\, then make JMPENV_PUSH and JMPENV_POP save and restore PL_delaymagic.

That way the var gets restored on both normal and exception scope exits\, which not having a penalty in hot code like pp_hot and pp_aassign.
I have now implemented that. Along with your patch\, I've just pushed it for smoking as smoke-me/davem/delaymg.

Thanks for doing the work to fix the performance regression caused by my naïve approach (I know hardly anything about scope/exception handling in C/XS space).

bench.pl shows that pp_push\, pp_unshift and pp_aassign are almost back at blead levels:

Looks good to me.

-- "The surreality of the universe tends towards a maximum" -- Skud's Law "Never formulate a law or axiom that you're not prepared to live with the consequences of." -- Skud's Meta-Law

p5pRT commented 9 years ago

From @iabyn

On Tue\, Oct 13\, 2015 at 08:10:00PM +0100\, Dagfinn Ilmari Mannsåker wrote:

Dave Mitchell \davem@iabyn\.com writes:
On Thu\, Oct 01\, 2015 at 11:50:19AM +0100\, Dave Mitchell wrote:
As you can see\, it added about a 20% overhead to the sum of padav(@a); const(1); push().

I wonder whether instead we could:

* in pp_push() etc just do
U16 = old\_delay = PL\_delaymagic;
PL\_delaymagic = \.\.\.;
\.\.\.
PL\_delaymagic = old\_delay;
* then add a 'saved_delaymagic' field to the jmpenv struct\, then make JMPENV_PUSH and JMPENV_POP save and restore PL_delaymagic.

That way the var gets restored on both normal and exception scope exits\, which not having a penalty in hot code like pp_hot and pp_aassign.
I have now implemented that. Along with your patch\, I've just pushed it for smoking as smoke-me/davem/delaymg.
Thanks for doing the work to fix the performance regression caused by my naïve approach (I know hardly anything about scope/exception handling in C/XS space).

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. 3953914 Delay @ISA magic while unshifting

-- O Unicef Clearasil! Gibberish and Drivel! -- "Bored of the Rings"

p5pRT commented 9 years ago

@iabyn - Status changed from 'open' to 'pending release'

p5pRT commented 8 years ago

From @khwilliamson

Thank you for submitting this report. You have helped make Perl better.
With the release of Perl 5.24.0 on May 9\, 2016\, this and 149 other issues have been resolved.

Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0

p5pRT commented 8 years ago

@khwilliamson - Status changed from 'pending release' to 'resolved'

Perl / perl5

unshift to @ISA #14915

From @VadimPushtaev

From zefram@fysh.org

From @VadimPushtaev

From [Unknown Contact. See original ticket]

From @ap

From @VadimPushtaev

From @ap

From @VadimPushtaev

From Eirik-Berg.Hanssen@allverden.no

From @ap

From @rurban

From @ilmari

From @leonerd

From @VadimPushtaev

From zefram@fysh.org

From @ilmari

From zefram@fysh.org

From @leonerd

From @ilmari

From @ilmari

From @ilmari

From @ilmari

From @leonerd

From @ilmari

From @ap

From @cowens

From @rurban

From @ilmari

From @ilmari

From @ilmari

From @tonycoz

From @ilmari

From @ilmari

From @ilmari

From @iabyn

From @iabyn

From @ilmari

From @iabyn

From @khwilliamson