Fix two (er, four) sub:lvalue { &$x } bugs

The lvalue context that the last statement of an lvalue subroutine
provides, when applied to entersub, causes the ops below the entersub
to be complied oddly.  Compare regular subs and lvalue subs:

$ ./perl -Ilib -MO=Concise,bar,foo -e 'sub bar { &$x } sub foo:lvalue { &$x }'
main::bar:
5  <1> leavesub[1 ref] K/REFC,1 ->(end)
-     <@> lineseq KP ->5
1        <;> nextstate(main 1 -e:1) v ->2
4        <1> entersub[t2] K/TARG ->5
-           <1> ex-list K ->4
2              <0> pushmark s ->3
-              <1> ex-rv2cv vK ->-
-                 <1> ex-rv2sv sK/1 ->-
3                    <#> gvsv[*x] s ->4
main::foo:
b  <1> leavesublv[1 ref] K/REFC,1 ->(end)
-     <@> lineseq KP ->b
6        <;> nextstate(main 2 -e:1) v ->7
a        <1> entersub[t2] K/LVINTRO,TARG,INARGS ->b
-           <1> ex-list K ->a
7              <0> pushmark s ->8
9              <1> rv2cv vK/NO() ->a
-                 <1> ex-rv2sv sK/1 ->9
8                    <#> gvsv[*x] s ->9
-e syntax OK

Notice that, in the second case, the rv2cv is not being optimised
away.  Under strict mode, this allows a sub call on a string, since
rv2cv is not subject to strict refs.

It’s this code in op.c:op_lvalue_flags that is to blame:

		if (kid->op_type != OP_GV) {
		    /* Restore RV2CV to check lvalueness */
		  restore_2cv:
		    if (kid->op_next && kid->op_next != kid) { /* Happens? */
			okid->op_next = kid->op_next;
			kid->op_next = okid;
		    }
		    else
			okid->op_next = NULL;
		    okid->op_type = OP_RV2CV;
		    okid->op_targ = 0;
		    okid->op_ppaddr = PL_ppaddr[OP_RV2CV];
		    okid->op_private |= OPpLVAL_INTRO;
		    okid->op_private &= ~1;
		    break;
		}

This code is a little strange.  Using rv2cv to check lvalueness causes
the problem with strict refs.  The lvalue check could just as well go
in entersub.

The way this is currently written (and this is something I missed when
supposedly fixing lvalue subs), the rv2cv op will reject a non-lvalue
subroutine even when the caller is not called in lvalue context.

So we actually have two bugs.

Presumably the check was done in rv2cv to keep entersub fast.  But the
code I quoted above is only part of it.  There is also a special block
to create an rv2cv op anew to deal with method calls.

This commit fixes both issues by moving the run-time lvalueness check
to entersub.  I put it after PUSHSUB for speed in the most common
case (when there is no error).  PUSHSUB already calls a function
(was_lvalue_sub) to determine whether the current sub call is happen-
ing in lvalue context.  So the check I am adding after it only has to
check a couple of flags, instead of calling was_lvalue_sub itself.

This also fixes a bug I introduced earlier in the 5.15.x series.  This
is supposed to die (in fact, I made the mistake earlier of changing
tests that were checking for this, but so many tests were wrong back
then it was an easy mistake to make):

$ ./perl -Ilib -e 'sub bar {$x} sub foo:lvalue { bar}; foo=3'

And a fourth bug I discovered when writing tests:

sub AUTOLOAD :lvalue { warn autoloading; $x }
sub _102486 { warn "called" }
&{'_102486'} = 72;
warn $x
__END__
autoloading at - line 1.
72 at - line 4.

And it happens even if there is an lvalue sub defined under that name:

sub AUTOLOAD :lvalue { warn autoloading; $x }
sub _102486 :lvalue { warn "called" }
&{'_102486'} = 72;
warn $x
__END__
autoloading at - line 1.
72 at - line 4.

Since the sub cannot be seen at compile time, the lvalue check in
rv2cv, as mentioned above.  The autoloading is happening in rv2cv,
too, instead of entersub (the code is repeated), but the sub is not
checked for definition first.  It was put in rv2cv because it had to
come before the lvalue check.  Putting the latter in entersub lets us
delete that repeated autoload code, which is completely wrong anyway.

1 files changed, 3 insertions, 0 deletions

diff --git a/pp_hot.c b/pp_hot.c
index 99cd2e199a..a4e171c44d 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -2714,6 +2714,9 @@ try_autoload:
 		MARK++;
 	    }
 	}
+	if ((cx->blk_u16 & OPpENTERSUB_LVAL_MASK) == OPpLVAL_INTRO &&
+	    !CvLVALUE(cv))
+	    DIE(aTHX_ "Can't modify non-lvalue subroutine call");
 	/* warning must come *after* we fully set up the context
 	 * stuff so that __WARN__ handlers can safely dounwind()
 	 * if they want to