caller behaving unexpectedly in re-evals

p5pRT commented 12 years ago

Migrated from rt.perl.org#113928 (status was 'resolved')

Searchable as RT113928$

p5pRT commented 12 years ago

From @cpansprout

If code blocks in regular expressions are supposed to be analogous to sort blocks\, then I could expect these to return the same thing:

$ ./perl -Ilib -le '$\,=" "; sub foo { @_ = sort {print caller 0 } 1..2 } foo()' main -e 1 main::foo 1 256
$ ./perl -Ilib -le '$\,=" "; sub foo { /(?{ print caller 0 })/ } foo()' main -e 1 main::foo 256

The only difference here is the 1 after the sub name ($has_args).

I get yet another set of results with a qr// thing:

$ ./perl -Ilib -le '$\,=" "; sub foo { ""=~qr/(?{ print caller 0 })/ } foo()' main -e 1 main::__ANON__ 256

If I change caller 0 to caller 1\, I get this:

$ ./perl -Ilib -le '$\,=" "; sub foo { @_ = sort {print caller 1 } 1..2 } foo()'

$ ./perl -Ilib -le '$\,=" "; sub foo { /(?{ print caller 1 })/ } foo()' main -e 1 main::foo 1 256
$ ./perl -Ilib -le '$\,=" "; sub foo { ""=~qr/(?{ print caller 1 })/ } foo()' main -e 1 main::foo 1 256

That a regexp code block inside foo should appear like a &foo call is weird. That it should show main::foo\, while qr shows main::__ANON__\, just seems inconsistent.

The implementation is leaking through. I think the regexp blocks should simply not be counted as call frames. I know how to fix this. I just wanted to check first.

On a related note:

$ ./perl -Ilib -e 'sub foo { ""=~qr/(?{CORE::__SUB__->()})/ } foo' Bus error

That was how I discovered this.

Flags: category=core severity=low

Site configuration information for perl 5.17.2:

Configured by sprout at Mon Jun 25 13:28:03 PDT 2012.

Summary of my perl5 (revision 5 version 17 subversion 2) configuration: Snapshot of: 256aa5e6f36285b09b3ba67ad9926c9d4f9ea7bc Platform: osname=darwin\, osvers=10.5.0\, archname=darwin-2level uname='darwin pint.local 10.5.0 darwin kernel version 10.5.0: fri nov 5 23:20:39 pdt 2010; root:xnu-1504.9.17~1release_i386 i386 ' config_args='-de -DDEBUGGING -Dusedevel' hint=recommended\, useposix=true\, d_sigaction=define useithreads=undef\, usemultiplicity=undef useperlio=define\, d_sfio=undef\, uselargefiles=define\, usesocks=undef use64bitint=undef\, use64bitall=undef\, uselongdouble=undef usemymalloc=n\, bincompat5005=undef Compiler: cc='cc'\, ccflags ='-fno-common -DPERL_DARWIN -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'\, optimize='-O3 -g'\, cppflags='-fno-common -DPERL_DARWIN -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include' ccversion=''\, gccversion='4.2.1 (Apple Inc. build 5664)'\, gccosandvers='' intsize=4\, longsize=4\, ptrsize=4\, doublesize=8\, byteorder=1234 d_longlong=define\, longlongsize=8\, d_longdbl=define\, longdblsize=16 ivtype='long'\, ivsize=4\, nvtype='double'\, nvsize=8\, Off_t='off_t'\, lseeksize=8 alignbytes=8\, prototype=define Linker and Libraries: ld='env MACOSX_DEPLOYMENT_TARGET=10.3 cc'\, ldflags =' -fstack-protector -L/usr/local/lib' libpth=/usr/local/lib /usr/lib libs=-ldbm -ldl -lm -lutil -lc perllibs=-ldl -lm -lutil -lc libc=\, so=dylib\, useshrplib=false\, libperl=libperl.a gnulibc_version='' Dynamic Linking: dlsrc=dl_dlopen.xs\, dlext=bundle\, d_dlsymun=undef\, ccdlflags=' ' cccdlflags=' '\, lddlflags=' -bundle -undefined dynamic_lookup -L/usr/local/lib -fstack-protector'

Locally applied patches:

@INC for perl 5.17.2: /usr/local/lib/perl5/site_perl/5.17.2/darwin-2level /usr/local/lib/perl5/site_perl/5.17.2 /usr/local/lib/perl5/5.17.2/darwin-2level /usr/local/lib/perl5/5.17.2 /usr/local/lib/perl5/site_perl .

