search

Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.9k stars 540 forks source link

Referencing a PADTMP twice produces two copies #10690

Closed p5pRT closed 11 years ago

p5pRT commented 13 years ago

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

Searchable as RT78194$

p5pRT commented 13 years ago

From @cpansprout

$ perl -le' for( "$y" ){print \$_.\$_}' SCALAR(0x8039f0)SCALAR(0x8038d0)

(The same address should be printed twice.)

It looks as though the TARG ought to be stolen by S_refto\, and then replaced via pad_swipe or something similar.

Flags​:   category=core   severity=low

Site configuration information for perl 5.13.5​:

Configured by sprout at Mon Sep 27 23​:15​:22 PDT 2010.

Summary of my perl5 (revision 5 version 13 subversion 5) configuration​:   Commit id​: 3eab78e37a32aa79c954c0d957e1e37964cc9cc4   Platform​:   osname=darwin\, osvers=10.4.0\, archname=darwin-thread-multi-2level   uname='darwin pint.local 10.4.0 darwin kernel version 10.4.0​: fri apr 23 18​:28​:53 pdt 2010; root​:xnu-1504.7.4~1release_i386 i386 '   config_args='-de -Dusedevel -Duseithreads -DDEBUGGING'   hint=recommended\, useposix=true\, d_sigaction=define   useithreads=define\, usemultiplicity=define   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 -no-cpp-precomp -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'\,   optimize='-O3 -g'\,   cppflags='-no-cpp-precomp -fno-common -DPERL_DARWIN -no-cpp-precomp -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=/usr/lib/libc.dylib\, 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.13.5​:   /usr/local/lib/perl5/site_perl/5.13.5/darwin-thread-multi-2level   /usr/local/lib/perl5/site_perl/5.13.5   /usr/local/lib/perl5/5.13.5/darwin-thread-multi-2level   /usr/local/lib/perl5/5.13.5   /usr/local/lib/perl5/site_perl   .

Environment for perl 5.13.5​:   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 13 years ago

From @cpansprout

On Sun Oct 03 14​:08​:37 2010\, sprout wrote​:

$ perl -le' for( "$y" ){print \$_.\$_}' SCALAR(0x8039f0)SCALAR(0x8038d0)

(The same address should be printed twice.)

It looks as though the TARG ought to be stolen by S_refto\, and then replaced via pad_swipe or something similar.

It might be cleaner for not only S_refto\, but also any op that aliases a PADTMP to copy it. That would include for\, map\, grep\, arguments to subroutines\, and the lhs of (...)x...

p5pRT commented 13 years ago

From [Unknown Contact. See original ticket]

On Sun Oct 03 14​:08​:37 2010\, sprout wrote​:

$ perl -le' for( "$y" ){print \$_.\$_}' SCALAR(0x8039f0)SCALAR(0x8038d0)

(The same address should be printed twice.)

It looks as though the TARG ought to be stolen by S_refto\, and then replaced via pad_swipe or something similar.

It might be cleaner for not only S_refto\, but also any op that aliases a PADTMP to copy it. That would include for\, map\, grep\, arguments to subroutines\, and the lhs of (...)x...

p5pRT commented 13 years ago

@cpansprout - Status changed from 'new' to 'open'

p5pRT commented 12 years ago

From @cpansprout

On Sun Jun 05 16​:04​:53 2011\, sprout wrote​:

On Sun Oct 03 14​:08​:37 2010\, sprout wrote​:

$ perl -le' for( "$y" ){print \$_.\$_}' SCALAR(0x8039f0)SCALAR(0x8038d0)

(The same address should be printed twice.)

It looks as though the TARG ought to be stolen by S_refto\, and then replaced via pad_swipe or something similar.

It might be cleaner for not only S_refto\, but also any op that aliases a PADTMP to copy it. That would include for\, map\, grep\, arguments to subroutines\, and the lhs of (...)x...

Here is a difficult case​:

$ ./perl -Ilib -e 'use Devel​::Peek; "$_" =~ /(?{ Dump $_ })/;' SV = PVMG(0x824b78) at 0x826ba0   REFCNT = 1   FLAGS = (PADTMP\,SMG\,POK\,pPOK) ...

Fixing the operators I mentioned above is easy. Fixing patterns without a speed hit (since regexps are used so often) is not so easy.

Are regular expressions with code blocks in them flagged somehow\, so we can avoid making a copy for every PADTMP =~ // ?

In case it’s not obvious\, this is the bug I’m trying to fix​:

$ ./perl -Ilib -le '"$_" =~ /(?{ print \$_; print \$_ })/;' SCALAR(0x803a10) SCALAR(0x803b50)

Taking a reference to $_ twice should not create references to two different variables. The implementation details are leaking through.

--

Father Chrysostomos

p5pRT commented 12 years ago

From [Unknown Contact. See original ticket]

On Sun Jun 05 16​:04​:53 2011\, sprout wrote​:

On Sun Oct 03 14​:08​:37 2010\, sprout wrote​:

$ perl -le' for( "$y" ){print \$_.\$_}' SCALAR(0x8039f0)SCALAR(0x8038d0)

(The same address should be printed twice.)

It looks as though the TARG ought to be stolen by S_refto\, and then replaced via pad_swipe or something similar.

It might be cleaner for not only S_refto\, but also any op that aliases a PADTMP to copy it. That would include for\, map\, grep\, arguments to subroutines\, and the lhs of (...)x...

Here is a difficult case​:

$ ./perl -Ilib -e 'use Devel​::Peek; "$_" =~ /(?{ Dump $_ })/;' SV = PVMG(0x824b78) at 0x826ba0   REFCNT = 1   FLAGS = (PADTMP\,SMG\,POK\,pPOK) ...

Fixing the operators I mentioned above is easy. Fixing patterns without a speed hit (since regexps are used so often) is not so easy.

Are regular expressions with code blocks in them flagged somehow\, so we can avoid making a copy for every PADTMP =~ // ?

In case it’s not obvious\, this is the bug I’m trying to fix​:

$ ./perl -Ilib -le '"$_" =~ /(?{ print \$_; print \$_ })/;' SCALAR(0x803a10) SCALAR(0x803b50)

Taking a reference to $_ twice should not create references to two different variables. The implementation details are leaking through.

--

Father Chrysostomos

p5pRT commented 12 years ago

From @iabyn

On Sun\, Jan 08\, 2012 at 02​:21​:34PM -0800\, Father Chrysostomos via RT wrote​:

On Sun Jun 05 16​:04​:53 2011\, sprout wrote​:

