pp_leavesublv(): croak on *all* PADTMPs

pp_leavesublv() generally croaks on returned PADTMPs when called in lvalue
context. However, an exception was made in scalar context if the PADTMP
had set magic. This was to allow for things like

    sub :lvalue { $tied{foo} }
and
    sub :lvalue { substr($foo,1,2) }

However, it was later found that in places like pp_substr(), when
returning a PVLV, it should return a new mortal rather than upgrading
its pad target to PVLV, because the PVLV holds a reference to $foo which
then gets delayed being freed indefinitely.

Since places like pp_susbtr() no longer return lvalue PADTMPs, there's
no longer any need to make a special exception in pp_leavesublv().

I've added an assertion to the effect that PADTMPs don't have set
container magic, but I've added the assert to pp_leavesub() rather than
pp_leavesublv(), since the former is much likely to be used in many weird
situations and edge cases that would quickly flush out any false
assumptions.

If this assumption is wrong and the exception needs to be re-instated in
pp_leavesublv(), then presumably it needs adding to the ARRAY context
branch too - I'm assuming that its previous absence there was an oversight;
i.e.

    sub foo :lvalue { $tied{foo}, substr($foo,1,2) }
    foo() = (1,2);

should work too.

diff --git a/pp_ctl.c b/pp_ctl.c
index c81df19..b49f86d 100644 (file)
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -2337,8 +2337,7 @@ PP(pp_leavesublv)
     if (gimme == G_SCALAR) {
        if (CxLVAL(cx) && !ref) {     /* Leave it as it is if we can. */
            if (MARK <= SP) {
-               if ((SvPADTMP(TOPs) || SvREADONLY(TOPs)) &&
-                   !SvSMAGICAL(TOPs)) {
+               if ((SvPADTMP(TOPs) || SvREADONLY(TOPs))) {
                    what =
                        SvREADONLY(TOPs) ? (TOPs == &PL_sv_undef) ? "undef"
                        : "a readonly value" : "a temporary";

diff --git a/pp_hot.c b/pp_hot.c
index 4c6beb4..3ed672d 100644 (file)
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -3381,6 +3381,35 @@ S_leavesub_adjust_stacks(pTHX_ SV **base_sp, I32 gimme)
             SV *sv = *from_sp++;
 
             assert(PL_tmps_ix + nargs < PL_tmps_max);
+#ifdef DEBUGGING
+            /* PADTMPs with container set magic shouldn't appear in the
+             * wild. This assert is more important for pp_leavesublv(),
+             * but by testing for it here, we're more likely to catch
+             * bad cases (what with :lvalue subs not being widely
+             * deployed). The two issues are that for something like
+             *     sub :lvalue { $tied{foo} }
+             * or
+             *     sub :lvalue { substr($foo,1,2) }
+             * pp_leavesublv() will croak if the sub returns a PADTMP,
+             * and currently functions like pp_substr() return a mortal
+             * rather than using their PADTMP when returning a PVLV.
+             * This is because the PVLV will hold a ref to $foo,
+             * so $foo would get delayed in being freed while
+             * the PADTMP SV remained in the PAD.
+             * So if this assert fails it means either:
+             *  1) there is pp code similar to pp_substr that is
+             *     returning a PADTMP instead of a mortal, and probably
+             *     needs fixing, or
+             *  2) pp_leavesub is making unwarranted assumptions
+             *     about always croaking on a PADTMP
+             */
+            if (SvPADTMP(sv) && SvSMAGICAL(sv)) {
+                MAGIC *mg;
+                for (mg = SvMAGIC(sv); mg; mg = mg->mg_moremagic) {
+                    assert(PERL_MAGIC_TYPE_IS_VALUE_MAGIC(mg->mg_type));
+                }
+            }
+#endif
 
             if (SvTEMP(sv) && !SvMAGICAL(sv) && SvREFCNT(sv) == 1) {
                 /* can optimise away the copy */