Environment for perl 5.17.2: DYLD_LIBRARY_PATH (unset) HOME=/Users/sprout LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/X11/bin:/usr/local/bin PERL_BADLANG (unset) SHELL=/bin/bash

p5pRT commented 12 years ago

From @iabyn

On Sun\, Jul 01\, 2012 at 01:55:31PM -0700\, Father Chrysostomos wrote:

# New Ticket Created by Father Chrysostomos # Please include the string: [perl #113928] # in the subject line of all future correspondence about this issue. # \<URL: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=113928 >

If code blocks in regular expressions are supposed to be analogous to sort blocks\, then I could expect these to return the same thing:

$ ./perl -Ilib -le '$\,=" "; sub foo { @_ = sort {print caller 0 } 1..2 } foo()' main -e 1 main::foo 1 256
$ ./perl -Ilib -le '$\,=" "; sub foo { /(?{ print caller 0 })/ } foo()' main -e 1 main::foo 256

The only difference here is the 1 after the sub name ($has_args).

In this case\, I think its sort in error. Sort doesn't set @_\, so has_args shouldn't be 1.

Ideally I'd like to see sort updated to use the MULTICALL interface (which was itself originally inspired by the sort implementation)\, but I don't know whether sort has enough quirks to make this infeasible.

I get yet another set of results with a qr// thing:

$ ./perl -Ilib -le '$\,=" "; sub foo { ""=~qr/(?{ print caller 0 })/ } foo()' main -e 1 main::__ANON__ 256

If I change caller 0 to caller 1\, I get this:

$ ./perl -Ilib -le '$\,=" "; sub foo { @_ = sort {print caller 1 } 1..2 } foo()'

$ ./perl -Ilib -le '$\,=" "; sub foo { /(?{ print caller 1 })/ } foo()' main -e 1 main::foo 1 256
$ ./perl -Ilib -le '$\,=" "; sub foo { ""=~qr/(?{ print caller 1 })/ } foo()' main -e 1 main::foo 1 256

That a regexp code block inside foo should appear like a &foo call is weird. That it should show main::foo\, while qr shows main::__ANON__\, just seems inconsistent.

The implementation is leaking through. I think the regexp blocks should simply not be counted as call frames. I know how to fix this. I just wanted to check first.

The __ANON__ is actually the bit that is most reasonable\, while the non-qr case is the weird one.

Basically\, from a scope/closure point of view\,

/A(?{$x})B/ is supposed to be like /A/ && do {$x} && /B/\,

while

qr/A(?{$x})B/ is supposed to be like sub { /A/ && do {$x} && /B/ }

The first one ends up as a bit of hack\, because to run the do{ $x }\, code\, I push the current sub onto the context stack again\, but at the same depth. Although thinking about it now\, I wonder whether for that case I could just skip pushing a new context at all?

For the qr// case\, I think exposing the __ANON__ to caller() is correct. Otherwise\, you get this:

1: sub gen { 2: qr/(?{ print caller 0; warn ...})/; 3: } 4: sub foo { 5: my $pat = gen(...); 6: /$pat/ 7: }

should the warning appear to come from line 2 or line 6? I think 2. Similarly for the caller.

-- That he said that that that that is is is debatable\, is debatable.

p5pRT commented 12 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 12 years ago

From @iabyn

On Fri\, Jul 06\, 2012 at 10:51:46AM +0100\, Dave Mitchell wrote:

Basically\, from a scope/closure point of view\,
/A$?\{$x\}$B/ is supposed to be like /A/ && do \{$x\} && /B/\,
while
qr/A$?\{$x\}$B/ is supposed to be like sub \{ /A/ && do \{$x\} && /B/ \}
The first one ends up as a bit of hack\, because to run the do{ $x }\, code\, I push the current sub onto the context stack again\, but at the same depth. Although thinking about it now\, I wonder whether for that case I could just skip pushing a new context at all?

Scrub that: we've pushed a complete new stackinfo\, so have an empty context stack\, so the current sub *does* need pushing again.

-- Justice is when you get what you deserve. Law is when you get what you pay for.

p5pRT commented 12 years ago

From @cpansprout

On Fri Jul 06 03:22:02 2012\, davem wrote:

