Skip Menu |

This queue is for tickets about the Object-Pad CPAN distribution.

Report information
The Basics
Id: 132178
Status: resolved
Priority: 0/
Queue: Object-Pad

People
Owner: Nobody in particular
Requestors: leonerd-cpan [...] leonerd.org.uk
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: 0.15
Fixed in: 0.24



Subject: Anon methods generated inside foreach loops break closures
Captured lexicals inside an anon `method` seem to get broken: class Generated { foreach my $letter (qw( x y z )) { my $code = method { return uc $letter; }; no strict 'refs'; *$letter = $code; } } my $g = Generated->new; is( $g->x, "X", 'generated anon method' ); Yields Use of uninitialized value $letter in uc at t/31pad-outside.t line 36. # Failed test 'generated anon method' # at t/31pad-outside.t line 45. # got: '' # expected: 'X' The same test with s/method/sub/ passes fine. -- Paul Evans
Some initial investigation with Devel::MAT suggests it might be related to how the lexical is captured, or rather isn't. We've just got the same SV in all generated methods: Show quoted text
pmat> show &Generated::x
CODE(PP,C) at 0x55bbbd917b28 with refcount 1 size 128 bytes named as &Generated::x no hekname stash=STASH(12) at 0x55bbbd8d9798 glob=GLOB(&*) at 0x55bbbd8c79c8 location=t/31pad-outside.t no scope no padlist no padnames_av pad[0]=PAD(5) at 0x55bbbd870fd0 Show quoted text
pmat> show 0x55bbbd870fd0
PAD(5) at 0x55bbbd870fd0 with refcount 1 size 104 bytes padcv=CODE(PP,C) at 0x55bbbd917b28 [0/@_ ]=ARRAY(0,!REAL) at 0x55bbbd8f27d0 [1/$self ]=REF() at 0x55bbbd925a58 => ARRAY(0)=Generated at 0x55bbbd925998 [2/@(Object::Pad/slots)]=ARRAY(0)=Generated at 0x55bbbd925998 [3/$letter ]=UNDEF() at 0x55bbbd189100 [4 ]=SCALAR(PV) at 0x55bbbd9256e0 Show quoted text
pmat> identify 0x55bbbd189100
UNDEF() at 0x55bbbd189100 is: ├─the lexical $allcount at depth 1 of CODE() at 0x55bbbd1666e0=main_cv, which is: │ └─the main code ├─the lexical $letter at depth 1 of CODE(PP,C) at 0x55bbbd917b28, which is: │ └─the symbol '&Generated::x' ├─the lexical $letter at depth 1 of CODE(PP,C) at 0x55bbbd925728, which is: │ └─the symbol '&Generated::z' └─the lexical $letter at depth 1 of CODE(PP,C) at 0x55bbbd925758, which is: └─the symbol '&Generated::y' This might require some investigation around the CvOUTSIDE scope hackery used to provide the slot lexicals when compiling a method. -- Paul Evans
Failure seems to depend on whether the previous test is visible or commented-out in the file above it. Without the other test there, this one passes: Show quoted text
pmat> show &Generated::x
CODE(PP,C) at 0x560df4a1d240 with refcount 1 size 128 bytes named as &Generated::x no hekname stash=STASH(12) at 0x560df515afc8 glob=GLOB(&*) at 0x560df5174ec8 location=t/31pad-outside.t no scope no padlist no padnames_av pad[0]=PAD(5) at 0x560df49fa470 Show quoted text
pmat> show 0x560df49fa470
PAD(5) at 0x560df49fa470 with refcount 1 size 104 bytes padcv=CODE(PP,C) at 0x560df4a1d240 [0/@_ ]=ARRAY(0,!REAL) at 0x560df4a1d180 [1/$self ]=REF() at 0x560df4a1d258 => ARRAY(0)=Generated at 0x560df51b9548 [2/@(Object::Pad/slots)]=ARRAY(0)=Generated at 0x560df51b9548 [3/$letter ]=SCALAR(PV) at 0x560df515b820 = "x" [4 ]=SCALAR(PV) at 0x560df4a1d1b0 Show quoted text
pmat> identify 0x560df515b820
SCALAR(PV) at 0x560df515b820 is: ├─a constant of CODE() at 0x560df49fa6e0=main_cv, which is: │ └─the main code └─the lexical $letter at depth 1 of CODE(PP,C) at 0x560df4a1d240, which is: └─the symbol '&Generated::x' with similarly correct-looking values for $letter in the other two generated methods. -- Paul Evans
Another more detailed test suggests this has nothing to do with the multiple instances being present. Even a foreach loop over a single value is enough to upset it: class Generated { foreach my $letter (qw( x )) { printf STDERR "\$letter is %#x\n", \$letter; Devel::MAT::Dumper::dump( "prior-method.pmat" ); my $code = method { return uc $letter; }; printf STDERR "\$code is %#x\n", $code; Devel::MAT::Dumper::dump( "subseq-method.pmat" ); no strict 'refs'; *$letter = $code; } } Output: $letter is 0x55dcb0bd7688 TODO: savestack type=10 $code is 0x55dcb047d1d0 TODO: savestack type=10 (ignore the TODO lines; they are Devel::MAT::Dumper complaining about an unrecognised SAVEt_FREEPV) Analysing these in the `subseq-method.pmat` file shows us: $ pmat subseq-method.pmat Perl memory dumpfile from perl 5.30.0 threaded Heap contains 23573 objects Show quoted text
pmat> show 0x55dcb0bd7688
SCALAR(PV) at 0x55dcb0bd7688 with refcount 2 size 50 bytes PV="x" PVLEN 1 Show quoted text
pmat> identify 0x55dcb0bd7688
SCALAR(PV) at 0x55dcb0bd7688 is: ├─a constant of CODE() at 0x55dcb045a6c8=main_cv, which is: │ └─the main code └─the lexical $letter at depth 1 of CODE() at 0x55dcb045a6c8=main_cv, which is: └─the main code (as anticipated) Show quoted text
pmat> show 0x55dcb047d1d0
CODE(PP,C) at 0x55dcb047d1d0 with refcount 1 size 128 bytes named as &Generated::__ANON__ no hekname stash=STASH(6) at 0x55dcb0ae8510 glob=GLOB(*) at 0x55dcb0b37a40 location=t/31pad-outside.t no scope no padlist no padnames_av pad[0]=PAD(5) at 0x55dcb0ae7fb8 Show quoted text
pmat> show 0x55dcb0ae7fb8
PAD(5) at 0x55dcb0ae7fb8 with refcount 1 size 104 bytes padcv=CODE(PP,C) at 0x55dcb047d1d0 [0/@_ ]=ARRAY(0,!REAL) at 0x55dcb0a841d8 [1/$self ]=UNDEF() at 0x55dcb047d1e8 [2/@(Object::Pad/slots)]=ARRAY(0) at 0x55dcb047d290 [3/$letter ]=UNDEF() at 0x55dcb047d278 [4 ]=UNDEF() at 0x55dcb045a470 The `$letter` lexical in this method has been detached from its scope and recreated independently. If instead we write the code as `my $code = sub { ... }` instead of method, then perl correctly captures it as expected: Show quoted text
pmat> show 0x558c82007b58
SCALAR(PV) at 0x558c82007b58 with refcount 3 size 50 bytes PV="x" PVLEN 1 Show quoted text
pmat> identify 0x558c82007b58
SCALAR(PV) at 0x558c82007b58 is: ├─a constant of CODE() at 0x558c8188e6c8=main_cv, which is: │ └─the main code ├─the lexical $letter at depth 1 of CODE() at 0x558c8188e6c8=main_cv, which is: │ └─the main code └─the lexical $letter at depth 1 of CODE(PP,C) at 0x558c818b11d0, which is: └─(via RV) the lexical $code at depth 1 of CODE() at 0x558c8188e6c8=main_cv, which is: └─the main code It's already here in the `identify` output as being a captured lexical of the anon sub found in $code. -- Paul Evans
A probably-significant difference is that in the (working) `sub` case, the closure's protosub still has a CvOUTSIDE: Show quoted text
pmat> show 0x56426e75a1d0
CODE(PP,closure) at 0x56426e75a1d0 with refcount 1 ... protosub=CODE(PP,proto) at 0x56426ee16168 Show quoted text
pmat> show 0x56426ee16168
CODE(PP,proto) at 0x56426ee16168 with refcount 1 ... scope=CODE() at 0x56426e7376c8=main_cv Whereas the non-working `method` one does not: Show quoted text
pmat> show 0x563e600291d0
CODE(PP,closure) at 0x563e600291d0 with refcount 1 ... protosub=CODE(PP,proto) at 0x563e6077eb40 Show quoted text
pmat> show 0x563e6077eb40
CODE(PP,proto) at 0x563e6077eb40 with refcount 1 ... no scope -- Paul Evans
Both the protosub and the closure mark the padname with the PadnameOUTSIDE flag, so that isn't it... Show quoted text
pmat> show --pad 0x55a499ade0a0
CODE(PP,proto) at 0x55a499ade0a0 with refcount 1 ... [3/$letter *OUTER ]=NULL Show quoted text
pmat> show --pad 0x55a49938a1d0
CODE(PP,closure) at 0x55a49938a1d0 with refcount 1 ... [3/$letter *OUTER ]=UNDEF() at 0x55a49938a278 I think I'll have to go code-diving into perl's cv_clone() and watch why it has decided to break this SV. -- Paul Evans
Progress: For a PadnameOUTER() name, cv_clone() will look in PARENT_PAD_INDEX(pn) to find the index in the parent's pad (i.e. CvPADLIST of CvOUTSIDE) where the variable lives. This avoids a runtime name lookup: 1986 if (PadnameOUTER(namesv)) { /* lexical from outside? */ 1987 /* formats may have an inactive, or even undefined, parent; 1988 but state vars are always available. */ 1989 if (!outpad || !(sv = outpad[PARENT_PAD_INDEX(namesv)]) That PARENT_PAD_INDEX is stored in the abused ->xpadn_low field which isn't meaningful for OUTER lexicals. padname_dump() doesn't know this so helpfully prints it anyway; in the non-working method case: 3: $letter *OUTER [1..0] = 0x55de688061e0 and in the working sub case: 1: $letter *OUTER [10..0] = 0x55de68fbc178 A different index. Sure enough, padix 10 in the outside pad does indeed contain $letter: [10/$letter ]=SCALAR(PV) at 0x55de68fbc178 = "x" I suspect therefore that the pre_blockend hook of method parsing is going to have to patch up these indexes to account for the respliced CvOUTSIDE hacks. -- Paul Evans
On Fri Apr 24 09:20:51 2020, PEVANS wrote: Show quoted text
> I suspect therefore that the pre_blockend hook of method parsing is > going to have to patch up these indexes to account for the respliced > CvOUTSIDE hacks.
Which is what I'm now doing and this seems to fix it. Patch attached. -- Paul Evans
Subject: rt132178.patch
=== modified file 'lib/Object/Pad.xs' --- old/lib/Object/Pad.xs 2020-04-23 19:06:27 +0000 +++ new/lib/Object/Pad.xs 2020-04-24 13:48:55 +0000 @@ -1233,6 +1233,32 @@ ctx->body = op_append_list(OP_LINESEQ, slotops, ctx->body); compclassmeta->methodscope = NULL; + + /* Restore CvOUTSIDE(PL_compcv) back to where it should be */ + { + CV *outside = CvOUTSIDE(PL_compcv); + PADNAMELIST *pnl = PadlistNAMES(CvPADLIST(PL_compcv)); + PADNAMELIST *outside_pnl = PadlistNAMES(CvPADLIST(outside)); + + /* Lexical captures will need their parent pad index fixing + * Technically these only matter for CvANON because they're only used when + * reconstructing the parent pad captures by OP_ANONCODE. But we might as + * well be polite and fix them for all CVs + */ + PADOFFSET padix; + for(padix = 1; padix <= PadnamelistMAX(pnl); padix++) { + PADNAME *pn = PadnamelistARRAY(pnl)[padix]; + if(!pn || !PadnameOUTER(pn)) + continue; + + PADNAME *outside_pn = PadnamelistARRAY(outside_pnl)[PARENT_PAD_INDEX(pn)]; + + PARENT_PAD_INDEX(pn) = PARENT_PAD_INDEX(outside_pn); + } + + CvOUTSIDE(PL_compcv) = CvOUTSIDE(outside); + CvOUTSIDE_SEQ(PL_compcv) = CvOUTSIDE_SEQ(outside); + } } static void parse_post_newcv(pTHX_ struct XSParseSublikeContext *ctx) === modified file 't/31pad-outside.t' --- old/t/31pad-outside.t 2020-03-18 18:15:21 +0000 +++ new/t/31pad-outside.t 2020-04-24 13:48:55 +0000 @@ -7,17 +7,17 @@ use Object::Pad; -class Counter { - has $count; - my $allcount = 0; - - method inc { $count++; $allcount++ } - - method count { $count } - sub allcount { $allcount } -} - { + class Counter { + has $count; + my $allcount = 0; + + method inc { $count++; $allcount++ } + + method count { $count } + sub allcount { $allcount } + } + my $countA = Counter->new; my $countB = Counter->new; @@ -28,4 +28,25 @@ is( Counter->allcount, 2, 'Counter->allcount' ); } +# anon methods can capture lexicals (RT132178) +{ + use Devel::MAT::Dumper; + + class Generated { + foreach my $letter (qw( x y z )) { + my $code = method { + return uc $letter; + }; + + no strict 'refs'; + *$letter = $code; + } + } + + my $g = Generated->new; + is( $g->x, "X", 'generated anon method' ); + is( $g->y, "Y", 'generated anon method' ); + is( $g->z, "Z", 'generated anon method' ); +} + done_testing;