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

Bizarre copy of ARRAY in last #1849

Closed
p5pRT opened this issue Apr 16, 2000 · 17 comments
Closed

Bizarre copy of ARRAY in last #1849

p5pRT opened this issue Apr 16, 2000 · 17 comments

Comments

@p5pRT
Copy link

p5pRT commented Apr 16, 2000

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

Searchable as RT3112$

@p5pRT
Copy link
Author

p5pRT commented Apr 16, 2000

From @pjscott

for (1..4) {
  push @​a, $_, last if $_ == 3;
}
print "@​a\n";
__END__
Bizarre copy of ARRAY in last at - line 1.

The behavior is the same in Perl 5.005_04, 5.005_03, and 5.6.0. Of course
the user is wrong but why miss an opportunity to tell them so in more
florid terms? :-)

@p5pRT
Copy link
Author

p5pRT commented Apr 17, 2000

From [Unknown Contact. See original ticket]

Peter Scott <Peter@​PSDT.com> wrote

for (1..4) {
push @​a, $_, last if $_ == 3;
}
print "@​a\n";
__END__
Bizarre copy of ARRAY in last at - line 1.

The behavior is the same in Perl 5.005_04, 5.005_03, and 5.6.0. Of course
the user is wrong but why miss an opportunity to tell them so in more
florid terms? :-)

It fails back to at least 5.002. But it works in Perl4 if you add
brackets to the push. :-)

And it's not clear in what sense that's "wrong". It certainly shouldn't
attract a fatal error, tho' it would be a candidate for the proposed
new warning "next/last/return/ ... used where value wanted".

Mike Guy

@p5pRT
Copy link
Author

p5pRT commented Jul 6, 2003

From @floatingatoll

[Peter@​PSDT.com - Sun Apr 16 10​:54​:31 2000]​:

for (1..4) {
push @​a, $_, last if $_ == 3;
}
print "@​a\n";
__END__
Bizarre copy of ARRAY in last at - line 1.

Continues to be present in @​18673.

@p5pRT
Copy link
Author

p5pRT commented Apr 27, 2006

From p5p@spam.wizbit.be

[Peter@​PSDT.com - Sun Apr 16 10​:54​:31 2000]​:

for (1..4) {
push @​a, $_, last if $_ == 3;
}
print "@​a\n";
__END__
Bizarre copy of ARRAY in last at - line 1.

A somewhat shorter version of this problem​: perl -le 'for (1) { push @​a, last; }'

(If in the initial problem parans are used around push then the error goes away ( push (@​a, $_), last ... )

This problem still exists in bleadperl (and 5.8.8, and 5.9.3)...

Regards,

Animator

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2010

From @gannett-ggreer

On Thu Apr 27 11​:14​:44 2006, animator wrote​:

[Peter@​PSDT.com - Sun Apr 16 10​:54​:31 2000]​:

for (1..4) {
push @​a, $_, last if $_ == 3;
}
print "@​a\n";
__END__
Bizarre copy of ARRAY in last at - line 1.

A somewhat shorter version of this problem​: perl -le 'for (1) { push
@​a, last; }'

(If in the initial problem parans are used around push then the error
goes away ( push (@​a, $_), last ... )

This problem still exists in bleadperl (and 5.8.8, and 5.9.3)...

I've attached a stab at it. I added a new test and it passes the
current ones also, however I'm not 100% versed in the perl stack enough
to know if using SvTYPE is the most accurate criteria here.

http​://m-l.org/~perl/misc/0001-Fix-for-RT-3112-Bizarre-copy-of-ARRAY-when-using-las.patch

http​://github.com/greerga/perl/commit/aeb45651ef8a89da73a16a74c7b676305e8ec7cd

--
George Greer

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2010

From @gannett-ggreer

On Fri Jul 16 19​:32​:48 2010, greerga wrote​:

On Thu Apr 27 11​:14​:44 2006, animator wrote​:

[Peter@​PSDT.com - Sun Apr 16 10​:54​:31 2000]​:

for (1..4) {
push @​a, $_, last if $_ == 3;
}
print "@​a\n";
__END__
Bizarre copy of ARRAY in last at - line 1.

A somewhat shorter version of this problem​: perl -le 'for (1) { push
@​a, last; }'