On Fri\, Jul 06\, 2012 at 10:51:46AM +0100\, Dave Mitchell wrote:
Basically\, from a scope/closure point of view\,
/A$?\{$x\}$B/ is supposed to be like /A/ && do \{$x\} && /B/\,
while
qr/A$?\{$x\}$B/ is supposed to be like sub \{ /A/ && do \{$x\} && /B/ \}
The first one ends up as a bit of hack\, because to run the do{ $x }\, code\, I push the current sub onto the context stack again\, but at the same depth. Although thinking about it now\, I wonder whether for that case I could just skip pushing a new context at all?
Scrub that: we've pushed a complete new stackinfo\, so have an empty context stack\, so the current sub *does* need pushing again.

What I’m trying to say is the fact that we are pushing a new context is an implementation detail. If I have only called a sub once and I can see it through caller 0 and caller 1\, then something is wrong. If we do uncanny things with the context stack\, we should hide that from Perl-level things like caller and __SUB__. sort is behaving exactly as I would expect\, and exactly as it has behaved for years. So I really think we should follow it\, at least in terms of how caller behaves\, even if it is implemented differently underneath.

--

Father Chrysostomos

p5pRT commented 12 years ago

From [Unknown Contact. See original ticket]

On Fri Jul 06 03:22:02 2012\, davem wrote:

On Fri\, Jul 06\, 2012 at 10:51:46AM +0100\, Dave Mitchell wrote:
Basically\, from a scope/closure point of view\,
/A$?\{$x\}$B/ is supposed to be like /A/ && do \{$x\} && /B/\,
while
qr/A$?\{$x\}$B/ is supposed to be like sub \{ /A/ && do \{$x\} && /B/ \}
The first one ends up as a bit of hack\, because to run the do{ $x }\, code\, I push the current sub onto the context stack again\, but at the same depth. Although thinking about it now\, I wonder whether for that case I could just skip pushing a new context at all?
Scrub that: we've pushed a complete new stackinfo\, so have an empty context stack\, so the current sub *does* need pushing again.

What I’m trying to say is the fact that we are pushing a new context is an implementation detail. If I have only called a sub once and I can see it through caller 0 and caller 1\, then something is wrong. If we do uncanny things with the context stack\, we should hide that from Perl-level things like caller and __SUB__. sort is behaving exactly as I would expect\, and exactly as it has behaved for years. So I really think we should follow it\, at least in terms of how caller behaves\, even if it is implemented differently underneath.

--

Father Chrysostomos

p5pRT commented 12 years ago

From @iabyn

On Fri\, Jul 06\, 2012 at 04:29:41AM -0700\, Father Chrysostomos via RT wrote:

On Fri Jul 06 03:22:02 2012\, davem wrote:
On Fri\, Jul 06\, 2012 at 10:51:46AM +0100\, Dave Mitchell wrote:
Basically\, from a scope/closure point of view\,
/A$?\{$x\}$B/ is supposed to be like /A/ && do \{$x\} && /B/\,
while
qr/A$?\{$x\}$B/ is supposed to be like sub \{ /A/ && do \{$x\} && /B/ \}
The first one ends up as a bit of hack\, because to run the do{ $x }\, code\, I push the current sub onto the context stack again\, but at the same depth. Although thinking about it now\, I wonder whether for that case I could just skip pushing a new context at all?
Scrub that: we've pushed a complete new stackinfo\, so have an empty context stack\, so the current sub *does* need pushing again.
What I’m trying to say is the fact that we are pushing a new context is an implementation detail. If I have only called a sub once and I can see it through caller 0 and caller 1\, then something is wrong. If we do uncanny things with the context stack\, we should hide that from Perl-level things like caller and __SUB__. sort is behaving exactly as I would expect\, and exactly as it has behaved for years. So I really think we should follow it\, at least in terms of how caller behaves\, even if it is implemented differently underneath.

Ok\, I think I agree that the second sub CX stack entry should be hidden for //; however\, I think the anon sub should still be shown for qr//.

-- This email is confidential\, and now that you have read it you are legally obliged to shoot yourself. Or shoot a lawyer\, if you prefer. If you have received this email in error\, place it in its original wrapping and return for a full refund. By opening this email\, you accept that Elvis lives.

p5pRT commented 12 years ago

From @cpansprout

On Fri Jul 06 08:48:01 2012\, davem wrote:

On Fri\, Jul 06\, 2012 at 04:29:41AM -0700\, Father Chrysostomos via RT wrote:

What I’m trying to say is the fact that we are pushing a new context is an implementation detail. If I have only called a sub once and I can see it through caller 0 and caller 1\, then something is wrong. If we do uncanny things with the context stack\, we should hide that from Perl-level things like caller and __SUB__. sort is behaving exactly as I would expect\, and exactly as it has behaved for years. So I really think we should follow it\, at least in terms of how caller behaves\, even if it is implemented differently underneath.