On Sun Oct 03 14​:08​:37 2010\, sprout wrote​:

$ perl -le' for( "$y" ){print \$_.\$_}' SCALAR(0x8039f0)SCALAR(0x8038d0)

(The same address should be printed twice.)

It looks as though the TARG ought to be stolen by S_refto\, and then replaced via pad_swipe or something similar.

It might be cleaner for not only S_refto\, but also any op that aliases a PADTMP to copy it. That would include for\, map\, grep\, arguments to subroutines\, and the lhs of (...)x...

Here is a difficult case​:

$ ./perl -Ilib -e 'use Devel​::Peek; "$_" =~ /(?{ Dump $_ })/;' SV = PVMG(0x824b78) at 0x826ba0 REFCNT = 1 FLAGS = (PADTMP\,SMG\,POK\,pPOK) ...

Fixing the operators I mentioned above is easy. Fixing patterns without a speed hit (since regexps are used so often) is not so easy.

Are regular expressions with code blocks in them flagged somehow\, so we can avoid making a copy for every PADTMP =~ // ?

yes\, RXf_EVAL_SEEN. It's used at the start of S_regtry to decide whether to alias $_ (and PL_reg_eval_set is then set\, to avoid doing it multiple times).

In case it’s not obvious\, this is the bug I’m trying to fix​:

$ ./perl -Ilib -le '"$_" =~ /(?{ print \$_; print \$_ })/;' SCALAR(0x803a10) SCALAR(0x803b50)

Taking a reference to $_ twice should not create references to two different variables. The implementation details are leaking through.

Confused the hell out me! :-) I didn't realise until I looked at it just now\, that $_ is aliased within code blocks.

-- Gravity is just a theory; teach Intelligent Falling in our schools!   http​://www.theonion.com/content/node/39512

p5pRT commented 12 years ago

From patcat88@snet.net

On Sun Jan 08 14​:21​:34 2012\, sprout wrote​: ................................

Taking a reference to $_ twice should not create references to two different variables. The implementation details are leaking through.

I'll add my PerlMonks discussion to this http​://www.perlmonks.org/?node_id=941973 . PADTMPs should always be copied (or sv body stolen ??)\, not referenced. srefgen/S_refto and newRV do different things but they should be the same. PADTMPs are supposed to be allocated and dealloced on block enterance/leaving\, but under the hood are effectively closures or C function level static. Its a mess.

p5pRT commented 12 years ago

From [Unknown Contact. See original ticket]

On Sun Jan 08 14​:21​:34 2012\, sprout wrote​: ................................

Taking a reference to $_ twice should not create references to two different variables. The implementation details are leaking through.

I'll add my PerlMonks discussion to this http​://www.perlmonks.org/?node_id=941973 . PADTMPs should always be copied (or sv body stolen ??)\, not referenced. srefgen/S_refto and newRV do different things but they should be the same. PADTMPs are supposed to be allocated and dealloced on block enterance/leaving\, but under the hood are effectively closures or C function level static. Its a mess.

p5pRT commented 11 years ago

From @cpansprout

This is related to #7505\, which is one instance of this bug.

p5pRT commented 11 years ago

From [Unknown Contact. See original ticket]

This is related to #7505\, which is one instance of this bug.

p5pRT commented 11 years ago

From @cpansprout

On Sat Jun 15 13​:49​:57 2013\, sprout wrote​:

This is related to #7505\, which is one instance of this bug.

Um\, nope\, it’s not. Ignore that.

--

Father Chrysostomos

p5pRT commented 11 years ago

From [Unknown Contact. See original ticket]

On Sat Jun 15 13​:49​:57 2013\, sprout wrote​:

This is related to #7505\, which is one instance of this bug.

Um\, nope\, it’s not. Ignore that.

--

Father Chrysostomos

p5pRT commented 11 years ago

From @cpansprout

Fixed in​:

a0ed822 b479c9f 8e079c2 706a6eb da9e430 2b66f6d 82c2360

--

Father Chrysostomos

p5pRT commented 11 years ago

From [Unknown Contact. See original ticket]

Fixed in​:

a0ed822 b479c9f 8e079c2 706a6eb da9e430 2b66f6d 82c2360

--

Father Chrysostomos

p5pRT commented 11 years ago

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

p5pRT commented 11 years ago

From @cpansprout

On Fri Jan 13 13​:16​:42 2012\, patcat88 wrote​:

On Sun Jan 08 14​:21​:34 2012\, sprout wrote​: ................................

Taking a reference to $_ twice should not create references to two different variables. The implementation details are leaking through.

I'll add my PerlMonks discussion to this http​://www.perlmonks.org/?node_id=941973 . PADTMPs should always be copied (or sv body stolen ??)\, not referenced. srefgen/S_refto and newRV do different things but they should be the same. PADTMPs are supposed to be allocated and dealloced on block enterance/leaving\, but under the hood are effectively closures or C function level static. Its a mess.

This part is not resolved. I’m reopening it.

--

Father Chrysostomos

p5pRT commented 11 years ago

From [Unknown Contact. See original ticket]

On Fri Jan 13 13​:16​:42 2012\, patcat88 wrote​:

On Sun Jan 08 14​:21​:34 2012\, sprout wrote​: ................................

Taking a reference to $_ twice should not create references to two different variables. The implementation details are leaking through.

I'll add my PerlMonks discussion to this http​://www.perlmonks.org/?node_id=941973 . PADTMPs should always be copied (or sv body stolen ??)\, not referenced. srefgen/S_refto and newRV do different things but they should be the same. PADTMPs are supposed to be allocated and dealloced on block enterance/leaving\, but under the hood are effectively closures or C function level static. Its a mess.

This part is not resolved. I’m reopening it.

--

Father Chrysostomos

p5pRT commented 11 years ago

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

p5pRT commented 11 years ago

From @cpansprout

On Fri Jul 26 23​:12​:23 2013\, sprout wrote​:

On Fri Jan 13 13​:16​:42 2012\, patcat88 wrote​:

On Sun Jan 08 14​:21​:34 2012\, sprout wrote​: ................................

Taking a reference to $_ twice should not create references to two different variables. The implementation details are leaking through.

I'll add my PerlMonks discussion to this http​://www.perlmonks.org/?node_id=941973 . PADTMPs should always be copied (or sv body stolen ??)\, not referenced. srefgen/S_refto and newRV do different things but they should be the same. PADTMPs are supposed to be allocated and dealloced on block enterance/leaving\, but under the hood are effectively closures or C function level static. Its a mess.

