Skip Menu |
 

This queue is for tickets about the Future-AsyncAwait CPAN distribution.

Report information
The Basics
Id: 122252
Status: open
Priority: 0/
Queue: Future-AsyncAwait

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

Bug Information
Severity: Wishlist
Broken in: (no value)
Fixed in: (no value)



Subject: Support perl versions prior to 5.24
Download (untitled) / with headers
text/plain 415b
Current implementation of the code requires 5.24+ for a variety of reasons, most notably the context stack rework that turned up around then. Ideally it would be nice to be able to support all the way to 5.14, when custom keywords were added. This may require some amount of compatibility shim around the context stack however. ((split out from https://rt.cpan.org/Ticket/Display.html?id=121569)) -- Paul Evans
Download (untitled) / with headers
text/plain 111b
Or, so as not to confuse RT's URL parser: https://rt.cpan.org/Ticket/Display.html?id=121569 -- Paul Evans
Download (untitled) / with headers
text/plain 473b
I now have an almost-working initial attempt on 5.22. Most things work, but one does not. The not-working test is the one about foreach loops, as it seems to have trouble saving and restoring the iterator variable which is part of the pre-5.24 context stack entries. There doesn't seem to be equivalent in the 5.24 reworked version of that for me to copy from, so it seems new things need adding. As yet I've not quite worked out how it all fits together. -- Paul Evans
Download (untitled) / with headers
text/plain 211b
On Mon Jan 15 12:26:54 2018, PEVANS wrote: Show quoted text
> I now have an almost-working initial attempt on 5.22. Most things > work, but one does not.
perl 5.22 now works as of -r121 I'm now looking at 5.20 -- Paul Evans
Future-AsyncAwait 0.12 now supports back as far as perl 5.20. -- Paul Evans
Download (untitled) / with headers
text/plain 624b
Latest development version now supports 5.18. Earlier than that, looking at 5.16. Making it compile isn't too hard - SAVEt_STRLEN needs to be #ifdef'ed out and a suitable SAVEt_INT_SMALL in replacement. The PADNAMEs structures need some compatibility macros. However, actually making it work and pass tests seems more difficult. There appears to be something odd about closures, that mean inner closures created after the first 'await' op don't work as expected, with captured lexical failing to stay shared. It may need some more research on how to make that work, but I think for now I'll stop at 5.18. -- Paul Evans
Download (untitled) / with headers
text/plain 1.4k
On Tue Jan 16 11:16:57 2018, PEVANS wrote: Show quoted text
> Latest development version now supports 5.18. > > Earlier than that, looking at 5.16. > > Making it compile isn't too hard - SAVEt_STRLEN needs to be #ifdef'ed > out and a suitable SAVEt_INT_SMALL in replacement. The PADNAMEs > structures need some compatibility macros. > > However, actually making it work and pass tests seems more difficult. > There appears to be something odd about closures, that mean inner > closures created after the first 'await' op don't work as expected, > with captured lexical failing to stay shared. It may need some more > research on how to make that work, but I think for now I'll stop at > 5.18.
I tried running ‘git log v5.16.0..v5.18.0 pad.c’ to jog my memory of what changed regarding closures, an area I have been involved in in recent years. It looks as though you may be running into old bugs that have since been fixed. See this commit, for instance: commit cae5dbbe30ba4a96ff5e570be0d90779f06fee71 Author: Father Chrysostomos <sprout@cpan.org> Date: Sat Aug 4 14:42:47 2012 -0700 Close over stale vars in active subs (I won’t repeat the whole commit message here. It’s long.) But most of the other bugs fixed back then had to do with formats closing over the wrong pad and crashing. And in the git log output there is a whole bunch of lexical sub stuff that you probably want to skip. I know I’m not being that helpful. But I thought I would at least take a look and report my meagre findings.
On Tue Jan 16 12:04:58 2018, SPROUT wrote: Show quoted text
> I tried running ‘git log v5.16.0..v5.18.0 pad.c’ to jog my memory of > what changed regarding closures, an area I have been involved in in > recent years. It looks as though you may be running into old bugs > that have since been fixed. See this commit, for instance: > > commit cae5dbbe30ba4a96ff5e570be0d90779f06fee71 > Author: Father Chrysostomos <sprout@cpan.org> > Date: Sat Aug 4 14:42:47 2012 -0700 > > Close over stale vars in active subs > > (I won’t repeat the whole commit message here. It’s long.)
For convenience the full commit is at https://perl5.git.perl.org/perl.git/commit/cae5dbbe30ba4a96ff5e570be0d90779f06fee71 The full description does indeed sound like it might be related though. Looking at the commit diff though, it suggests that if my test was affected by this bug I'd be seeing lots of Variable $foo is not available warnings, which I'm not. Still, an easy test might be to temporarily SvPADSTALE_off() all the PAD slots during cloning, just to artificially trick clone_sv() into keeping them. -- Paul Evans
Download (untitled) / with headers
text/plain 1.2k
Actually, most of the failures in 5.16 turn out to be fixed by bugfixes applied in F-AA 0.14, relating to the way that outer lexical captures are handled by `cv_clone()`. Now, running against 5.16 the only failure is t/10pad.t .............. 1/? # Failed test '$captured in subD' # at t/10pad.t line 118. # got: undef # expected: 'ABC' # Failed test '$fret now ready after done for inner subs' # at t/10pad.t line 137. # got: 'ABCE' # expected: 'ABCDE' # Looks like you failed 2 tests of 10. t/10pad.t .............. Dubious, test returned 2 (wstat 512, 0x200) Ignoring that momentarily and looking ahead to 5.14, it seems it won't compile because 5.14 doesn't provide a `cv_clone()`. That feels a potentially harder one to work around at 5.14. That said, given all the hackery F-AA 0.14 has had to add to work around the fact that `cv_clone()` doesn't do the right thing for the odd case that we're presenting to it here, it might be worth looking into whether a different solution can be found that implements a CV copying function that will work better for us, than calling cv_clone - the latter being designed to copy protosubs into new anonsubs, which isn't really what we're using it for. -- Paul Evans
Download (untitled) / with headers
text/plain 1.1k
On Mon Jan 22 10:39:43 2018, PEVANS wrote: Show quoted text
> Ignoring that momentarily and looking ahead to 5.14, it seems it won't > compile because 5.14 doesn't provide a `cv_clone()`. That feels a > potentially harder one to work around at 5.14. > > That said, given all the hackery F-AA 0.14 has had to add to work > around the fact that `cv_clone()` doesn't do the right thing for the > odd case that we're presenting to it here, it might be worth looking > into whether a different solution can be found that implements a CV > copying function that will work better for us, than calling cv_clone - > the latter being designed to copy protosubs into new anonsubs, which > isn't really what we're using it for.
With the new replacement of cv_clone() now in latest development commit, 5.16 looks exactly the same - still fails this test. I might have to deploy Devel::MAT at the moment $subD is created to inspect its pad and see what happened there. Looking at 5.14 the situation is *slightly* improved - fails to compile still due to lack of `pad_new()` but as the pad structure on 5.14 was all bare AVs, this should be an easy one to provide by hax. -- Paul Evans
Download (untitled) / with headers
text/plain 1004b
On Tue Jan 23 21:46:58 2018, PEVANS wrote: Show quoted text
> With the new replacement of cv_clone() now in latest development > commit, 5.16 looks exactly the same - still fails this test. I might > have to deploy Devel::MAT at the moment $subD is created to inspect > its pad and see what happened there.
As I suspected; `$captured` has captured the wrong thing. On t/10pad.t line 122 after $subD has been created, it already looks like: [ 4/$captured]=SCALAR(PV) at 0x1e771c8 = "ABC" ... [ 9/$subD ]=REF() at 0x1e77090 => CODE(PP,C) at 0x1e3e0b8 Show quoted text
pmat> show 0x1e3e0b8
CODE(PP,C) at 0x1e3e0b8 with refcount 1 ... pad[0]=PAD(4) at 0x1e3e3e8 Show quoted text
pmat> show 0x1e3e3e8
PAD(4) at 0x1e3e3e8 with refcount 1 ... [ 1/$captured]=NULL So it appears that the 5.16 implementation of cv_clone() here that's called as part of the OP_REFGEN op in the optree to implement my $subD = sub { ... }; has found the wrong `$captured` lexical. I wonder if we can patch this up somehow. -- Paul Evans
Download (untitled) / with headers
text/plain 267b
On Tue Jan 23 22:13:15 2018, PEVANS wrote: Show quoted text
> I wonder if we can patch this up somehow.
Turns out in summary: yes we can. Latest devel (will become 0.15) supports 5.16. All that remains now is 5.14, and that has /lots/ of test failures right now :( -- Paul Evans


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.