Ok\, I think I agree that the second sub CX stack entry should be hidden for //; however\, I think the anon sub should still be shown for qr//.

What should __SUB__ return inside qr//?

--

Father Chrysostomos

p5pRT commented 12 years ago

From [Unknown Contact. See original ticket]

On Fri Jul 06 08:48:01 2012\, davem wrote:

On Fri\, Jul 06\, 2012 at 04:29:41AM -0700\, Father Chrysostomos via RT wrote:

What I’m trying to say is the fact that we are pushing a new context is an implementation detail. If I have only called a sub once and I can see it through caller 0 and caller 1\, then something is wrong. If we do uncanny things with the context stack\, we should hide that from Perl-level things like caller and __SUB__. sort is behaving exactly as I would expect\, and exactly as it has behaved for years. So I really think we should follow it\, at least in terms of how caller behaves\, even if it is implemented differently underneath.

Ok\, I think I agree that the second sub CX stack entry should be hidden for //; however\, I think the anon sub should still be shown for qr//.

What should __SUB__ return inside qr//?

--

Father Chrysostomos

p5pRT commented 12 years ago

From @iabyn

On Fri\, Jul 06\, 2012 at 09:22:15AM -0700\, Father Chrysostomos via RT wrote:

On Fri Jul 06 08:48:01 2012\, davem wrote:

On Fri\, Jul 06\, 2012 at 04:29:41AM -0700\, Father Chrysostomos via RT wrote:

What I’m trying to say is the fact that we are pushing a new context is an implementation detail. If I have only called a sub once and I can see it through caller 0 and caller 1\, then something is wrong. If we do uncanny things with the context stack\, we should hide that from Perl-level things like caller and __SUB__. sort is behaving exactly as I would expect\, and exactly as it has behaved for years. So I really think we should follow it\, at least in terms of how caller behaves\, even if it is implemented differently underneath.

Ok\, I think I agree that the second sub CX stack entry should be hidden for //; however\, I think the anon sub should still be shown for qr//.

What should __SUB__ return inside qr//?

My gut reaction is the anon sub currently being executed\, but I'm willing to be persuaded otherwise.

i.e in

use feature 'current_sub'; sub f { my @s; my $r = qr/(?{ push @s\, __SUB__ })/; my $code = '(?{ push @s\, __SUB__ })'; use re 'eval'; /(?{ push @s\, __SUB__ })$r$code/; }

I would expect @s to contain \&f\, plus two different anon subs. (Although the fact that $s[2] is an anon sub sounds very much like an implementation detail\, and might change).

-- That he said that that that that is is is debatable\, is debatable.

p5pRT commented 12 years ago

From @cpansprout

On Sun Jul 08 05:00:46 2012\, davem wrote:

On Fri\, Jul 06\, 2012 at 09:22:15AM -0700\, Father Chrysostomos via RT wrote:

On Fri Jul 06 08:48:01 2012\, davem wrote:

On Fri\, Jul 06\, 2012 at 04:29:41AM -0700\, Father Chrysostomos via RT wrote:

What I’m trying to say is the fact that we are pushing a new context is an implementation detail. If I have only called a sub once and I can see it through caller 0 and caller 1\, then something is wrong. If we do uncanny things with the context stack\, we should hide that from Perl-level things like caller and __SUB__. sort is behaving exactly as I would expect\, and exactly as it has behaved for years. So I really think we should follow it\, at least in terms of how caller behaves\, even if it is implemented differently underneath.

Ok\, I think I agree that the second sub CX stack entry should be hidden for //; however\, I think the anon sub should still be shown for qr//.

What should __SUB__ return inside qr//?

My gut reaction is the anon sub currently being executed\, but I'm willing to be persuaded otherwise.

i.e in
use feature 'current\_sub';
sub f \{ 
my @&#8203;s;
my $r = qr/$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$/;
my $code = '$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$';
use re 'eval';
/$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$$r$code/;
\}
I would expect @s to contain \&f\, plus two different anon subs. (Although the fact that $s[2] is an anon sub sounds very much like an implementation detail\, and might change).

And calling it crashes. So we really shouldn’t be exposing it.

$ ./perl -Ilib -e '"" =~ qr/(?{ CORE::__SUB__->() })/' Bus error