This part is not resolved. I’m reopening it.

I did not fix this bug for calls to XSUBs\, because it will make Devel​::Peek less useful. Calling Dump("$x") and being able to see what pp_stringify is return is a useful feature.

(Here I’m referring to the original bug reported\, not the newRV issue. Calling foo("$x") allowed the foo sub to see a TARG in $_[0]\, such that print \$_[0]\, \$_[0] would show two different addresses.)

Currently foo("$x") will make a COW copy of the TARG if it is a Perl sub\, but pass the TARG itself if it is an XSUB. So for XS code to call newRV on that is wrong (or what newRV does is wrong).

I think the real solution here is to apply the fix to XSUBs (copy TARG arguments) but modify Devel​::Peek to inline itself\, so that it remains exempt.

That way the only XS code having access to TARGs will be that which rummages through pads. For most XS code this will just dwim.

In fact\, having Devel​::Peek inline itself would be useful for seeing the return value of substr and vec in rvalue context\, something currently not possible.

--

Father Chrysostomos

p5pRT commented 11 years ago

From [Unknown Contact. See original ticket]

On Fri Jul 26 23​:12​:23 2013\, sprout wrote​:

On Fri Jan 13 13​:16​:42 2012\, patcat88 wrote​:

On Sun Jan 08 14​:21​:34 2012\, sprout wrote​: ................................

Taking a reference to $_ twice should not create references to two different variables. The implementation details are leaking through.

I'll add my PerlMonks discussion to this http​://www.perlmonks.org/?node_id=941973 . PADTMPs should always be copied (or sv body stolen ??)\, not referenced. srefgen/S_refto and newRV do different things but they should be the same. PADTMPs are supposed to be allocated and dealloced on block enterance/leaving\, but under the hood are effectively closures or C function level static. Its a mess.

This part is not resolved. I’m reopening it.

I did not fix this bug for calls to XSUBs\, because it will make Devel​::Peek less useful. Calling Dump("$x") and being able to see what pp_stringify is return is a useful feature.

(Here I’m referring to the original bug reported\, not the newRV issue. Calling foo("$x") allowed the foo sub to see a TARG in $_[0]\, such that print \$_[0]\, \$_[0] would show two different addresses.)

Currently foo("$x") will make a COW copy of the TARG if it is a Perl sub\, but pass the TARG itself if it is an XSUB. So for XS code to call newRV on that is wrong (or what newRV does is wrong).

I think the real solution here is to apply the fix to XSUBs (copy TARG arguments) but modify Devel​::Peek to inline itself\, so that it remains exempt.

That way the only XS code having access to TARGs will be that which rummages through pads. For most XS code this will just dwim.

In fact\, having Devel​::Peek inline itself would be useful for seeing the return value of substr and vec in rvalue context\, something currently not possible.

--

Father Chrysostomos

p5pRT commented 11 years ago

From @cpansprout

On Sat Jul 27 11​:27​:57 2013\, sprout wrote​:

I did not fix this bug for calls to XSUBs\, because it will make Devel​::Peek less useful. Calling Dump("$x") and being able to see what pp_stringify is return[ing] is a useful feature.

(Here I’m referring to the original bug reported\, not the newRV issue. Calling foo("$x") allowed the foo sub to see a TARG in $_[0]\, such that print \$_[0]\, \$_[0] would show two different addresses.)

Currently foo("$x") will make a COW copy of the TARG if it is a Perl sub\, but pass the TARG itself if it is an XSUB. So for XS code to call newRV on that is wrong (or what newRV does is wrong).

I think the real solution here is to apply the fix to XSUBs (copy TARG arguments) but modify Devel​::Peek to inline itself\, so that it remains exempt.

That way the only XS code having access to TARGs will be that which rummages through pads. For most XS code this will just dwim.

In fact\, having Devel​::Peek inline itself would be useful for seeing the return value of substr and vec in rvalue context\, something currently not possible.

Attached is a patch to get Devel​::Peek to inline itself. It also allows Dump %hash and Dump @​array.

It breaks compatibility in that ‘@​args = ($thing\, 5); Dump @​args’ no longer works; scalar context is applied to both arguments now\, and the number of arguments is checked at compile time\, rather than run time.

I still think it is worth it.

The patch can also be found on the sprout/peek branch.

--

Father Chrysostomos

p5pRT commented 11 years ago

From @cpansprout

From b71bcee5cdd1e9dc06692914a330bb543cacc16b Mon Sep 17 00​:00​:00 2001 From​: Father Chrysostomos \sprout@​cpan\.org Date​: Sun\, 11 Aug 2013 21​:54​:11 -0700 Subject​: [PATCH] Inline Devel​::Peek​::Dump; allow Dump %hash etc. MIME-Version​: 1.0 Content-Type​: text/plain; charset=UTF-8 Content-Transfer-Encoding​: 8bit

This commit makes Devel​::Peek​::Dump modify the op tree to allow it to dump arrays and hashes directly via Dump @​array and Dump %hash. It also puts other operators in rvalue context\, allowing the return value of rvalue substr for instance to be dumped\, making Devel​::Peek more useful as a debugging tool.

