`pack` folded constants get the SvPADTMP flag set

[leonerd]

Calls to pack with constant arguments get constant-folded at compiletime, leaving only their result in the optree:

$ perl -MO=Concise -E 'my $x = pack "C", 65'
6  <@> leave[1 ref] vKP/REFC ->(end)
1     <0> enter v ->2
2     <;> nextstate(main 2 -e:1) v:%,us,{,fea=15 ->3
5     <2> sassign vKS/2 ->6
3        <$> const[PV "A"] s/FOLD ->4
4        <0> padsv[$x:2,3] sRM*/LVINTRO ->5

Note the const[PV "A"] as step 3 here.

On threaded perls, OP_CONST being an SVOP will store its SV value on the pad.

Unfortunately, the SV stored in the pad has the SvPADTMP flag set. (I don't have an easy one-liner to demonstrate this one, but it became evident in debugging the issue mentioned below).

This probably doesn't matter to most situations (indeed, core perl doesn't appear to care), but one place it does cause a problem is when my cv_copy_flags() function is taking a copy of the CV. Object::Pad uses this to take a copy of a role method to inject into a target class. When copying a CV known not to be currently running (because this happens during compiletime), there's no need to copy the PADTMPs, so it instead just makes new empty SVs there:

      else if(SvPADTMP(origpad[padix])) {
        newval = newSV(0);
        SvPADTMP_on(newval);
      }

https://metacpan.org/release/PEVANS/Object-Pad-0.64/source/hax/cv_copy_flags.c.inc#L125-128

This means that any role method that ends up with a pack-folded constant on a threaded perl accidentally ends up forgetting that constant, and has undef there instead:

$ perl -MObject::Pad
role R {
  method a { pack "C", 65 }
}

class C :does(R) {}
print "Capital A is ", C->new->a;

__END__
Use of uninitialized value in print at - line 6.
Capital A is 

Whereas on a non-thready perl we get:

Capital A is A

This issue also affects Future::AsyncAwait which also uses the same cv_copy_flags() function, which is where I first encountered it: https://rt.cpan.org/Ticket/Display.html?id=142468. (That one is erroneous for two reasons though; because we're copying a live-running CV, we actually should copy even the PADTMPs there, so that's a bug I need to fix in AsyncAwait.xs as well.)

[leonerd]

PS: I haven't checked yet; this issue might affect many other kinds of constant-folding, or it might be specific just to pack().

[tonycoz]

I expect it happens for anything constant folded where the OP returns TARG.

CvPADLIST() documents:

Iterating over the PADNAMELIST iterates over all possible pad
items.  Pad slots for targets (C<SVs_PADTMP>)
and GVs end up having &PL_padname_undef "names", while slots for constants 
have C<&PL_padname_const> "names" (see C<L</pad_alloc>>).  That
C<&PL_padname_undef>
and C<&PL_padname_const> are used is an implementation detail subject to
change.  To test for them, use C<!PadnamePV(name)> and
S<C<PadnamePV(name) && !PadnameLEN(name)>>, respectively.

Shouldn't you be checking if PadnamePV(name) && PadnameLEN(name) == 0 to detect constants (including folded constants) to copy?

For example (using a PAD dumper based on the above):

$ PERL_DESTRUCT_LEVEL=2 ./perl -Ilib -MDevel::Peek -E 'my $x = pack("C", 65); my $y = length($x) + 2; Devel::Peek::DumpMainPad(0xF)'
CV 0x621000001f50 PAD 0x621000001f68 PADLIST 0x604000000e68 NAMES 0x606000000c98 ARRAY 0x60c000002d58 MAX 10
  [1] my $x
    sv = 0x621000019c70
    value = "A"
  [2] PADTMP
    sv = NULL
  [3] my $y
    sv = 0x621000019bf8
    value = 3
  [4] NULL NAME
    sv = 0x621000019d48
    value = 1
  [5] NULL NAME
    sv = 0x621000019cb8
    value = 3
  [6] CONST
    sv = 0x621000034358
    value = GV(Devel::Peek::DumpMainPad)
  [7] NULL NAME
    sv = 0x621000019c10
    value = undef
  [8] CONST
    sv = 0x621000001e78
    value = "A"
  [9] CONST
    sv = 0x621000019bc8
    value = 2
  [10] CONST
    sv = 0x621000019c28
    value = 15
$ PERL_DESTRUCT_LEVEL=2 ./perl -Ilib -MO=Concise,-terse -MDevel::Peek -E 'my $x = pack("C", 65); my $y = length($x) + 2; Devel::Peek::DumpMainPad(0xF)'
LISTOP (0x61900003b510) leave [1] 
    OP (0x61900003b1c8) enter 
    COP (0x61900003b550) nextstate 
    UNOP (0x616000000cd8) padsv_store [1] 
        SVOP (0x616000000d48) const [8] PV (0x6210000a7eb8) "A" 
        OP (0x616000000db8) null [9] 
    COP (0x61900003b3c0) nextstate 
    UNOP (0x61900003b420) padsv_store [3] 
        BINOP (0x61900003b460) add [5] 
            UNOP (0x61900003b4d8) length [4] 
                OP (0x616000000d80) padsv [1] 
            SVOP (0x61900003b4a0) const [9] IV (0x6210000a7ea0) 2 
        OP (0x616000000d18) null [9] 
    COP (0x61900003b208) nextstate 
    UNOP (0x61900003b268) entersub [7] 
        UNOP (0x61900003b2a0) null [160] 
            OP (0x61900003b2e0) pushmark 
            SVOP (0x61900003b348) const [10] IV (0x6210000a7e70) 15 
            UNOP (0x61900003b310) null [17] 
                PADOP (0x61900003b380) gv  GV (0x6210000a8008) *Devel::Peek::DumpMainPad 
-e syntax OK

(-terse since it shows the numeric targ)

[richardleach]

Shouldn't you be checking if PadnamePV(name) && PadnameLEN(name) == 0 to detect constants (including folded constants) to copy?

@leonerd - was the above suggestion any use? Is there a bug in core here, or is this issue closeable?