$ gdb --args ./perl -Ilib -e '"" =~ qr/(?{ CORE::__SUB__->() })/' GNU gdb 6.3.50-20050815 (Apple version gdb-1469) (Wed May 5 04:30:06 UTC 2010) Copyright 2004 Free Software Foundation\, Inc. GDB is free software\, covered by the GNU General Public License\, and you are welcome to change it and/or distribute copies of it under certain conditions. Type "show copying" to see the conditions. There is absolutely no warranty for GDB. Type "show warranty" for details. This GDB was configured as "i386-apple-darwin"...Reading symbols for shared libraries ... done

(gdb) run Starting program: /Users/sprout/Perl/perl.git/perl -Ilib -e \"\"\ =\~\ qr/$\?\{\ CORE::__SUB__-\>\($\ \}\)/ Reading symbols for shared libraries ++.. done

Program received signal EXC_BAD_ACCESS\, Could not access memory. Reason: KERN_PROTECTION_FAILURE at address: 0x00000000 0x0008e4a5 in Perl_reg_temp_copy (my_perl=0x800000\, ret_x=0x0\, rx=0x0) at regcomp.c:13410 warning: Source file is more recent than executable. 13410 struct regexp *const r = (struct regexp *)SvANY(rx); (gdb) bt #0 0x0008e4a5 in Perl_reg_temp_copy (my_perl=0x800000\, ret_x=0x0\, rx=0x0) at regcomp.c:13410 #1 0x000ff700 in Perl_pp_qr (my_perl=0x800000) at pp_hot.c:1225 #2 0x000bf0b9 in Perl_runops_debug (my_perl=0x800000) at dump.c:2133 #3 0x001ca685 in S_regtry (my_perl=0x800000\, reginfo=0xbffff598\, startpos=0xbffff5cc) at regexec.c:4405 #4 0x001e0378 in Perl_regexec_flags (my_perl=0x800000\, rx=0x804620\, stringarg=0x3085f0 ""\, strend=0x3085f0 ""\, strbeg=0x3085f0 ""\, minend=\<value temporarily unavailable\, due to optimizations>\, sv=0x825b80\, data=0x0\, flags=\<value temporarily unavailable\, due to optimizations>) at regexec.c:2528 #5 0x000fe65e in Perl_pp_match (my_perl=0x800000) at pp_hot.c:1378 #6 0x000bf0b9 in Perl_runops_debug (my_perl=0x800000) at dump.c:2133 #7 0x00030aa7 in S_run_body [inlined] () at /Users/sprout/Perl/perl.git/perl.c:2414 #8 0x00030aa7 in perl_run (my_perl=0x800000) at perl.c:2332 #9 0x00001a7c in main (argc=4\, argv=0xbffff830\, env=0xbffff844) at perlmain.c:120 (gdb)

--

Father Chrysostomos

p5pRT commented 12 years ago

From [Unknown Contact. See original ticket]

On Sun Jul 08 05:00:46 2012\, davem wrote:

On Fri\, Jul 06\, 2012 at 09:22:15AM -0700\, Father Chrysostomos via RT wrote:

On Fri Jul 06 08:48:01 2012\, davem wrote:

On Fri\, Jul 06\, 2012 at 04:29:41AM -0700\, Father Chrysostomos via RT wrote:

What I’m trying to say is the fact that we are pushing a new context is an implementation detail. If I have only called a sub once and I can see it through caller 0 and caller 1\, then something is wrong. If we do uncanny things with the context stack\, we should hide that from Perl-level things like caller and __SUB__. sort is behaving exactly as I would expect\, and exactly as it has behaved for years. So I really think we should follow it\, at least in terms of how caller behaves\, even if it is implemented differently underneath.

Ok\, I think I agree that the second sub CX stack entry should be hidden for //; however\, I think the anon sub should still be shown for qr//.

What should __SUB__ return inside qr//?

My gut reaction is the anon sub currently being executed\, but I'm willing to be persuaded otherwise.

i.e in
use feature 'current\_sub';
sub f \{ 
my @&#8203;s;
my $r = qr/$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$/;
my $code = '$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$';
use re 'eval';
/$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$$r$code/;
\}
I would expect @s to contain \&f\, plus two different anon subs. (Although the fact that $s[2] is an anon sub sounds very much like an implementation detail\, and might change).

And calling it crashes. So we really shouldn’t be exposing it.

$ ./perl -Ilib -e '"" =~ qr/(?{ CORE::__SUB__->() })/' Bus error