(If in the initial problem parans are used around push then the
error
goes away ( push (@​a, $_), last ... )

This problem still exists in bleadperl (and 5.8.8, and 5.9.3)...

I've attached a stab at it. I added a new test and it passes the
current ones also, however I'm not 100% versed in the perl stack
enough
to know if using SvTYPE is the most accurate criteria here.

http​://m-l.org/~perl/misc/0001-Fix-for-RT-3112-Bizarre-copy-of-ARRAY-
when-using-las.patch

http​://github.com/greerga/perl/commit/aeb45651ef8a89da73a16a74c7b676305e8ec7cd

Duh. Now with 100% more attaching action.

--
George Greer

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2010

From @gannett-ggreer

0001-Fix-for-RT-3112-Bizarre-copy-of-ARRAY-when-using-las.patch
From aeb45651ef8a89da73a16a74c7b676305e8ec7cd Mon Sep 17 00:00:00 2001
From: George Greer <perl@greerga.m-l.org>
Date: Fri, 16 Jul 2010 22:25:27 -0400
Subject: [PATCH] Fix for RT#3112: Bizarre copy of ARRAY when using last from push.

last() shouldn't try to make a mortal copy of push/unshift's array
on the stack because sv_mortalcopy() can't handle a SVt_PVAV.
---
 pp_ctl.c       |    3 ++-
 t/op/loopctl.t |    8 +++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/pp_ctl.c b/pp_ctl.c
index a93d6dc..ea25810 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -2292,7 +2292,8 @@ PP(pp_last)
     }
     else if (gimme == G_ARRAY) {
 	while (++MARK <= SP) {
-	    *++newsp = ((pop2 == CXt_SUB) && SvTEMP(*MARK))
+	    *++newsp = ((pop2 == CXt_SUB && SvTEMP(*MARK))
+                        || SvTYPE(*MARK) == SVt_PVAV)
 			? *MARK : sv_mortalcopy(*MARK);
 	    TAINT_NOT;		/* Each item is independent */
 	}
diff --git a/t/op/loopctl.t b/t/op/loopctl.t
index d8faec1..54a4f27 100644
--- a/t/op/loopctl.t
+++ b/t/op/loopctl.t
@@ -36,7 +36,7 @@ BEGIN {
 }
 
 require "test.pl";
-plan( tests => 47 );
+plan( tests => 48 );
 
 my $ok;
 
@@ -978,3 +978,9 @@ cmp_ok($ok,'==',1,'dynamically scoped');
     cmp_ok("@a37725",'eq',"5 4 3 2",'bug 27725: reverse with empty slots bug');
 }
 
+{
+    # RT#3112
+    fresh_perl_is(<<'EOI', 'ok', { }, 'last in push');
+        for (1, 2) { push @a, last } print "ok"
+EOI
+}
-- 
1.7.0.4

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2010

From @rgarcia

On 17 July 2010 04​:32, George Greer via RT <perlbug-followup@​perl.org> wrote​:

On Thu Apr 27 11​:14​:44 2006, animator wrote​:

[Peter@​PSDT.com - Sun Apr 16 10​:54​:31 2000]​:

for (1..4) {
   push @​a, $_, last if $_ == 3;
}
print "@​a\n";
__END__
Bizarre copy of ARRAY in last at - line 1.

A somewhat shorter version of this problem​: perl -le 'for (1) { push
@​a, last; }'