Since a future commit (to fix the rest of #78194) is likely to make pp_entersub copy PADTMPs (operator return values) for XSUBs (it already happens for Perl subs as of b479c9f2a)\, to the detriment of Devel​::Peek’s usefulness\, I also made it inline Dump as a custom op.

This does introduce a backward-incompatible change\, in that both argu- ments to Dump are now in scalar context\, and the number of arguments is checked at compile time instead of run time (still run time for &Dump(...))\, but I think it is worth it.

Inline Patch ```diff diff --git a/ext/Devel-Peek/Peek.xs b/ext/Devel-Peek/Peek.xs index 4c5f974..edcb02f 100644 --- a/ext/Devel-Peek/Peek.xs +++ b/ext/Devel-Peek/Peek.xs @@ -323,6 +323,94 @@ mstats2hash(SV *sv, SV *rv, int level) (SvROK(cv) && (SvTYPE(SvRV(cv))==SVt_PVCV) \ ? SvREFCNT_inc(CvGV((CV*)SvRV(cv))) : &PL_sv_undef) +static void +S_do_dump(pTHX_ SV *const sv, I32 lim) +{ + dVAR; + SV *pv_lim_sv = perl_get_sv("Devel::Peek::pv_limit", 0); + const STRLEN pv_lim = pv_lim_sv ? SvIV(pv_lim_sv) : 0; + SV *dumpop = perl_get_sv("Devel::Peek::dump_ops", 0); + const U16 save_dumpindent = PL_dumpindent; + PL_dumpindent = 2; + do_sv_dump(0, Perl_debug_log, sv, 0, lim, + (bool)(dumpop && SvTRUE(dumpop)), pv_lim); + PL_dumpindent = save_dumpindent; +} + +static OP * +S_pp_dump(pTHX) +{ + dSP; + const I32 lim = PL_op->op_private == 2 ? (I32)POPi : 4; + dPOPss; + S_do_dump(aTHX_ sv, lim); + RETPUSHUNDEF; +} + +static OP * +S_ck_dump(pTHX_ OP *entersubop, GV *namegv, SV *cv) +{ + OP *aop, *prev, *first, *second = NULL; + BINOP *newop; + size_t arg = 0; + + ck_entersub_args_proto(entersubop, namegv, + newSVpvn_flags("$;$", 3, SVs_TEMP)); + + aop = cUNOPx(entersubop)->op_first; + if (!aop->op_sibling) + aop = cUNOPx(aop)->op_first; + prev = aop; + aop = aop->op_sibling; + while (PL_madskills && aop->op_type == OP_STUB) { + prev = aop; + aop = aop->op_sibling; + } + if (PL_madskills && aop->op_type == OP_NULL) { + first = ((UNOP*)aop)->op_first; + ((UNOP*)aop)->op_first = NULL; + prev = aop; + } + else { + first = aop; + prev->op_sibling = first->op_sibling; + } + if (first->op_type == OP_RV2AV || + first->op_type == OP_PADAV || + first->op_type == OP_RV2HV || + first->op_type == OP_PADHV + ) + first->op_flags |= OPf_REF; + else + first->op_flags &= ~OPf_MOD; + aop = aop->op_sibling; + while (PL_madskills && aop->op_type == OP_STUB) { + prev = aop; + aop = aop->op_sibling; + } + /* aop now points to the second arg if there is one, the cvop otherwise + */ + if ((prev->op_sibling = aop->op_sibling)) { + second = aop; + second->op_sibling = NULL; + } + first->op_sibling = second; + + op_free(entersubop); + + NewOp(1234, newop, 1, BINOP); + newop->op_type = OP_CUSTOM; + newop->op_ppaddr = S_pp_dump; + newop->op_first = first; + newop->op_last = second; + newop->op_private= second ? 2 : 1; + newop->op_flags = OPf_KIDS|OPf_WANT_SCALAR; + + return (OP *)newop; +} + +static XOP my_xop; + MODULE = Devel::Peek PACKAGE = Devel::Peek void @@ -346,14 +434,18 @@ SV * sv I32 lim PPCODE: { - SV *pv_lim_sv = perl_get_sv("Devel::Peek::pv_limit", 0); - const STRLEN pv_lim = pv_lim_sv ? SvIV(pv_lim_sv) : 0; - SV *dumpop = perl_get_sv("Devel::Peek::dump_ops", 0); - const U16 save_dumpindent = PL_dumpindent; - PL_dumpindent = 2; - do_sv_dump(0, Perl_debug_log, sv, 0, lim, - (bool)(dumpop && SvTRUE(dumpop)), pv_lim); - PL_dumpindent = save_dumpindent; + S_do_dump(aTHX_ sv, lim); +} + +BOOT: +{ + CV * const cv = get_cvn_flags("Devel::Peek::Dump", 17, 0); + cv_set_call_checker(cv, S_ck_dump, (SV *)cv); + + XopENTRY_set(&my_xop, xop_name, "Dump"); + XopENTRY_set(&my_xop, xop_desc, "Dump"); + XopENTRY_set(&my_xop, xop_class, OA_BINOP); + Perl_custom_op_register(aTHX_ S_pp_dump, &my_xop); } void diff --git a/ext/Devel-Peek/t/Peek.t b/ext/Devel-Peek/t/Peek.t index 088f505..1f344df 100644 --- a/ext/Devel-Peek/t/Peek.t +++ b/ext/Devel-Peek/t/Peek.t @@ -31,11 +31,24 @@ sub do_test { my $todo = $_[3]; my $repeat_todo = $_[4]; my $pattern = $_[2]; + my $do_eval = $_[5]; if (open(OUT,">peek$$")) { open(STDERR, ">&OUT") or die "Can't dup OUT: $!"; - Dump($_[1]); - print STDERR "*****\n"; - Dump($_[1]); # second dump to compare with the first to make sure nothing changed. + if ($do_eval) { + my $sub = eval "sub { Dump $_[1] }"; + $sub->(); + print STDERR "*****\n"; + # second dump to compare with the first to make sure nothing + # changed. + $sub->(); + } + else { + Dump($_[1]); + print STDERR "*****\n"; + # second dump to compare with the first to make sure nothing + # changed. + Dump($_[1]); + } open(STDERR, ">&SAVERR") or die "Can't restore STDERR: $!"; close(OUT); if (open(IN, "peek$$")) { @@ -196,8 +209,8 @@ do_test('integer constant', do_test('undef', undef, 'SV = NULL\\(0x0\\) at $ADDR - REFCNT = 1 - FLAGS = \\(\\)'); + REFCNT = \d+ + FLAGS = \\(READONLY\\)'); do_test('reference to scalar', \$a, @@ -335,6 +348,8 @@ do_test('reference to named subroutine without prototype', \\d+\\. $ADDR<\\d+> \\(\\d+,\\d+\\) "\\$todo" \\d+\\. $ADDR<\\d+> \\(\\d+,\\d+\\) "\\$repeat_todo" \\d+\\. $ADDR<\\d+> \\(\\d+,\\d+\\) "\\$pattern" + \\d+\\. $ADDR<\\d+> \\(\\d+,\\d+\\) "\\$do_eval" + \\d+\\. $ADDR<\\d+> \\(\\d+,\\d+\\) "\\$sub" \\d+\\. $ADDR<\\d+> FAKE "\\$DEBUG" # $] < 5.009 \\d+\\. $ADDR<\\d+> FAKE "\\$DEBUG" flags=0x0 index=0 # $] >= 5.009 \\d+\\. $ADDR<\\d+> \\(\\d+,\\d+\\) "\\$dump" @@ -968,6 +983,59 @@ do_test('large hash', Elt .* '); +# Dump with arrays, hashes, and operator return values +@array = 1..3; +do_test('Dump @array', '@array', <<'ARRAY', '', '', 1); +SV = PVAV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(\) + ARRAY = $ADDR + FILL = 2 + MAX = 3 + ARYLEN = 0x0 + FLAGS = \(REAL\) + Elt No. 0 + SV = IV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(IOK,pIOK\) + IV = 1 + Elt No. 1 + SV = IV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(IOK,pIOK\) + IV = 2 + Elt No. 2 + SV = IV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(IOK,pIOK\) + IV = 3 +ARRAY +%hash = 1..2; +do_test('Dump %hash', '%hash', <<'HASH', '', '', 1); +SV = PVHV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(SHAREKEYS\) + ARRAY = $ADDR \(0:7, 1:1\) + hash quality = 100.0% + KEYS = 1 + FILL = 1 + MAX = 7 + Elt "1" HASH = $ADDR + SV = IV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(IOK,pIOK\) + IV = 2 +HASH +$_ = "hello"; +do_test('rvalue substr', 'substr $_, 1, 2', <<'SUBSTR', '', '', 1); +SV = PV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(PADTMP,POK,pPOK\) + PV = $ADDR "el"\\0 + CUR = 2 + LEN = 20 +SUBSTR + SKIP: { skip "Not built with usemymalloc", 2 unless $Config{usemymalloc} eq 'y'; ```
p5pRT commented 11 years ago

From [Unknown Contact. See original ticket]

On Sat Jul 27 11​:27​:57 2013\, sprout wrote​:

I did not fix this bug for calls to XSUBs\, because it will make Devel​::Peek less useful. Calling Dump("$x") and being able to see what pp_stringify is return[ing] is a useful feature.

(Here I’m referring to the original bug reported\, not the newRV issue. Calling foo("$x") allowed the foo sub to see a TARG in $_[0]\, such that print \$_[0]\, \$_[0] would show two different addresses.)

Currently foo("$x") will make a COW copy of the TARG if it is a Perl sub\, but pass the TARG itself if it is an XSUB. So for XS code to call newRV on that is wrong (or what newRV does is wrong).

I think the real solution here is to apply the fix to XSUBs (copy TARG arguments) but modify Devel​::Peek to inline itself\, so that it remains exempt.

That way the only XS code having access to TARGs will be that which rummages through pads. For most XS code this will just dwim.

In fact\, having Devel​::Peek inline itself would be useful for seeing the return value of substr and vec in rvalue context\, something currently not possible.

Attached is a patch to get Devel​::Peek to inline itself. It also allows Dump %hash and Dump @​array.

It breaks compatibility in that ‘@​args = ($thing\, 5); Dump @​args’ no longer works; scalar context is applied to both arguments now\, and the number of arguments is checked at compile time\, rather than run time.

I still think it is worth it.

The patch can also be found on the sprout/peek branch.

--

Father Chrysostomos

p5pRT commented 11 years ago

From @Hugmeir

On Mon\, Aug 12\, 2013 at 6​:00 AM\, Father Chrysostomos via RT \< perlbug-comment@​perl.org> wrote​:

On Sat Jul 27 11​:27​:57 2013\, sprout wrote​:

I did not fix this bug for calls to XSUBs\, because it will make Devel​::Peek less useful. Calling Dump("$x") and being able to see what pp_stringify is return[ing] is a useful feature.

(Here I’m referring to the original bug reported\, not the newRV issue. Calling foo("$x") allowed the foo sub to see a TARG in $_[0]\, such that print \$_[0]\, \$_[0] would show two different addresses.)

Currently foo("$x") will make a COW copy of the TARG if it is a Perl sub\, but pass the TARG itself if it is an XSUB. So for XS code to call newRV on that is wrong (or what newRV does is wrong).

I think the real solution here is to apply the fix to XSUBs (copy TARG arguments) but modify Devel​::Peek to inline itself\, so that it remains exempt.

That way the only XS code having access to TARGs will be that which rummages through pads. For most XS code this will just dwim.

In fact\, having Devel​::Peek inline itself would be useful for seeing the return value of substr and vec in rvalue context\, something currently not possible.

Attached is a patch to get Devel​::Peek to inline itself. It also allows Dump %hash and Dump @​array.

It breaks compatibility in that ‘@​args = ($thing\, 5); Dump @​args’ no longer works; scalar context is applied to both arguments now\, and the number of arguments is checked at compile time\, rather than run time.

I still think it is worth it.

The patch can also be found on the sprout/peek branch.

This is unrelated to the thread\, but I just wanted to say that looking over your attached patch made me go "Oh\, so THAT'S how you create a custom op!" Thanks\, Father C! This was educational.

About the patch itself\, I have never written Dump(@​foo) and not meant Dump(\@​foo)\, or Dump %hash and not meant Dump(\%hash)\, so the incompatible change seems good to me.

--

Father Chrysostomos

From b71bcee5cdd1e9dc06692914a330bb543cacc16b Mon Sep 17 00​:00​:00 2001 From​: Father Chrysostomos \sprout@&#8203;cpan\.org Date​: Sun\, 11 Aug 2013 21​:54​:11 -0700 Subject​: [PATCH] Inline Devel​::Peek​::Dump; allow Dump %hash etc. MIME-Version​: 1.0 Content-Type​: text/plain; charset=UTF-8 Content-Transfer-Encoding​: 8bit

This commit makes Devel​::Peek​::Dump modify the op tree to allow it to dump arrays and hashes directly via Dump @​array and Dump %hash. It also puts other operators in rvalue context\, allowing the return value of rvalue substr for instance to be dumped\, making Devel​::Peek more useful as a debugging tool.

Since a future commit (to fix the rest of #78194) is likely to make pp_entersub copy PADTMPs (operator return values) for XSUBs (it already happens for Perl subs as of b479c9f2a)\, to the detriment of Devel​::Peek’s usefulness\, I also made it inline Dump as a custom op.

This does introduce a backward-incompatible change\, in that both argu- ments to Dump are now in scalar context\, and the number of arguments is checked at compile time instead of run time (still run time for &Dump(...))\, but I think it is worth it.

diff --git a/ext/Devel-Peek/Peek.xs b/ext/Devel-Peek/Peek.xs index 4c5f974..edcb02f 100644 --- a/ext/Devel-Peek/Peek.xs +++ b/ext/Devel-Peek/Peek.xs @​@​ -323\,6 +323\,94 @​@​ mstats2hash(SV *sv\, SV *rv\, int level) (SvROK(cv) && (SvTYPE(SvRV(cv))==SVt_PVCV) \ ? SvREFCNT_inc(CvGV((CV*)SvRV(cv))) : &PL_sv_undef)

+static void +S_do_dump(pTHX_ SV *const sv\, I32 lim) +{ + dVAR; + SV *pv_lim_sv = perl_get_sv("Devel​::Peek​::pv_limit"\, 0); + const STRLEN pv_lim = pv_lim_sv ? SvIV(pv_lim_sv) : 0; + SV *dumpop = perl_get_sv("Devel​::Peek​::dump_ops"\, 0); + const U16 save_dumpindent = PL_dumpindent; + PL_dumpindent = 2; + do_sv_dump(0\, Perl_debug_log\, sv\, 0\, lim\, + (bool)(dumpop && SvTRUE(dumpop))\, pv_lim); + PL_dumpindent = save_dumpindent; +} + +static OP * +S_pp_dump(pTHX) +{ + dSP; + const I32 lim = PL_op->op_private == 2 ? (I32)POPi : 4; + dPOPss; + S_do_dump(aTHX_ sv\, lim); + RETPUSHUNDEF; +} + +static OP * +S_ck_dump(pTHX_ OP *entersubop\, GV *namegv\, SV *cv) +{ + OP *aop\, *prev\, *first\, *second = NULL; + BINOP *newop; + size_t arg = 0; + + ck_entersub_args_proto(entersubop\, namegv\, + newSVpvn_flags("$;$"\, 3\, SVs_TEMP)); + + aop = cUNOPx(entersubop)->op_first; + if (!aop->op_sibling) + aop = cUNOPx(aop)->op_first; + prev = aop; + aop = aop->op_sibling; + while (PL_madskills && aop->op_type == OP_STUB) { + prev = aop; + aop = aop->op_sibling; + } + if (PL_madskills && aop->op_type == OP_NULL) { + first = ((UNOP*)aop)->op_first; + ((UNOP*)aop)->op_first = NULL; + prev = aop; + } + else { + first = aop; + prev->op_sibling = first->op_sibling; + } + if (first->op_type == OP_RV2AV || + first->op_type == OP_PADAV || + first->op_type == OP_RV2HV || + first->op_type == OP_PADHV + ) + first->op_flags |= OPf_REF; + else + first->op_flags &= ~OPf_MOD; + aop = aop->op_sibling; + while (PL_madskills && aop->op_type == OP_STUB) { + prev = aop; + aop = aop->op_sibling; + } + /* aop now points to the second arg if there is one\, the cvop otherwise + */ + if ((prev->op_sibling = aop->op_sibling)) { + second = aop; + second->op_sibling = NULL; + } + first->op_sibling = second; + + op_free(entersubop); + + NewOp(1234\, newop\, 1\, BINOP); + newop->op_type = OP_CUSTOM; + newop->op_ppaddr = S_pp_dump; + newop->op_first = first; + newop->op_last = second; + newop->op_private= second ? 2 : 1; + newop->op_flags = OPf_KIDS|OPf_WANT_SCALAR; + + return (OP *)newop; +} + +static XOP my_xop; + MODULE = Devel​::Peek PACKAGE = Devel​::Peek

void @​@​ -346\,14 +434\,18 @​@​ SV * sv I32 lim PPCODE​: { - SV *pv_lim_sv = perl_get_sv("Devel​::Peek​::pv_limit"\, 0); - const STRLEN pv_lim = pv_lim_sv ? SvIV(pv_lim_sv) : 0; - SV *dumpop = perl_get_sv("Devel​::Peek​::dump_ops"\, 0); - const U16 save_dumpindent = PL_dumpindent; - PL_dumpindent = 2; - do_sv_dump(0\, Perl_debug_log\, sv\, 0\, lim\, - (bool)(dumpop && SvTRUE(dumpop))\, pv_lim); - PL_dumpindent = save_dumpindent; + S_do_dump(aTHX_ sv\, lim); +} + +BOOT​: +{ + CV * const cv = get_cvn_flags("Devel​::Peek​::Dump"\, 17\, 0); + cv_set_call_checker(cv\, S_ck_dump\, (SV *)cv); + + XopENTRY_set(&my_xop\, xop_name\, "Dump"); + XopENTRY_set(&my_xop\, xop_desc\, "Dump"); + XopENTRY_set(&my_xop\, xop_class\, OA_BINOP); + Perl_custom_op_register(aTHX_ S_pp_dump\, &my_xop); }

void diff --git a/ext/Devel-Peek/t/Peek.t b/ext/Devel-Peek/t/Peek.t index 088f505..1f344df 100644 --- a/ext/Devel-Peek/t/Peek.t +++ b/ext/Devel-Peek/t/Peek.t @​@​ -31\,11 +31\,24 @​@​ sub do_test { my $todo = $_[3]; my $repeat_todo = $_[4]; my $pattern = $_[2]; + my $do_eval = $_[5]; if (open(OUT\,">peek$$")) { open(STDERR\, ">&OUT") or die "Can't dup OUT​: $!"; - Dump($_[1]); - print STDERR "*****\n"; - Dump($_[1]); # second dump to compare with the first to make sure nothing changed. + if ($do_eval) { + my $sub = eval "sub { Dump $_[1] }"; + $sub->(); + print STDERR "*****\n"; + # second dump to compare with the first to make sure nothing + # changed. + $sub->(); + } + else { + Dump($_[1]); + print STDERR "*****\n"; + # second dump to compare with the first to make sure nothing + # changed. + Dump($_[1]); + } open(STDERR\, ">&SAVERR") or die "Can't restore STDERR​: $!"; close(OUT); if (open(IN\, "peek$$")) { @​@​ -196\,8 +209\,8 @​@​ do_test('integer constant'\, do_test('undef'\, undef\, 'SV = NULL\\(0x0\\) at $ADDR - REFCNT = 1 - FLAGS = \\(\\)'); + REFCNT = \d+ + FLAGS = \\(READONLY\\)');

do_test('reference to scalar'\, \$a\, @​@​ -335\,6 +348\,8 @​@​ do_test('reference to named subroutine without prototype'\, \\d+\\. $ADDR\<\\d+> \\(\\d+\,\\d+\\) "\\$todo" \\d+\\. $ADDR\<\\d+> \\(\\d+\,\\d+\\) "\\$repeat_todo" \\d+\\. $ADDR\<\\d+> \\(\\d+\,\\d+\\) "\\$pattern" + \\d+\\. $ADDR\<\\d+> \\(\\d+\,\\d+\\) "\\$do_eval" + \\d+\\. $ADDR\<\\d+> \\(\\d+\,\\d+\\) "\\$sub" \\d+\\. $ADDR\<\\d+> FAKE "\\$DEBUG" # $] \< 5.009 \\d+\\. $ADDR\<\\d+> FAKE "\\$DEBUG" flags=0x0 index=0 # $] >= 5.009 \\d+\\. $ADDR\<\\d+> \\(\\d+\,\\d+\\) "\\$dump" @​@​ -968\,6 +983\,59 @​@​ do_test('large hash'\, Elt .* ');

+# Dump with arrays\, hashes\, and operator return values +@​array = 1..3; +do_test('Dump @​array'\, '@​array'\, \<\<'ARRAY'\, ''\, ''\, 1); +SV = PVAV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(\) + ARRAY = $ADDR + FILL = 2 + MAX = 3 + ARYLEN = 0x0 + FLAGS = \(REAL\) + Elt No. 0 + SV = IV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(IOK\,pIOK\) + IV = 1 + Elt No. 1 + SV = IV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(IOK\,pIOK\) + IV = 2 + Elt No. 2 + SV = IV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(IOK\,pIOK\) + IV = 3 +ARRAY +%hash = 1..2; +do_test('Dump %hash'\, '%hash'\, \<\<'HASH'\, ''\, ''\, 1); +SV = PVHV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(SHAREKEYS\) + ARRAY = $ADDR \(0​:7\, 1​:1\) + hash quality = 100.0% + KEYS = 1 + FILL = 1 + MAX = 7 + Elt "1" HASH = $ADDR + SV = IV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(IOK\,pIOK\) + IV = 2 +HASH +$_ = "hello"; +do_test('rvalue substr'\, 'substr $_\, 1\, 2'\, \<\<'SUBSTR'\, ''\, ''\, 1); +SV = PV\($ADDR\) at $ADDR + REFCNT = 1 + FLAGS = \(PADTMP\,POK\,pPOK\) + PV = $ADDR "el"\\0 + CUR = 2 + LEN = 20 +SUBSTR + SKIP​: { skip "Not built with usemymalloc"\, 2 unless $Config{usemymalloc} eq 'y';

p5pRT commented 11 years ago

From @cpansprout

On Mon Aug 12 12​:24​:53 2013\, Hugmeir wrote​:

This is unrelated to the thread\, but I just wanted to say that looking over your attached patch made me go "Oh\, so THAT'S how you create a custom op!" Thanks\, Father C! This was educational.

I just copied bits of XS​::APItest and ck_entersub_args_proto. :-)

--

Father Chrysostomos

p5pRT commented 11 years ago

From [Unknown Contact. See original ticket]

On Mon Aug 12 12​:24​:53 2013\, Hugmeir wrote​:

This is unrelated to the thread\, but I just wanted to say that looking over your attached patch made me go "Oh\, so THAT'S how you create a custom op!" Thanks\, Father C! This was educational.

I just copied bits of XS​::APItest and ck_entersub_args_proto. :-)

--

Father Chrysostomos

p5pRT commented 11 years ago

From @ikegami

On Mon\, Aug 12\, 2013 at 3​:24 PM\, Brian Fraser \fraserbn@&#8203;gmail\.com wrote​:

This is unrelated to the thread\, but I just wanted to say that looking over your attached patch made me go "Oh\, so THAT'S how you create a custom op!" Thanks\, Father C! This was educational.

See also​: Syntax​::Feature​::Loop http​://search.cpan.org/perldoc?Syntax​::Feature​::Loop

p5pRT commented 11 years ago

From @ikegami

oops\, nevermind. That demonstrates custom keywords\, custom parsing\, inlining of subs and lexical scoping of subs\, but not custom ops.

On Mon\, Aug 12\, 2013 at 3​:49 PM\, Eric Brine \ikegami@&#8203;adaelis\.com wrote​:

On Mon\, Aug 12\, 2013 at 3​:24 PM\, Brian Fraser \fraserbn@&#8203;gmail\.com wrote​:

This is unrelated to the thread\, but I just wanted to say that looking over your attached patch made me go "Oh\, so THAT'S how you create a custom op!" Thanks\, Father C! This was educational.

See also​: Syntax​::Feature​::Loop http​://search.cpan.org/perldoc?Syntax​::Feature​::Loop

p5pRT commented 11 years ago

From @cpansprout

On Mon Aug 12 02​:00​:18 2013\, sprout wrote​:

On Sat Jul 27 11​:27​:57 2013\, sprout wrote​:

I did not fix this bug for calls to XSUBs\, because it will make Devel​::Peek less useful. Calling Dump("$x") and being able to see what pp_stringify is return[ing] is a useful feature.

(Here I’m referring to the original bug reported\, not the newRV issue. Calling foo("$x") allowed the foo sub to see a TARG in $_[0]\, such that print \$_[0]\, \$_[0] would show two different addresses.)

Currently foo("$x") will make a COW copy of the TARG if it is a Perl sub\, but pass the TARG itself if it is an XSUB. So for XS code to call newRV on that is wrong (or what newRV does is wrong).

I think the real solution here is to apply the fix to XSUBs (copy TARG arguments) but modify Devel​::Peek to inline itself\, so that it remains exempt.

That way the only XS code having access to TARGs will be that which rummages through pads. For most XS code this will just dwim.

I have made that change in commit 3455055.

In fact\, having Devel​::Peek inline itself would be useful for seeing the return value of substr and vec in rvalue context\, something currently not possible.

Attached is a patch to get Devel​::Peek to inline itself. It also allows Dump %hash and Dump @​array.

And I applied that patch\, with a slight tweak (with one arg it was leaking ops)\, as 34b94bc4d.

--

Father Chrysostomos

p5pRT commented 11 years ago

From [Unknown Contact. See original ticket]

On Mon Aug 12 02​:00​:18 2013\, sprout wrote​:

On Sat Jul 27 11​:27​:57 2013\, sprout wrote​:

I did not fix this bug for calls to XSUBs\, because it will make Devel​::Peek less useful. Calling Dump("$x") and being able to see what pp_stringify is return[ing] is a useful feature.

(Here I’m referring to the original bug reported\, not the newRV issue. Calling foo("$x") allowed the foo sub to see a TARG in $_[0]\, such that print \$_[0]\, \$_[0] would show two different addresses.)

Currently foo("$x") will make a COW copy of the TARG if it is a Perl sub\, but pass the TARG itself if it is an XSUB. So for XS code to call newRV on that is wrong (or what newRV does is wrong).

I think the real solution here is to apply the fix to XSUBs (copy TARG arguments) but modify Devel​::Peek to inline itself\, so that it remains exempt.

That way the only XS code having access to TARGs will be that which rummages through pads. For most XS code this will just dwim.

I have made that change in commit 3455055.

In fact\, having Devel​::Peek inline itself would be useful for seeing the return value of substr and vec in rvalue context\, something currently not possible.

Attached is a patch to get Devel​::Peek to inline itself. It also allows Dump %hash and Dump @​array.

And I applied that patch\, with a slight tweak (with one arg it was leaking ops)\, as 34b94bc4d.

--

Father Chrysostomos

p5pRT commented 11 years ago

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

p5pRT commented 11 years ago

From @bulk88

On Tue Aug 13 13​:46​:33 2013\, sprout wrote​:

And I applied that patch\, with a slight tweak (with one arg it was leaking ops)\, as 34b94bc4d.

The patch looks inefficient. Every XSUB call now has a larger overhead.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 11 years ago

From @ikegami

On Wed\, Aug 14\, 2013 at 11​:24 AM\, bulk88 via RT \perlbug\-followup@&#8203;perl\.orgwrote​:

On Tue Aug 13 13​:46​:33 2013\, sprout wrote​:

And I applied that patch\, with a slight tweak (with one arg it was leaking ops)\, as 34b94bc4d.

The patch looks inefficient. Every XSUB call now has a larger overhead.

Why do you think so? Do you think S_ck_dump gets called for every XSUB call? Isn't it only called for calls Devel​::Peek​::Dump (and imports thereof)?

+ CV * const cv = get_cvn_flags("Devel​::Peek​::Dump"\, 17\, 0); + cv_set_call_checker(cv\, S_ck_dump\, (SV *)cv);

p5pRT commented 11 years ago

From @bulk88

On Wed Aug 14 09​:11​:42 2013\, ikegami@​adaelis.com wrote​:

On Wed\, Aug 14\, 2013 at 11​:24 AM\, bulk88 via RT \perlbug\-followup@&#8203;perl\.orgwrote​:

On Tue Aug 13 13​:46​:33 2013\, sprout wrote​:

And I applied that patch\, with a slight tweak (with one arg it was leaking ops)\, as 34b94bc4d.

The patch looks inefficient. Every XSUB call now has a larger overhead.

Why do you think so? Do you think S_ck_dump gets called for every XSUB call? Isn't it only called for calls Devel​::Peek​::Dump (and imports thereof)?

+ CV * const cv = get_cvn_flags("Devel​::Peek​::Dump"\, 17\, 0); + cv_set_call_checker(cv\, S_ck_dump\, (SV *)cv);

Whoops\, I meant the patch in http​://perl5.git.perl.org/perl.git/commit/3455055faace06645b99a6ed63fce90144ab47e1 which claims to be part of this ticket. I didn't compare the SHA1s.

-- bulk88 ~ bulk88 at hotmail.com

p5pRT commented 11 years ago

From @cpansprout

On Wed Aug 14 09​:27​:13 2013\, bulk88 wrote​:

On Wed Aug 14 09​:11​:42 2013\, ikegami@​adaelis.com wrote​:

On Wed\, Aug 14\, 2013 at 11​:24 AM\, bulk88 via RT \perlbug\-followup@&#8203;perl\.orgwrote​:

On Tue Aug 13 13​:46​:33 2013\, sprout wrote​:

And I applied that patch\, with a slight tweak (with one arg it was leaking ops)\, as 34b94bc4d.

The patch

(He means 3455055fa; see below.)

looks inefficient. Every XSUB call now has a larger overhead.

True\, but the overhead is minuscule\, just a quick scan and flag check in most cases. This is less than the argument-processing overhead of most XSUBs.

An alternative would be to fix this bug by having newRV copy anything marked PADTMP\, but then newRV_noinc would have to SvREFCNT_dec its argument\, leaving the caller with a pointer to a freed SV. That would result in subtle bugs hard to diagnose.

Another way would be to modify ops that return PADTMPs not to return them in lvalue context to begin with\, but that would shift the complexity elsewhere into many pp_ functions\, including pp_const which is hotter than pp_entersub.

Why do you think so? Do you think S_ck_dump gets called for every XSUB call? Isn't it only called for calls Devel​::Peek​::Dump (and imports thereof)?

+ CV * const cv = get_cvn_flags("Devel​::Peek​::Dump"\, 17\, 0); + cv_set_call_checker(cv\, S_ck_dump\, (SV *)cv);

Whoops\, I meant the patch in

http​://perl5.git.perl.org/perl.git/commit/3455055faace06645b99a6ed63fce90144ab47e1 which claims to be part of this ticket. I didn't compare the SHA1s.

--

Father Chrysostomos

p5pRT commented 11 years ago

From [Unknown Contact. See original ticket]

On Wed Aug 14 09​:27​:13 2013\, bulk88 wrote​:

On Wed Aug 14 09​:11​:42 2013\, ikegami@​adaelis.com wrote​:

On Wed\, Aug 14\, 2013 at 11​:24 AM\, bulk88 via RT \perlbug\-followup@&#8203;perl\.orgwrote​:

On Tue Aug 13 13​:46​:33 2013\, sprout wrote​:

And I applied that patch\, with a slight tweak (with one arg it was leaking ops)\, as 34b94bc4d.

The patch

(He means 3455055fa; see below.)

looks inefficient. Every XSUB call now has a larger overhead.

True\, but the overhead is minuscule\, just a quick scan and flag check in most cases. This is less than the argument-processing overhead of most XSUBs.

An alternative would be to fix this bug by having newRV copy anything marked PADTMP\, but then newRV_noinc would have to SvREFCNT_dec its argument\, leaving the caller with a pointer to a freed SV. That would result in subtle bugs hard to diagnose.

Another way would be to modify ops that return PADTMPs not to return them in lvalue context to begin with\, but that would shift the complexity elsewhere into many pp_ functions\, including pp_const which is hotter than pp_entersub.

Why do you think so? Do you think S_ck_dump gets called for every XSUB call? Isn't it only called for calls Devel​::Peek​::Dump (and imports thereof)?

+ CV * const cv = get_cvn_flags("Devel​::Peek​::Dump"\, 17\, 0); + cv_set_call_checker(cv\, S_ck_dump\, (SV *)cv);

Whoops\, I meant the patch in

http​://perl5.git.perl.org/perl.git/commit/3455055faace06645b99a6ed63fce90144ab47e1 which claims to be part of this ticket. I didn't compare the SHA1s.

--

Father Chrysostomos