$ gdb --args ./perl -Ilib -e '"" =~ qr/(?{ CORE::__SUB__->() })/' GNU gdb 6.3.50-20050815 (Apple version gdb-1469) (Wed May 5 04:30:06 UTC 2010) Copyright 2004 Free Software Foundation\, Inc. GDB is free software\, covered by the GNU General Public License\, and you are welcome to change it and/or distribute copies of it under certain conditions. Type "show copying" to see the conditions. There is absolutely no warranty for GDB. Type "show warranty" for details. This GDB was configured as "i386-apple-darwin"...Reading symbols for shared libraries ... done

(gdb) run Starting program: /Users/sprout/Perl/perl.git/perl -Ilib -e \"\"\ =\~\ qr/$\?\{\ CORE::__SUB__-\>\($\ \}\)/ Reading symbols for shared libraries ++.. done

Program received signal EXC_BAD_ACCESS\, Could not access memory. Reason: KERN_PROTECTION_FAILURE at address: 0x00000000 0x0008e4a5 in Perl_reg_temp_copy (my_perl=0x800000\, ret_x=0x0\, rx=0x0) at regcomp.c:13410 warning: Source file is more recent than executable. 13410 struct regexp *const r = (struct regexp *)SvANY(rx); (gdb) bt #0 0x0008e4a5 in Perl_reg_temp_copy (my_perl=0x800000\, ret_x=0x0\, rx=0x0) at regcomp.c:13410 #1 0x000ff700 in Perl_pp_qr (my_perl=0x800000) at pp_hot.c:1225 #2 0x000bf0b9 in Perl_runops_debug (my_perl=0x800000) at dump.c:2133 #3 0x001ca685 in S_regtry (my_perl=0x800000\, reginfo=0xbffff598\, startpos=0xbffff5cc) at regexec.c:4405 #4 0x001e0378 in Perl_regexec_flags (my_perl=0x800000\, rx=0x804620\, stringarg=0x3085f0 ""\, strend=0x3085f0 ""\, strbeg=0x3085f0 ""\, minend=\<value temporarily unavailable\, due to optimizations>\, sv=0x825b80\, data=0x0\, flags=\<value temporarily unavailable\, due to optimizations>) at regexec.c:2528 #5 0x000fe65e in Perl_pp_match (my_perl=0x800000) at pp_hot.c:1378 #6 0x000bf0b9 in Perl_runops_debug (my_perl=0x800000) at dump.c:2133 #7 0x00030aa7 in S_run_body [inlined] () at /Users/sprout/Perl/perl.git/perl.c:2414 #8 0x00030aa7 in perl_run (my_perl=0x800000) at perl.c:2332 #9 0x00001a7c in main (argc=4\, argv=0xbffff830\, env=0xbffff844) at perlmain.c:120 (gdb)

--

Father Chrysostomos

p5pRT commented 12 years ago

From @iabyn

On Sun\, Jul 08\, 2012 at 06:40:28AM -0700\, Father Chrysostomos via RT wrote:

On Sun Jul 08 05:00:46 2012\, davem wrote:
My gut reaction is the anon sub currently being executed\, but I'm willing to be persuaded otherwise.

i.e in
use feature 'current\_sub';
sub f \{ 
my @&#8203;s;
my $r = qr/$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$/;
my $code = '$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$';
use re 'eval';
/$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$$r$code/;
\}
I would expect @s to contain \&f\, plus two different anon subs. (Although the fact that $s[2] is an anon sub sounds very much like an implementation detail\, and might change).
And calling it crashes. So we really shouldn’t be exposing it.

Or alternatively\, should we/I be making it so that it doesn't crash if called?

-- Art is anything that has a label (especially if the label is "untitled 1")

p5pRT commented 12 years ago

From @cpansprout

On Sun Jul 08 06:55:21 2012\, davem wrote:

On Sun\, Jul 08\, 2012 at 06:40:28AM -0700\, Father Chrysostomos via RT wrote:
On Sun Jul 08 05:00:46 2012\, davem wrote:
My gut reaction is the anon sub currently being executed\, but I'm willing to be persuaded otherwise.

i.e in
use feature 'current\_sub';
sub f \{ 
my @&#8203;s;
my $r = qr/$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$/;
my $code = '$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$';
use re 'eval';
/$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$$r$code/;
\}
I would expect @s to contain \&f\, plus two different anon subs. (Although the fact that $s[2] is an anon sub sounds very much like an implementation detail\, and might change).
And calling it crashes. So we really shouldn’t be exposing it.
Or alternatively\, should we/I be making it so that it doesn't crash if called?