(If in the initial problem parans are used around push then the error
goes away ( push (@​a, $_), last ... )

This problem still exists in bleadperl (and 5.8.8, and 5.9.3)...

I've attached a stab at it.  I added a new test and it passes the
current ones also, however I'm not 100% versed in the perl stack enough
to know if using SvTYPE is the most accurate criteria here.

Maybe it's better if you use >= instead of == for comparing the SV
type ? (so you'll include also other complex types, hashes, etc) Does
it work that way ?

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2010

From @nwc10

On Fri, Jul 16, 2010 at 07​:34​:17PM -0700, George Greer via RT wrote​:

On Fri Jul 16 19​:32​:48 2010, greerga wrote​:

On Thu Apr 27 11​:14​:44 2006, animator wrote​:

A somewhat shorter version of this problem​: perl -le 'for (1) { push
@​a, last; }'

(If in the initial problem parans are used around push then the
error
goes away ( push (@​a, $_), last ... )

This problem still exists in bleadperl (and 5.8.8, and 5.9.3)...

I've attached a stab at it. I added a new test and it passes the
current ones also, however I'm not 100% versed in the perl stack
enough
to know if using SvTYPE is the most accurate criteria here.

http​://m-l.org/~perl/misc/0001-Fix-for-RT-3112-Bizarre-copy-of-ARRAY-
when-using-las.patch

http​://github.com/greerga/perl/commit/aeb45651ef8a89da73a16a74c7b676305e8ec7cd

Duh. Now with 100% more attaching action.

Which, frustratingly, I can't persuade my mailer to inline for a reply. So
I've had to grab it​:

Inline Patch
diff --git a/pp_ctl.c b/pp_ctl.c
index a93d6dc..ea25810 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -2292,7 +2292,8 @@ PP(pp_last)
     }
     else if (gimme == G_ARRAY) {
 	while (++MARK <= SP) {
-	    *++newsp = ((pop2 == CXt_SUB) && SvTEMP(*MARK))
+	    *++newsp = ((pop2 == CXt_SUB && SvTEMP(*MARK))
+                        || SvTYPE(*MARK) == SVt_PVAV)
 			? *MARK : sv_mortalcopy(*MARK);
 	    TAINT_NOT;		/* Each item is independent */
 	}


I'm not even sure that that is right solution.

Looking through the code I see roughly that idiom in a lot of places.

1​: Why is this one special?
  (ie can the same problem hit the other paths?)
2​: What is the code actually trying to do?
  I *thought* that it's doing the following​:
  a​: FREETMPS and LEAVE have to be called to leave the subroutine
  b​: This would free mortals which are on the stack
  c​: Some of these need to survive
  d​: Therefore it copies the values of mortals (ie, onto an outer mortals
  stack which will last a bit longer)

  Only it's *not* doing that. It's copying everything that isn't marked as
  TEMP.
  Why does it need to do this at all? What is supposed to be on Perl's stack
  after a last? Is a last ever followed by anything other than pp_nextstate?
  (Which does this​:
  PL_stack_sp = PL_stack_base + cxstack[cxstack_ix].blk_oldsp;
  which I interpret as "reset the stack")

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jul 20, 2010

From @greerga

On Mon, 19 Jul 2010, Nicholas Clark wrote​:

On Fri, Jul 16, 2010 at 07​:34​:17PM -0700, George Greer via RT wrote​:

On Fri Jul 16 19​:32​:48 2010, greerga wrote​:

On Thu Apr 27 11​:14​:44 2006, animator wrote​:

A somewhat shorter version of this problem​: perl -le 'for (1) { push
@​a, last; }'

(If in the initial problem parans are used around push then the
error
goes away ( push (@​a, $_), last ... )

This problem still exists in bleadperl (and 5.8.8, and 5.9.3)...

I've attached a stab at it. I added a new test and it passes the
current ones also, however I'm not 100% versed in the perl stack
enough
to know if using SvTYPE is the most accurate criteria here.

http​://m-l.org/~perl/misc/0001-Fix-for-RT-3112-Bizarre-copy-of-ARRAY-
when-using-las.patch

http​://github.com/greerga/perl/commit/aeb45651ef8a89da73a16a74c7b676305e8ec7cd

Duh. Now with 100% more attaching action.

Which, frustratingly, I can't persuade my mailer to inline for a reply. So
I've had to grab it​:

diff --git a/pp_ctl.c b/pp_ctl.c
index a93d6dc..ea25810 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@​@​ -2292,7 +2292,8 @​@​ PP(pp_last)
}
else if (gimme == G_ARRAY) {
while (++MARK <= SP) {
- *++newsp = ((pop2 == CXt_SUB) && SvTEMP(*MARK))
+ *++newsp = ((pop2 == CXt_SUB && SvTEMP(*MARK))
+ || SvTYPE(*MARK) == SVt_PVAV)
? *MARK : sv_mortalcopy(*MARK);
TAINT_NOT; /* Each item is independent */
}

I'm not even sure that that is right solution.

Looking through the code I see roughly that idiom in a lot of places.

1​: Why is this one special?
(ie can the same problem hit the other paths?)
2​: What is the code actually trying to do?
I *thought* that it's doing the following​:
a​: FREETMPS and LEAVE have to be called to leave the subroutine
b​: This would free mortals which are on the stack
c​: Some of these need to survive
d​: Therefore it copies the values of mortals (ie, onto an outer mortals
stack which will last a bit longer)

Only it's *not* doing that. It's copying everything that isn't marked as
TEMP.
Why does it need to do this at all? What is supposed to be on Perl's stack
after a last? Is a last ever followed by anything other than pp_nextstate?
(Which does this​:
PL_stack_sp = PL_stack_base + cxstack[cxstack_ix].blk_oldsp;
which I interpret as "reset the stack")

Good questions, and I think I'll need to go diving with 'git blame' to
figure out the history of all this. My initial take was that it converted
stuff left on stack to mortals so they'd get freed and skipped temporaries
since they were already marked. Although like I said I'm fuzzy on the
stack uses so I'll see what I can find.

I'll also check out Rafael's suggestion of >=, assuming that SvTYPE
comparison survives the re-evaluation.

--
George Greer

@p5pRT
Copy link
Author

p5pRT commented Jul 1, 2013

From @tonycoz

On Mon Jul 19 21​:10​:20 2010, perl@​greerga.m-l.org wrote​:

On Mon, 19 Jul 2010, Nicholas Clark wrote​:

Why does it need to do this at all? What is supposed to be on
Perl's stack
after a last? Is a last ever followed by anything other than
pp_nextstate?
(Which does this​:
PL_stack_sp = PL_stack_base + cxstack[cxstack_ix].blk_oldsp;
which I interpret as "reset the stack")

Good questions, and I think I'll need to go diving with 'git blame' to
figure out the history of all this. My initial take was that it
converted
stuff left on stack to mortals so they'd get freed and skipped
temporaries
since they were already marked. Although like I said I'm fuzzy on the
stack uses so I'll see what I can find.

I'll also check out Rafael's suggestion of >=, assuming that SvTYPE
comparison survives the re-evaluation.

Based on the discussion, and that the patch no longer applies, I'm
rejecting the patch.

This bug is still present in blead v5.19.1-178-gdc12741, so I'm
obviously not closing the ticket.

I'm going to take the ticket to have a look at it at that far off time I
run out of patches to review, but if someone else wants to fiddle with
it, please steal the ticket or reply to the ticket with a note.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jul 1, 2013

From @cpansprout

On Mon Jul 19 03​:07​:26 2010, nicholas wrote​:

diff --git a/pp_ctl.c b/pp_ctl.c
index a93d6dc..ea25810 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@​@​ -2292,7 +2292,8 @​@​ PP(pp_last)
}
else if (gimme == G_ARRAY) {
while (++MARK <= SP) {
- *++newsp = ((pop2 == CXt_SUB) && SvTEMP(*MARK))
+ *++newsp = ((pop2 == CXt_SUB && SvTEMP(*MARK))
+ || SvTYPE(*MARK) == SVt_PVAV)
? *MARK : sv_mortalcopy(*MARK);
TAINT_NOT; /* Each item is independent */
}

I'm not even sure that that is right solution.

Looking through the code I see roughly that idiom in a lot of places.

1​: Why is this one special?
(ie can the same problem hit the other paths?)
2​: What is the code actually trying to do?
I *thought* that it's doing the following​:
a​: FREETMPS and LEAVE have to be called to leave the subroutine
b​: This would free mortals which are on the stack
c​: Some of these need to survive
d​: Therefore it copies the values of mortals (ie, onto an outer
mortals
stack which will last a bit longer)

Only it's *not* doing that. It's copying everything that isn't
marked as
TEMP.
Why does it need to do this at all? What is supposed to be on Perl's
stack
after a last? Is a last ever followed by anything other than
pp_nextstate?
(Which does this​:
PL_stack_sp = PL_stack_base + cxstack[cxstack_ix].blk_oldsp;
which I interpret as "reset the stack")

perl -e 'print do{{&{sub{"Just another Perl hacker,\n"}},last}}'

I once had some production code relying on this. I looked at my code,
spotted the bug in it, and then went to write a test for it. I found
that I already had a test that was *passing*! So I did a bit of
experimentation and found that last returns the current stack values in
certain cases. Of course, then I wrote that japh.

It seems to be that for it to do that is a mistake, and it should simply
reset the stack, like nextstate. I could be wrong.

This reminds me of grep and sort, which used to share some code (a
similar mistake), until I finally disentangled them in 354dd55.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 1, 2013

From @nwc10

On Mon, Jul 01, 2013 at 12​:06​:14AM -0700, Father Chrysostomos via RT wrote​:

On Mon Jul 19 03​:07​:26 2010, nicholas wrote​:

diff --git a/pp_ctl.c b/pp_ctl.c
index a93d6dc..ea25810 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@​@​ -2292,7 +2292,8 @​@​ PP(pp_last)
}
else if (gimme == G_ARRAY) {
while (++MARK <= SP) {
- *++newsp = ((pop2 == CXt_SUB) && SvTEMP(*MARK))
+ *++newsp = ((pop2 == CXt_SUB && SvTEMP(*MARK))
+ || SvTYPE(*MARK) == SVt_PVAV)
? *MARK : sv_mortalcopy(*MARK);
TAINT_NOT; /* Each item is independent */
}

I'm not even sure that that is right solution.

Looking through the code I see roughly that idiom in a lot of places.

1​: Why is this one special?
(ie can the same problem hit the other paths?)
2​: What is the code actually trying to do?
I *thought* that it's doing the following​:
a​: FREETMPS and LEAVE have to be called to leave the subroutine
b​: This would free mortals which are on the stack
c​: Some of these need to survive
d​: Therefore it copies the values of mortals (ie, onto an outer
mortals
stack which will last a bit longer)

Only it's *not* doing that. It's copying everything that isn't
marked as
TEMP.
Why does it need to do this at all? What is supposed to be on Perl's
stack
after a last? Is a last ever followed by anything other than
pp_nextstate?
(Which does this​:
PL_stack_sp = PL_stack_base + cxstack[cxstack_ix].blk_oldsp;
which I interpret as "reset the stack")

perl -e 'print do{{&{sub{"Just another Perl hacker,\n"}},last}}'

Thanks for the example which demonstrates how to trigger this code path.

I once had some production code relying on this. I looked at my code,
spotted the bug in it, and then went to write a test for it. I found
that I already had a test that was *passing*! So I did a bit of
experimentation and found that last returns the current stack values in
certain cases. Of course, then I wrote that japh.

It seems to be that for it to do that is a mistake, and it should simply
reset the stack, like nextstate. I could be wrong.

Yes, I'm not convinced that it's really a designed in feature. I can't think
that it's documented anywhere, or relied on by much code.

Given that the implementation would be quite a bit simpler if last simply
reset the stack, I think it would be better to consider this a "bug" and
"fix" it. There are too many quirks and special cases in the language
already - I don't see that blessing this one gains us more than it costs.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jul 1, 2013

From @cpansprout

On Mon Jul 01 02​:23​:14 2013, nicholas wrote​:

On Mon, Jul 01, 2013 at 12​:06​:14AM -0700, Father Chrysostomos via RT
wrote​:

It seems to be that for it to do that is a mistake, and it should simply
reset the stack, like nextstate. I could be wrong.

Yes, I'm not convinced that it's really a designed in feature. I can't
think
that it's documented anywhere, or relied on by much code.

Given that the implementation would be quite a bit simpler if last simply
reset the stack, I think it would be better to consider this a "bug" and
"fix" it. There are too many quirks and special cases in the language
already - I don't see that blessing this one gains us more than it costs.

And blessing it would involve blessing ‘Bizarre copy of ARRAY in last’,
which is a good argument against it.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2013

From @cpansprout

On Mon Jul 19 03​:07​:26 2010, nicholas wrote​:

On Fri, Jul 16, 2010 at 07​:34​:17PM -0700, George Greer via RT wrote​:

On Fri Jul 16 19​:32​:48 2010, greerga wrote​:

On Thu Apr 27 11​:14​:44 2006, animator wrote​:

A somewhat shorter version of this problem​: perl -le 'for (1) {
push
@​a, last; }'

(If in the initial problem parans are used around push then the
error
goes away ( push (@​a, $_), last ... )

This problem still exists in bleadperl (and 5.8.8, and 5.9.3)...

I've attached a stab at it. I added a new test and it passes the
current ones also, however I'm not 100% versed in the perl stack
enough
to know if using SvTYPE is the most accurate criteria here.

http​://m-l.org/~perl/misc/0001-Fix-for-RT-3112-Bizarre-copy-of-
ARRAY-
when-using-las.patch

http​://github.com/greerga/perl/commit/aeb45651ef8a89da73a16a74c7b676305e8ec7cd

Duh. Now with 100% more attaching action.

Which, frustratingly, I can't persuade my mailer to inline for a
reply. So
I've had to grab it​:

diff --git a/pp_ctl.c b/pp_ctl.c
index a93d6dc..ea25810 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@​@​ -2292,7 +2292,8 @​@​ PP(pp_last)
}
else if (gimme == G_ARRAY) {
while (++MARK <= SP) {
- *++newsp = ((pop2 == CXt_SUB) && SvTEMP(*MARK))
+ *++newsp = ((pop2 == CXt_SUB && SvTEMP(*MARK))
+ || SvTYPE(*MARK) == SVt_PVAV)
? *MARK : sv_mortalcopy(*MARK);
TAINT_NOT; /* Each item is independent */
}

I'm not even sure that that is right solution.

Looking through the code I see roughly that idiom in a lot of places.

1​: Why is this one special?
(ie can the same problem hit the other paths?)
2​: What is the code actually trying to do?
I *thought* that it's doing the following​:
a​: FREETMPS and LEAVE have to be called to leave the subroutine
b​: This would free mortals which are on the stack
c​: Some of these need to survive
d​: Therefore it copies the values of mortals (ie, onto an outer
mortals
stack which will last a bit longer)

Only it's *not* doing that. It's copying everything that isn't
marked as
TEMP.
Why does it need to do this at all?

As already discussed, it doesn’t need to do that for last (just reset
the stack).

But for other things, like eval and subroutines, copying appears to be
the intent. The assumption is that anything marked TEMP is unused
elsewhere and can therefore be returned without copying, since that fact
that it is not copied is not observable. (That assumption is the cause
of bug #105910. I think I see several other problems with it, too.)

George’s patch will probably actually fix bug #119797, but it would only
be a partial fix, since S_adjust_stack_on_leave would still do the wrong
thing for scalar lvalues.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2013

From @cpansprout

Fixed in 0c0c317.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 20, 2013

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

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

1 participant