Either that\, or we can make __SUB__ return undef in that case\, the way it does inside an eval. (Or\, again\, skip the entire call frame as I earlier suggested.)

Making it callable does not make much sense to me\, as it doesn’t do much:

$ ./perl -Ilib -MO=Concise\,foo -e 'BEGIN{"" =~ qr/(?{ *foo = CORE::__SUB__ })/}' main::foo: 2 \<1> leavesub[1 ref] K/REFC\,1 ->(end) 1 \</> qr() P/RTIME ->2 -e syntax OK

--

Father Chrysostomos

p5pRT commented 12 years ago

From [Unknown Contact. See original ticket]

On Sun Jul 08 06:55:21 2012\, davem wrote:

On Sun\, Jul 08\, 2012 at 06:40:28AM -0700\, Father Chrysostomos via RT wrote:
On Sun Jul 08 05:00:46 2012\, davem wrote:
My gut reaction is the anon sub currently being executed\, but I'm willing to be persuaded otherwise.

i.e in
use feature 'current\_sub';
sub f \{ 
my @&#8203;s;
my $r = qr/$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$/;
my $code = '$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$';
use re 'eval';
/$?\{ push @&#8203;s\, \_\_SUB\_\_ \}$$r$code/;
\}
I would expect @s to contain \&f\, plus two different anon subs. (Although the fact that $s[2] is an anon sub sounds very much like an implementation detail\, and might change).
And calling it crashes. So we really shouldn’t be exposing it.
Or alternatively\, should we/I be making it so that it doesn't crash if called?

Either that\, or we can make __SUB__ return undef in that case\, the way it does inside an eval. (Or\, again\, skip the entire call frame as I earlier suggested.)

Making it callable does not make much sense to me\, as it doesn’t do much:

$ ./perl -Ilib -MO=Concise\,foo -e 'BEGIN{"" =~ qr/(?{ *foo = CORE::__SUB__ })/}' main::foo: 2 \<1> leavesub[1 ref] K/REFC\,1 ->(end) 1 \</> qr() P/RTIME ->2 -e syntax OK

--

Father Chrysostomos

p5pRT commented 12 years ago

From @iabyn

On Sun\, Jul 08\, 2012 at 12:41:14PM -0700\, Father Chrysostomos via RT wrote:

Either that\, or we can make __SUB__ return undef in that case\, the way it does inside an eval.

Ah\, interesting about eval\, I didn't know that. In that case\, it makes more sense for me now that __SUB__ should always return undef for regex code blocks (and in fact\, that's what they used to do). We could document __SUB__ as returning undef for (?{})\, but subject to change\, to leave an opening for doing Something Better at some point.

(Or\, again\, skip the entire call frame as I earlier suggested.)

I still don't like the idea that in the following:

1: sub c { print +(caller)[2]\, "\n"; } 2: $qr = qr/(?{ c() })/; 3: "foo" =~ /foo$qr/;

the caller of c() would be identified as being on line 3 rather than 2\, by analogy with

1: sub c { print +(caller)[2]\, "\n"; } 2: $s = sub { c() }; 3: $s->();

which reports c being called from line 2\, not 3.

-- Modern art: "That's easy\, I could have done that!" "Ah\, but you didn't!"

p5pRT commented 11 years ago

From @iabyn

On Thu\, Jul 12\, 2012 at 04:40:14PM +0100\, Dave Mitchell wrote: [ stuff about caller and __SUB__ in /(?{})? code blocks. ]

I've just applied the following three commits to blead\, which collectively fix up the behaviour of caller() / __SUB__ enough for 5.18.0 IMHO.

commit b00652470bcd63ec9dee77ad3149214aebcee0df Author: David Mitchell \davem@iabyn\.com AuthorDate: Wed Apr 24 11:14:39 2013 +0100 Commit: David Mitchell \davem@iabyn\.com CommitDate: Wed Apr 24 16:39:47 2013 +0100

PUSH_MULTICALL_WITHDEPTH becomes ..._FLAGS
Two non-API macros were added with 5.17.1 to support the more complex calling conventions required by /({})/ code blocks:
PUSH_MULTICALL_WITHDEPTH(the_cv\, depth) CHANGE_MULTICALL_WITHDEPTH(the_cv\, depth)
which allowed us to do the same as the API versions\, but to optionally not increment the caller depth\, and to change the current CV.
Replace these with two new macros:
PUSH_MULTICALL_FLAGS(the_cv\, flags) CHANGE_MULTICALL_FLAGS(the_cv\, flags)
which instead allow us to set extra flags in cx->cx_type. The depth increment skip is handled by the new CXp_SUB_RE_FAKE flag\, and all (?{}) calls set the new CXp_SUB_RE flag.
These two new flags will shortly allow us to change how caller() and __SUB__ handle code blocks.

commit 5fbe83117ea59ccad42477c465113c7550a3675d Author: David Mitchell \davem@iabyn\.com AuthorDate: Wed Apr 24 14:41:33 2013 +0100 Commit: David Mitchell \davem@iabyn\.com CommitDate: Wed Apr 24 16:39:47 2013 +0100

fix caller with re_evals.
(See RT #113928)
In code like
sub foo { /A(?{ bar; caller(); }B/; }
the regex /A(?{B})C/ is\, from a scope point of view\, supposed to be compiled and executed as:
/A/ && do { B } && /C/;
i.e. the code block in B is part of the same sub as the code surrounding the regex. Thus the result of caller() above should see the caller as whoever called foo.
Due to an implementation detail\, we actually push a hidden extra sub CX before calling the pattern. This detail was leaking when caller() was used. Fux it so that it ignores this extra context frame.
Conversely\, for a qr//\, that *is* supposed to be seen as an extra level of anonymous sub\, so add tests to ensure that is so. i.e.
$r = qr/...(?{code}).../ /...$r.../
is supposed to behave like
$r = sub { code }; $r->();

commit a453e28ae209e82ef2533e2e2bfe25490fd60654 Author: David Mitchell \davem@iabyn\.com AuthorDate: Wed Apr 24 16:29:42 2013 +0100 Commit: David Mitchell \davem@iabyn\.com CommitDate: Wed Apr 24 16:40:15 2013 +0100

make qr/(?{ __SUB__ })/ safe
(See RT #113928)
Formerly\, __SUB__ within a code block within a qr// returned a pointer to the "hidden" anon CV that implements the qr// closure. Since this was never designed to be called directly\, it would SEGV if you tried.
The easiest way to make this safe is to skip any CXt_SUB frames that are marked as CXp_SUB_RE: i.e. skip any subs that are there just to execute code blocks. For a qr//\, this means that we return the sub which the pattern match is embedded in.
Also\, document the behaviour of __SUB__ within code blocks as being subject to change. It could be argued for example that in these cases it should return undef. But with the 5.18.0 release a month or two away\, just make it safe for now\, and revisit the semantics later if necessary.

-- "Procrastination grows to fill the available time" -- Mitchell's corollary to Parkinson's Law

p5pRT commented 11 years ago

@iabyn - Status changed from 'open' to 'resolved'

p5pRT commented 11 years ago

From @cpansprout

On Wed Apr 24 09:18:59 2013\, davem wrote:

On Thu\, Jul 12\, 2012 at 04:40:14PM +0100\, Dave Mitchell wrote: [ stuff about caller and __SUB__ in /(?{})? code blocks. ]

I've just applied the following three commits to blead\, which collectively fix up the behaviour of caller() / __SUB__ enough for 5.18.0 IMHO.

commit b00652470bcd63ec9dee77ad3149214aebcee0df Author: David Mitchell \davem@iabyn\.com AuthorDate: Wed Apr 24 11:14:39 2013 +0100 Commit: David Mitchell \davem@iabyn\.com CommitDate: Wed Apr 24 16:39:47 2013 +0100
PUSH\_MULTICALL\_WITHDEPTH becomes \.\.\.\_FLAGS

Thank you for taking care of that.

--

Father Chrysostomos

p5pRT commented 11 years ago

From [Unknown Contact. See original ticket]

On Wed Apr 24 09:18:59 2013\, davem wrote:

On Thu\, Jul 12\, 2012 at 04:40:14PM +0100\, Dave Mitchell wrote: [ stuff about caller and __SUB__ in /(?{})? code blocks. ]

I've just applied the following three commits to blead\, which collectively fix up the behaviour of caller() / __SUB__ enough for 5.18.0 IMHO.

commit b00652470bcd63ec9dee77ad3149214aebcee0df Author: David Mitchell \davem@iabyn\.com AuthorDate: Wed Apr 24 11:14:39 2013 +0100 Commit: David Mitchell \davem@iabyn\.com CommitDate: Wed Apr 24 16:39:47 2013 +0100
PUSH\_MULTICALL\_WITHDEPTH becomes \.\.\.\_FLAGS

Thank you for taking care of that.

--

Father Chrysostomos

Perl / perl5