Skip Menu |
Report information
Id: 122405
Status: rejected
Priority: 0/
Queue: perl5

Owner: tonyc <tony [at] develop-help.com>
Requestors: creaktive [at] gmail.com
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: low
Type:
Perl Version: (no value)
Fixed In: (no value)

Attachments
0001-Proper-naming-for-the-new-helper-functions-more-newS.patch
0001-Started-work-on-Remove-the-use-of-SVs-as-temporaries.patch
0002-Cleanup-whitespace-error-detected-by-git-am.patch
0003-Proper-naming-for-the-new-helper-functions-more-newS.patch



Subject: [PATCH] Started work on "Remove the use of SVs as temporaries in dump.c"
From: Stanislaw Pusep <creaktive [...] gmail.com>
Date: Fri, 25 Jul 2014 18:10:18 +0200
To: perlbug [...] perl.org
Download (untitled) / with headers
text/plain 55.5k

Message body is not shown because it is too large.

Message body is not shown because it is too large.

Subject: Re: [perl #122405] perlbug AutoReply: [PATCH] Started work on "Remove the use of SVs as temporaries in dump.c"
To: perlbug-followup [...] perl.org
Date: Fri, 25 Jul 2014 18:18:25 +0200
From: Stanislaw Pusep <creaktive [...] gmail.com>
Download (untitled) / with headers
text/plain 111b
Sorry about the GMail noise. Patch also available here: https://gist.github.com/creaktive/14081a74a13728bab7dc
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.5k
On Fri Jul 25 09:11:04 2014, creaktive@gmail.com wrote: Show quoted text
> Picked this yak from Porting/todo.pod :) > This is a work in progress, some functions still use newSV*. > Also, a cleanup is planned (_sv_catpv/_sv_cpypv macros will be removed). > --- > AUTHORS | 1 + > dump.c | 599 > +++++++++++++++++++++++++++++++++++--------------------------- > embed.fnc | 1 + > embed.h | 1 + > proto.h | 6 + > utf8.c | 118 ++++++++----- > 6 files changed, 421 insertions(+), 305 deletions(-) >
Thank you for taking this task on. I have created a smoke-testing branch for it in our git repository: smoke-me/jkeenan/122405-dump-c-cleanup You indicate that this is a work-in-progress. I would encourage you to send future patches as attachments to this ticket at rt.perl.org or as attachments to posts to the p5p list. I.e., avoid inlines. Porters: Because this patch is (a) large, (b) presumably the first of several, and (c) touches the internals, we should develop a plan for its review. That means we have to call upon people who understand what dump.c does and why this cleanup is recommended in Porting/todo.pod. When applied in a branch (with one whitespace correction), the patch passes all tests for me on Linux/x86_64. But I don't know whether we currently have any tests that directly exercise dump.c, so I can't say that that PASS is a green light to proceed. Bugs due to changes in the internals sometimes do not materialize for a year or more. How, then, shall we proceed? Thank you very much. Jim Keenan
From: Dave Mitchell <davem [...] iabyn.com>
Date: Sat, 26 Jul 2014 20:51:41 +0100
Subject: Re: [perl #122405] [PATCH] Started work on "Remove the use of SVs as temporaries in dump.c"
To: perl5-porters [...] perl.org
On Fri, Jul 25, 2014 at 09:11:05AM -0700, Stanislaw Pusep wrote: I had a cursory glance at this diff, and noticed that you've added several functions with names beginning with an underscore, e.g. Show quoted text
> +_pv_escape( pTHX_ char *dsv, char const * const str, > + const STRLEN count, const STRLEN max, > + STRLEN * const escaped, const U32 flags )
This isn't a generally done practise within the perl core. Are these supposed to be private helper functions local to dump.c? In which case, they should be declared static, and have names starting with S_; but not with a name that clashes with Perl_ - i.e. you shouldn't have both S_foo() and Perl_foo(). -- "But Sidley Park is already a picture, and a most amiable picture too. The slopes are green and gentle. The trees are companionably grouped at intervals that show them to advantage. The rill is a serpentine ribbon unwound from the lake peaceably contained by meadows on which the right amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"
Date: Mon, 28 Jul 2014 11:08:35 +0200
To: perlbug-followup [...] perl.org
From: Stanislaw Pusep <creaktive [...] gmail.com>
Subject: Re: [perl #122405] [PATCH] Started work on "Remove the use of SVs as temporaries in dump.c"
Download (untitled) / with headers
text/plain 582b
@James:
Sorry about the inlining, will send the patches as attachments from now on :)
The test that more-or-less directly exercises dump.c is ext/Devel-Peek/t/Peek.t
Also, my initial changes to utf8.c managed to break these tests:
t/uni/gv.t
t/uni/variables.t
lib/warnings.t
So, if any of these reports as failing, that was probably caused by my changes. BTW, I've tested under Darwin/x86_64, for threaded/non-threaded & debug/non-debug builds.

@Dave:
Thanks for pointing this out, I was quite confused with the naming for these private helpers. Will rename in the upcoming patch.
Download (untitled) / with headers
text/plain 642b
OK, here's the second part. Also, I am confused about Perl_sv_peek(). In a nutshell, this is how it looks like: char *Perl_sv_peek(pTHX_ SV *sv) { dVAR; SV * const t = sv_newmortal(); /* do all kind of stuff with "t" */ return SvPV_nolen(t); } What happens to "t", then? Isn't the reference to it lost, since we only return the pointer to it's payload? From what I see, the pattern of usage of Perl_sv_peek (AKA SvPEEK) is: newSVpv(Perl_sv_peek(aTHX_ sv), 0); Would it be an acceptable idea to make Perl_sv_peek store the data in a static buffer? (sort of: what's worse, leaking memory or being dangerously thread-unsafe?)
Subject: 0001-Proper-naming-for-the-new-helper-functions-more-newS.patch
From 2cda05049b8b42e037a3f13f00ea9e81603b5eff Mon Sep 17 00:00:00 2001 From: Stanislaw Pusep <creaktive@gmail.com> Date: Mon, 28 Jul 2014 12:21:00 +0200 Subject: [PATCH] Proper naming for the new helper functions; more newSVpv() references removed. Perl_sv_peek() now operates internally without allocating new SVs. But then, a single SV is allocated for the result so we can return SvPV_nolen(). Does that configure a memory leak? Perl_sv_peek() already works that way. --- dump.c | 151 ++++++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 83 insertions(+), 68 deletions(-) diff --git a/dump.c b/dump.c index 5f81704..9b80afa 100644 --- a/dump.c +++ b/dump.c @@ -90,7 +90,7 @@ S_append_flags(char *s, U32 flags, const struct flag_to_name *start, #define append_flags(s, f, flags) \ S_append_flags((s), (f), (flags), C_ARRAY_END(flags)) -#define generic_pv_escape(sv,s,len,utf8) _pv_escape( aTHX_ (sv), (s), (len), \ +#define generic_pv_escape(sv,s,len,utf8) S_pv_escape( aTHX_ (sv), (s), (len), \ (len) * (4+UTF8_MAXBYTES) + 1, NULL, \ PERL_PV_ESCAPE_NONASCII | PERL_PV_ESCAPE_DWIM \ | ((utf8) ? PERL_PV_ESCAPE_UNI : 0) ) @@ -137,8 +137,8 @@ Returns a pointer to the escaped text as held by dsv. */ #define PV_ESCAPE_OCTBUFSIZE 32 -char * -_pv_escape( pTHX_ char *dsv, char const * const str, +static char * +S_pv_escape( pTHX_ char *dsv, char const * const str, const STRLEN count, const STRLEN max, STRLEN * const escaped, const U32 flags ) { @@ -256,7 +256,7 @@ Perl_pv_escape( pTHX_ SV *dsv, char const * const str, } Newxz(buf, DO_SV_DUMP_BUFSIZE, char); - sv_catpv(dsv, _pv_escape(aTHX_ buf, str, count, max, escaped, flags)); + sv_catpv(dsv, S_pv_escape(aTHX_ buf, str, count, max, escaped, flags)); Safefree(buf); return SvPVX(dsv); @@ -286,8 +286,8 @@ Returns a pointer to the prettified text as held by dsv. =cut */ -char * -_pv_pretty( pTHX_ char *dsv, char const * const str, const STRLEN count, +static char * +S_pv_pretty( pTHX_ char *dsv, char const * const str, const STRLEN count, const STRLEN max, char const * const start_color, char const * const end_color, const U32 flags ) { @@ -302,7 +302,7 @@ _pv_pretty( pTHX_ char *dsv, char const * const str, const STRLEN count, if ( start_color != NULL ) _sv_catpv(dsv, start_color); - _pv_escape( aTHX_ dsv, str, count, max, &escaped, flags ); + S_pv_escape( aTHX_ dsv, str, count, max, &escaped, flags ); if ( end_color != NULL ) _sv_catpv(dsv, end_color); @@ -332,7 +332,7 @@ Perl_pv_pretty( pTHX_ SV *dsv, char const * const str, const STRLEN count, } Newxz(buf, DO_SV_DUMP_BUFSIZE, char); - sv_catpv(dsv, _pv_pretty(aTHX_ buf, str, count, max, start_color, end_color, flags)); + sv_catpv(dsv, S_pv_pretty(aTHX_ buf, str, count, max, start_color, end_color, flags)); Safefree(buf); return SvPVX(dsv); @@ -352,11 +352,11 @@ Note that the final string may be up to 7 chars longer than pvlim. =cut */ -char * -_pv_display( pTHX_ char *dsv, const char *pv, STRLEN cur, STRLEN len, STRLEN pvlim) +static char * +S_pv_display( pTHX_ char *dsv, const char *pv, STRLEN cur, STRLEN len, STRLEN pvlim) { _sv_setpv(dsv, ""); - _pv_pretty( aTHX_ dsv, pv, cur, pvlim, NULL, NULL, PERL_PV_PRETTY_DUMP); + S_pv_pretty( aTHX_ dsv, pv, cur, pvlim, NULL, NULL, PERL_PV_PRETTY_DUMP); if (len > cur && pv[cur] == '\0') _sv_catpv( dsv, "\\0"); return dsv; @@ -369,14 +369,14 @@ Perl_pv_display(pTHX_ SV *dsv, const char *pv, STRLEN cur, STRLEN len, STRLEN pv PERL_ARGS_ASSERT_PV_DISPLAY; Newxz(buf, DO_SV_DUMP_BUFSIZE, char); - sv_setpv(dsv, _pv_display(aTHX_ buf, pv, cur, len, pvlim)); + sv_setpv(dsv, S_pv_display(aTHX_ buf, pv, cur, len, pvlim)); Safefree(buf); return SvPVX(dsv); } -char * -_sv_uni_display( pTHX_ char *dest, SV *ssv, STRLEN pvlim, UV flags) +static char * +S_sv_uni_display( pTHX_ char *dest, SV *ssv, STRLEN pvlim, UV flags) { STRLEN len = SvCUR(ssv); U8 *spv = (U8 *) @@ -389,31 +389,34 @@ char * Perl_sv_peek(pTHX_ SV *sv) { dVAR; - SV * const t = sv_newmortal(); + SV * const out = sv_newmortal(); int unref = 0; U32 type; + char *s, *t; + Newx(s, DO_SV_DUMP_BUFSIZE, char); + Newx(t, DO_SV_DUMP_BUFSIZE, char); - sv_setpvs(t, ""); + _sv_setpv(t, ""); retry: if (!sv) { - sv_catpv(t, "VOID"); + _sv_catpv(t, "VOID"); goto finish; } else if (sv == (const SV *)0x55555555 || ((char)SvTYPE(sv)) == 'U') { /* detect data corruption under memory poisoning */ - sv_catpv(t, "WILD"); + _sv_catpv(t, "WILD"); goto finish; } else if (sv == &PL_sv_undef || sv == &PL_sv_no || sv == &PL_sv_yes || sv == &PL_sv_placeholder) { if (sv == &PL_sv_undef) { - sv_catpv(t, "SV_UNDEF"); + _sv_catpv(t, "SV_UNDEF"); if (!(SvFLAGS(sv) & (SVf_OK|SVf_OOK|SVs_OBJECT| SVs_GMG|SVs_SMG|SVs_RMG)) && SvREADONLY(sv)) goto finish; } else if (sv == &PL_sv_no) { - sv_catpv(t, "SV_NO"); + _sv_catpv(t, "SV_NO"); if (!(SvFLAGS(sv) & (SVf_ROK|SVf_OOK|SVs_OBJECT| SVs_GMG|SVs_SMG|SVs_RMG)) && !(~SvFLAGS(sv) & (SVf_POK|SVf_NOK|SVf_READONLY| @@ -423,7 +426,7 @@ Perl_sv_peek(pTHX_ SV *sv) goto finish; } else if (sv == &PL_sv_yes) { - sv_catpv(t, "SV_YES"); + _sv_catpv(t, "SV_YES"); if (!(SvFLAGS(sv) & (SVf_ROK|SVf_OOK|SVs_OBJECT| SVs_GMG|SVs_SMG|SVs_RMG)) && !(~SvFLAGS(sv) & (SVf_POK|SVf_NOK|SVf_READONLY| @@ -434,16 +437,16 @@ Perl_sv_peek(pTHX_ SV *sv) goto finish; } else { - sv_catpv(t, "SV_PLACEHOLDER"); + _sv_catpv(t, "SV_PLACEHOLDER"); if (!(SvFLAGS(sv) & (SVf_OK|SVf_OOK|SVs_OBJECT| SVs_GMG|SVs_SMG|SVs_RMG)) && SvREADONLY(sv)) goto finish; } - sv_catpv(t, ":"); + _sv_catpv(t, ":"); } else if (SvREFCNT(sv) == 0) { - sv_catpv(t, "("); + _sv_catpv(t, "("); unref++; } else if (DEBUG_R_TEST_) { @@ -456,19 +459,19 @@ Perl_sv_peek(pTHX_ SV *sv) break; } } - if (SvREFCNT(sv) > 1) - Perl_sv_catpvf(aTHX_ t, "<%"UVuf"%s>", (UV)SvREFCNT(sv), - is_tmp ? "T" : ""); - else if (is_tmp) - sv_catpv(t, "<T>"); + if (SvREFCNT(sv) > 1) { + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "<%"UVuf"%s>", (UV)SvREFCNT(sv), + is_tmp ? "T" : ""); + _sv_catpv(t, s); + } else if (is_tmp) + _sv_catpv(t, "<T>"); } if (SvROK(sv)) { - sv_catpv(t, "\\"); - if (SvCUR(t) + unref > 10) { - SvCUR_set(t, unref + 3); - *SvEND(t) = '\0'; - sv_catpv(t, "..."); + _sv_catpv(t, "\\"); + if (strlen(t) + unref > 10) { + t[unref + 4] = '\0'; + _sv_catpv(t, "..."); goto finish; } sv = SvRV(sv); @@ -478,60 +481,71 @@ Perl_sv_peek(pTHX_ SV *sv) if (type == SVt_PVCV) { GV* gvcv = CvGV(sv); char *tmp; Newxz(tmp, DO_SV_DUMP_BUFSIZE, char); - Perl_sv_catpvf(aTHX_ t, "CV(%s)", gvcv + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "CV(%s)", gvcv ? generic_pv_escape( tmp, GvNAME(gvcv), GvNAMELEN(gvcv), GvNAMEUTF8(gvcv)) : ""); + _sv_catpv(t, s); Safefree(tmp); goto finish; } else if (type < SVt_LAST) { - sv_catpv(t, svshorttypenames[type]); + _sv_catpv(t, svshorttypenames[type]); if (type == SVt_NULL) goto finish; } else { - sv_catpv(t, "FREED"); + _sv_catpv(t, "FREED"); goto finish; } if (SvPOKp(sv)) { if (!SvPVX_const(sv)) - sv_catpv(t, "(null)"); + _sv_catpv(t, "(null)"); else { - SV * const tmp = newSVpvs(""); - sv_catpv(t, "("); + char *tmp; Newxz(tmp, DO_SV_DUMP_BUFSIZE, char); + _sv_catpv(t, "("); if (SvOOK(sv)) { STRLEN delta; SvOOK_offset(sv, delta); - Perl_sv_catpvf(aTHX_ t, "[%s]", pv_display(tmp, SvPVX_const(sv)-delta, delta, 0, 127)); + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "[%s]", S_pv_display(tmp, SvPVX_const(sv)-delta, delta, 0, 127)); + _sv_catpv(t, s); } - Perl_sv_catpvf(aTHX_ t, "%s)", pv_display(tmp, SvPVX_const(sv), SvCUR(sv), SvLEN(sv), 127)); - if (SvUTF8(sv)) - Perl_sv_catpvf(aTHX_ t, " [UTF8 \"%s\"]", - sv_uni_display(tmp, sv, 6 * SvCUR(sv), - UNI_DISPLAY_QQ)); - SvREFCNT_dec_NN(tmp); + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "%s)", S_pv_display(tmp, SvPVX_const(sv), SvCUR(sv), SvLEN(sv), 127)); + _sv_catpv(t, s); + if (SvUTF8(sv)) { + my_snprintf(s, DO_SV_DUMP_BUFSIZE, " [UTF8 \"%s\"]", + S_sv_uni_display(tmp, sv, 6 * SvCUR(sv), + UNI_DISPLAY_QQ)); + _sv_catpv(t, s); + } + Safefree(tmp); } } else if (SvNOKp(sv)) { STORE_NUMERIC_LOCAL_SET_STANDARD(); - Perl_sv_catpvf(aTHX_ t, "(%"NVgf")",SvNVX(sv)); + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "(%"NVgf")",SvNVX(sv)); + _sv_catpv(t, s); RESTORE_NUMERIC_LOCAL(); } else if (SvIOKp(sv)) { - if (SvIsUV(sv)) - Perl_sv_catpvf(aTHX_ t, "(%"UVuf")", (UV)SvUVX(sv)); - else - Perl_sv_catpvf(aTHX_ t, "(%"IVdf")", (IV)SvIVX(sv)); + if (SvIsUV(sv)) + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "(%"UVuf")", (UV)SvUVX(sv)); + else + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "(%"IVdf")", (IV)SvIVX(sv)); + _sv_catpv(t, s); } else - sv_catpv(t, "()"); + _sv_catpv(t, "()"); finish: while (unref--) - sv_catpv(t, ")"); + _sv_catpv(t, ")"); if (TAINTING_get && sv && SvTAINTED(sv)) - sv_catpv(t, " [tainted]"); - return SvPV_nolen(t); + _sv_catpv(t, " [tainted]"); + + sv_setpv(out, t); + Safefree(s); + Safefree(t); + return SvPV_nolen(out); } /* @@ -1365,13 +1379,14 @@ Perl_do_magic_dump(pTHX_ I32 level, PerlIO *file, const MAGIC *mg, I32 nest, I32 PTR2UV(mg->mg_obj)); if (mg->mg_type == PERL_MAGIC_qr) { REGEXP* const re = (REGEXP *)mg->mg_obj; - SV * const dsv = sv_newmortal(); - const char * const s - = pv_pretty(dsv, RX_WRAPPED(re), RX_WRAPLEN(re), + char *s; + char *tmp; Newxz(tmp, DO_SV_DUMP_BUFSIZE, char); + s = S_pv_pretty(tmp, RX_WRAPPED(re), RX_WRAPLEN(re), 60, NULL, NULL, ( PERL_PV_PRETTY_QUOTE | PERL_PV_ESCAPE_RE | PERL_PV_PRETTY_ELLIPSES | (RX_UTF8(re) ? PERL_PV_ESCAPE_UNI : 0)) ); + Safefree(tmp); Perl_dump_indent(aTHX_ level+1, file, " PAT = %s\n", s); Perl_dump_indent(aTHX_ level+1, file, " REFCNT = %"IVdf"\n", (IV)RX_REFCNT(re)); @@ -1385,9 +1400,9 @@ Perl_do_magic_dump(pTHX_ I32 level, PerlIO *file, const MAGIC *mg, I32 nest, I32 Perl_dump_indent(aTHX_ level, file, " MG_PTR = 0x%"UVxf, PTR2UV(mg->mg_ptr)); if (mg->mg_len >= 0) { if (mg->mg_type != PERL_MAGIC_utf8) { - SV * const sv = newSVpvs(""); - PerlIO_printf(file, " %s", pv_display(sv, mg->mg_ptr, mg->mg_len, 0, pvlim)); - SvREFCNT_dec_NN(sv); + char *tmp; Newxz(tmp, DO_SV_DUMP_BUFSIZE, char); + PerlIO_printf(file, " %s", S_pv_display(tmp, mg->mg_ptr, mg->mg_len, 0, pvlim)); + Safefree(tmp); } } else if (mg->mg_len == HEf_SVKEY) { @@ -1806,7 +1821,7 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo Perl_dump_indent(aTHX_ level, file," PV = 0x%"UVxf" ", PTR2UV(ptr)); if (SvOOK(sv)) { PerlIO_printf(file, "( %s . ) ", - _pv_display(aTHX_ d, ptr - delta, delta, 0, + S_pv_display(aTHX_ d, ptr - delta, delta, 0, pvlim)); } if (type == SVt_INVLIST) { @@ -1815,12 +1830,12 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo _invlist_dump(file, level, " ", sv); } else { - PerlIO_printf(file, "%s", _pv_display(aTHX_ d, ptr, SvCUR(sv), + PerlIO_printf(file, "%s", S_pv_display(aTHX_ d, ptr, SvCUR(sv), re ? 0 : SvLEN(sv), pvlim)); if (SvUTF8(sv)) /* the 6? \x{....} */ PerlIO_printf(file, " [UTF8 \"%s\"]", - _sv_uni_display(aTHX_ d, sv, 6 * SvCUR(sv), + S_sv_uni_display(aTHX_ d, sv, 6 * SvCUR(sv), UNI_DISPLAY_QQ)); PerlIO_printf(file, "\n"); } @@ -2124,9 +2139,9 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo keypv = SvPV_const(keysv, len); elt = HeVAL(he); - Perl_dump_indent(aTHX_ level+1, file, "Elt %s ", _pv_display(aTHX_ d, keypv, len, 0, pvlim)); + Perl_dump_indent(aTHX_ level+1, file, "Elt %s ", S_pv_display(aTHX_ d, keypv, len, 0, pvlim)); if (SvUTF8(keysv)) - PerlIO_printf(file, "[UTF8 \"%s\"] ", _sv_uni_display(aTHX_ d, keysv, 6 * SvCUR(keysv), UNI_DISPLAY_QQ)); + PerlIO_printf(file, "[UTF8 \"%s\"] ", S_sv_uni_display(aTHX_ d, keysv, 6 * SvCUR(keysv), UNI_DISPLAY_QQ)); if (HvEITER_get(hv) == he) PerlIO_printf(file, "[CURRENT] "); PerlIO_printf(file, "HASH = 0x%"UVxf"\n", (UV) hash); @@ -2362,7 +2377,7 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo if (r->subbeg) Perl_dump_indent(aTHX_ level, file, " SUBBEG = 0x%"UVxf" %s\n", PTR2UV(r->subbeg), - _pv_display(aTHX_ d, r->subbeg, r->sublen, 50, pvlim)); + S_pv_display(aTHX_ d, r->subbeg, r->sublen, 50, pvlim)); else Perl_dump_indent(aTHX_ level, file, " SUBBEG = 0x0\n"); Perl_dump_indent(aTHX_ level, file, " MOTHER_RE = 0x%"UVxf"\n", -- 2.0.1
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.8k
On Mon Jul 28 06:08:52 2014, creaktive@gmail.com wrote: Show quoted text
> OK, here's the second part. > Also, I am confused about Perl_sv_peek(). In a nutshell, this is how > it looks like: > > char *Perl_sv_peek(pTHX_ SV *sv) { > dVAR; > SV * const t = sv_newmortal(); > /* do all kind of stuff with "t" */ > return SvPV_nolen(t); > } > > What happens to "t", then? Isn't the reference to it lost, since we > only return the pointer to it's payload? > From what I see, the pattern of usage of Perl_sv_peek (AKA SvPEEK) is: > > newSVpv(Perl_sv_peek(aTHX_ sv), 0); > > Would it be an acceptable idea to make Perl_sv_peek store the data in > a static buffer? > (sort of: what's worse, leaking memory or being dangerously thread- > unsafe?)
Although I was able to apply the second locally, I got conflicts when I attempted to push it to the smoke-me branch. ##### [perl] 69 $ git push -f origin 122405-dump-c-cleanup:smoke-me/jkeenan/122405-dump-c-cleanup Counting objects: 22, done. Delta compression using up to 4 threads. Compressing objects: 100% (4/4), done. Writing objects: 100% (4/4), 4.44 KiB | 0 bytes/s, done. Total 4 (delta 2), reused 0 (delta 0) remote: error: denying non-fast-forward refs/heads/smoke-me/jkeenan/122405-dump-c-cleanup (you should pull first) To jkeenan@perl5.git.perl.org:/gitroot/perl.git ! [remote rejected] 122405-dump-c-cleanup -> smoke-me/jkeenan/122405-dump-c-cleanup (non-fast-forward) error: failed to push some refs to 'jkeenan@perl5.git.perl.org:/gitroot/perl.git' [perl] 70 $ vi .git/config [perl] 71 $ git pull origin smoke-me/jkeenan/122405-dump-c-cleanup From perl5.git.perl.org:/gitroot/perl * branch smoke-me/jkeenan/122405-dump-c-cleanup -> FETCH_HEAD Auto-merging dump.c CONFLICT (content): Merge conflict in dump.c Automatic merge failed; fix conflicts and then commit the result. #####
Date: Mon, 4 Aug 2014 14:24:02 +0200
To: perlbug-followup [...] perl.org
From: Stanislaw Pusep <creaktive [...] gmail.com>
Subject: Re: [perl #122405] [PATCH] Started work on "Remove the use of SVs as temporaries in dump.c"
Download (untitled) / with headers
text/plain 2.2k
Sorry about my clumsiness, I've literally discovered "git am" 10 days ago. May I use GitHub mirror? I am more familiar with that tool.


On Mon, Aug 4, 2014 at 1:53 PM, James E Keenan via RT <perlbug-followup@perl.org> wrote:
Show quoted text
On Mon Jul 28 06:08:52 2014, creaktive@gmail.com wrote:
> OK, here's the second part.
> Also, I am confused about Perl_sv_peek(). In a nutshell, this is how
> it looks like:
>
> char *Perl_sv_peek(pTHX_ SV *sv) {
>     dVAR;
>     SV * const t = sv_newmortal();
>     /* do all kind of stuff with "t" */
>     return SvPV_nolen(t);
> }
>
> What happens to "t", then? Isn't the reference to it lost, since we
> only return the pointer to it's payload?
> From what I see, the pattern of usage of Perl_sv_peek (AKA SvPEEK) is:
>
> newSVpv(Perl_sv_peek(aTHX_ sv), 0);
>
> Would it be an acceptable idea to make Perl_sv_peek store the data in
> a static buffer?
> (sort of: what's worse, leaking memory or being dangerously thread-
> unsafe?)


Although I was able to apply the second locally, I got conflicts when I attempted to push it to the smoke-me branch.

#####
[perl] 69 $ git push -f origin 122405-dump-c-cleanup:smoke-me/jkeenan/122405-dump-c-cleanup
Counting objects: 22, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (4/4), done.
Writing objects: 100% (4/4), 4.44 KiB | 0 bytes/s, done.
Total 4 (delta 2), reused 0 (delta 0)
remote: error: denying non-fast-forward refs/heads/smoke-me/jkeenan/122405-dump-c-cleanup (you should pull first)
To jkeenan@perl5.git.perl.org:/gitroot/perl.git
 ! [remote rejected] 122405-dump-c-cleanup -> smoke-me/jkeenan/122405-dump-c-cleanup (non-fast-forward)
error: failed to push some refs to 'jkeenan@perl5.git.perl.org:/gitroot/perl.git'
[perl] 70 $ vi .git/config
[perl] 71 $ git pull origin smoke-me/jkeenan/122405-dump-c-cleanup
From perl5.git.perl.org:/gitroot/perl
 * branch            smoke-me/jkeenan/122405-dump-c-cleanup -> FETCH_HEAD
Auto-merging dump.c
CONFLICT (content): Merge conflict in dump.c
Automatic merge failed; fix conflicts and then commit the result.
#####

From: James E Keenan <jkeen [...] verizon.net>
Date: Mon, 04 Aug 2014 18:37:29 -0400
To: perl5-porters [...] perl.org
Subject: Re: [perl #122405] [PATCH] Started work on "Remove the use of SVs astemporaries in dump.c"
Download (untitled) / with headers
text/plain 884b
On 08/04/2014 08:24 AM, Stanislaw Pusep wrote: Show quoted text
> Sorry about my clumsiness, I've literally discovered "git am" 10 days > ago. May I use GitHub mirror? I am more familiar with that tool.
'git am' is what we would use to apply your patch. What you probably want is 'git format-patch'. It creates one diff file -- in a format well suited for 'git am' on the committer side -- for each commit since a designated one. So let's suppose that your starting point was commit 0bfc07433cfcdda355e0. To create a patch(es) and place them in /home/ spusep/uploads, you would say: git format-patch -o /home/spusep/uploads 0bfc07433cfcdda355e0 The patches would be named by prepending '0001-', '0002-', etc. to a munged version of the top line of the commit message. You would then attach them to your email or to your post via the GUI at rt.perl.org. Thank you very much. Jim Keenan
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 999b
On Mon Jul 28 06:08:52 2014, creaktive@gmail.com wrote: Show quoted text
> OK, here's the second part. > Also, I am confused about Perl_sv_peek(). In a nutshell, this is how > it looks like: > > char *Perl_sv_peek(pTHX_ SV *sv) { > dVAR; > SV * const t = sv_newmortal(); > /* do all kind of stuff with "t" */ > return SvPV_nolen(t); > } > > What happens to "t", then? Isn't the reference to it lost, since we > only return the pointer to it's payload?
sv_newmortal() creates an SV and registers it to be freed on the next (nested) FREETMPS, eg: SAVETMPS; s = sv_newmortal(); ... SAVETMPS; ... FREETMPS; ... FREETMPS; /* s released here */ Show quoted text
> From what I see, the pattern of usage of Perl_sv_peek (AKA SvPEEK) is: > > newSVpv(Perl_sv_peek(aTHX_ sv), 0); > > Would it be an acceptable idea to make Perl_sv_peek store the data in > a static buffer? > (sort of: what's worse, leaking memory or being dangerously thread- > unsafe?)
SAVEFREEPV() might be what you want here. Tony
Download (untitled) / with headers
text/plain 1.4k
Thanks for the explanation. But the .patch file generated that way is almost the same, the only thing that did changed were the first and the last line (not the diff body). Just to make sure, I've rebased all the changes on my local copy of 'smoke-me/jkeenan/122405-dump-c-cleanup' and made the .patch files that do apply to the current 'blead'. So, in the worst case, 'smoke-me/jkeenan/122405-dump-c-cleanup' could be dropped and regenerated from the latest codebase. On Mon Aug 04 15:37:54 2014, jkeen@verizon.net wrote: Show quoted text
> On 08/04/2014 08:24 AM, Stanislaw Pusep wrote:
> > Sorry about my clumsiness, I've literally discovered "git am" 10 days > > ago. May I use GitHub mirror? I am more familiar with that tool.
> > > 'git am' is what we would use to apply your patch. > > What you probably want is 'git format-patch'. It creates one diff file > -- in a format well suited for 'git am' on the committer side -- for > each commit since a designated one. > > So let's suppose that your starting point was commit > 0bfc07433cfcdda355e0. To create a patch(es) and place them in /home/ > spusep/uploads, you would say: > > git format-patch -o /home/spusep/uploads 0bfc07433cfcdda355e0 > > The patches would be named by prepending '0001-', '0002-', etc. to a > munged version of the top line of the commit message. You would then > attach them to your email or to your post via the GUI at rt.perl.org. > > Thank you very much. > Jim Keenan >
Subject: 0001-Started-work-on-Remove-the-use-of-SVs-as-temporaries.patch

Message body is not shown because it is too large.

Subject: 0002-Cleanup-whitespace-error-detected-by-git-am.patch
From 1fd5ca6aff63a7486d08d83f4254560f99b9aeba Mon Sep 17 00:00:00 2001 From: James E Keenan <jkeenan@cpan.org> Date: Sat, 26 Jul 2014 10:49:40 -0400 Subject: [PATCH 2/3] Cleanup whitespace error detected by 'git am'. --- dump.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dump.c b/dump.c index c368794..5f81704 100644 --- a/dump.c +++ b/dump.c @@ -1640,7 +1640,7 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo } append_flags(d, flags, first_sv_flags_names); if (flags & SVf_ROK) { - _sv_catpv(d, "ROK,"); + _sv_catpv(d, "ROK,"); if (SvWEAKREF(sv)) _sv_catpv(d, "WEAKREF,"); } append_flags(d, flags, second_sv_flags_names); -- 2.0.4
Subject: 0003-Proper-naming-for-the-new-helper-functions-more-newS.patch
From 78197f452327b2e737a540ef64a7e3dd167c0e57 Mon Sep 17 00:00:00 2001 From: Stanislaw Pusep <creaktive@gmail.com> Date: Mon, 28 Jul 2014 12:21:00 +0200 Subject: [PATCH 3/3] Proper naming for the new helper functions; more newSVpv() references removed. Perl_sv_peek() now operates internally without allocating new SVs. But then, a single SV is allocated for the result so we can return SvPV_nolen(). Does that configure a memory leak? Perl_sv_peek() already works that way. --- dump.c | 151 ++++++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 83 insertions(+), 68 deletions(-) diff --git a/dump.c b/dump.c index 5f81704..9b80afa 100644 --- a/dump.c +++ b/dump.c @@ -90,7 +90,7 @@ S_append_flags(char *s, U32 flags, const struct flag_to_name *start, #define append_flags(s, f, flags) \ S_append_flags((s), (f), (flags), C_ARRAY_END(flags)) -#define generic_pv_escape(sv,s,len,utf8) _pv_escape( aTHX_ (sv), (s), (len), \ +#define generic_pv_escape(sv,s,len,utf8) S_pv_escape( aTHX_ (sv), (s), (len), \ (len) * (4+UTF8_MAXBYTES) + 1, NULL, \ PERL_PV_ESCAPE_NONASCII | PERL_PV_ESCAPE_DWIM \ | ((utf8) ? PERL_PV_ESCAPE_UNI : 0) ) @@ -137,8 +137,8 @@ Returns a pointer to the escaped text as held by dsv. */ #define PV_ESCAPE_OCTBUFSIZE 32 -char * -_pv_escape( pTHX_ char *dsv, char const * const str, +static char * +S_pv_escape( pTHX_ char *dsv, char const * const str, const STRLEN count, const STRLEN max, STRLEN * const escaped, const U32 flags ) { @@ -256,7 +256,7 @@ Perl_pv_escape( pTHX_ SV *dsv, char const * const str, } Newxz(buf, DO_SV_DUMP_BUFSIZE, char); - sv_catpv(dsv, _pv_escape(aTHX_ buf, str, count, max, escaped, flags)); + sv_catpv(dsv, S_pv_escape(aTHX_ buf, str, count, max, escaped, flags)); Safefree(buf); return SvPVX(dsv); @@ -286,8 +286,8 @@ Returns a pointer to the prettified text as held by dsv. =cut */ -char * -_pv_pretty( pTHX_ char *dsv, char const * const str, const STRLEN count, +static char * +S_pv_pretty( pTHX_ char *dsv, char const * const str, const STRLEN count, const STRLEN max, char const * const start_color, char const * const end_color, const U32 flags ) { @@ -302,7 +302,7 @@ _pv_pretty( pTHX_ char *dsv, char const * const str, const STRLEN count, if ( start_color != NULL ) _sv_catpv(dsv, start_color); - _pv_escape( aTHX_ dsv, str, count, max, &escaped, flags ); + S_pv_escape( aTHX_ dsv, str, count, max, &escaped, flags ); if ( end_color != NULL ) _sv_catpv(dsv, end_color); @@ -332,7 +332,7 @@ Perl_pv_pretty( pTHX_ SV *dsv, char const * const str, const STRLEN count, } Newxz(buf, DO_SV_DUMP_BUFSIZE, char); - sv_catpv(dsv, _pv_pretty(aTHX_ buf, str, count, max, start_color, end_color, flags)); + sv_catpv(dsv, S_pv_pretty(aTHX_ buf, str, count, max, start_color, end_color, flags)); Safefree(buf); return SvPVX(dsv); @@ -352,11 +352,11 @@ Note that the final string may be up to 7 chars longer than pvlim. =cut */ -char * -_pv_display( pTHX_ char *dsv, const char *pv, STRLEN cur, STRLEN len, STRLEN pvlim) +static char * +S_pv_display( pTHX_ char *dsv, const char *pv, STRLEN cur, STRLEN len, STRLEN pvlim) { _sv_setpv(dsv, ""); - _pv_pretty( aTHX_ dsv, pv, cur, pvlim, NULL, NULL, PERL_PV_PRETTY_DUMP); + S_pv_pretty( aTHX_ dsv, pv, cur, pvlim, NULL, NULL, PERL_PV_PRETTY_DUMP); if (len > cur && pv[cur] == '\0') _sv_catpv( dsv, "\\0"); return dsv; @@ -369,14 +369,14 @@ Perl_pv_display(pTHX_ SV *dsv, const char *pv, STRLEN cur, STRLEN len, STRLEN pv PERL_ARGS_ASSERT_PV_DISPLAY; Newxz(buf, DO_SV_DUMP_BUFSIZE, char); - sv_setpv(dsv, _pv_display(aTHX_ buf, pv, cur, len, pvlim)); + sv_setpv(dsv, S_pv_display(aTHX_ buf, pv, cur, len, pvlim)); Safefree(buf); return SvPVX(dsv); } -char * -_sv_uni_display( pTHX_ char *dest, SV *ssv, STRLEN pvlim, UV flags) +static char * +S_sv_uni_display( pTHX_ char *dest, SV *ssv, STRLEN pvlim, UV flags) { STRLEN len = SvCUR(ssv); U8 *spv = (U8 *) @@ -389,31 +389,34 @@ char * Perl_sv_peek(pTHX_ SV *sv) { dVAR; - SV * const t = sv_newmortal(); + SV * const out = sv_newmortal(); int unref = 0; U32 type; + char *s, *t; + Newx(s, DO_SV_DUMP_BUFSIZE, char); + Newx(t, DO_SV_DUMP_BUFSIZE, char); - sv_setpvs(t, ""); + _sv_setpv(t, ""); retry: if (!sv) { - sv_catpv(t, "VOID"); + _sv_catpv(t, "VOID"); goto finish; } else if (sv == (const SV *)0x55555555 || ((char)SvTYPE(sv)) == 'U') { /* detect data corruption under memory poisoning */ - sv_catpv(t, "WILD"); + _sv_catpv(t, "WILD"); goto finish; } else if (sv == &PL_sv_undef || sv == &PL_sv_no || sv == &PL_sv_yes || sv == &PL_sv_placeholder) { if (sv == &PL_sv_undef) { - sv_catpv(t, "SV_UNDEF"); + _sv_catpv(t, "SV_UNDEF"); if (!(SvFLAGS(sv) & (SVf_OK|SVf_OOK|SVs_OBJECT| SVs_GMG|SVs_SMG|SVs_RMG)) && SvREADONLY(sv)) goto finish; } else if (sv == &PL_sv_no) { - sv_catpv(t, "SV_NO"); + _sv_catpv(t, "SV_NO"); if (!(SvFLAGS(sv) & (SVf_ROK|SVf_OOK|SVs_OBJECT| SVs_GMG|SVs_SMG|SVs_RMG)) && !(~SvFLAGS(sv) & (SVf_POK|SVf_NOK|SVf_READONLY| @@ -423,7 +426,7 @@ Perl_sv_peek(pTHX_ SV *sv) goto finish; } else if (sv == &PL_sv_yes) { - sv_catpv(t, "SV_YES"); + _sv_catpv(t, "SV_YES"); if (!(SvFLAGS(sv) & (SVf_ROK|SVf_OOK|SVs_OBJECT| SVs_GMG|SVs_SMG|SVs_RMG)) && !(~SvFLAGS(sv) & (SVf_POK|SVf_NOK|SVf_READONLY| @@ -434,16 +437,16 @@ Perl_sv_peek(pTHX_ SV *sv) goto finish; } else { - sv_catpv(t, "SV_PLACEHOLDER"); + _sv_catpv(t, "SV_PLACEHOLDER"); if (!(SvFLAGS(sv) & (SVf_OK|SVf_OOK|SVs_OBJECT| SVs_GMG|SVs_SMG|SVs_RMG)) && SvREADONLY(sv)) goto finish; } - sv_catpv(t, ":"); + _sv_catpv(t, ":"); } else if (SvREFCNT(sv) == 0) { - sv_catpv(t, "("); + _sv_catpv(t, "("); unref++; } else if (DEBUG_R_TEST_) { @@ -456,19 +459,19 @@ Perl_sv_peek(pTHX_ SV *sv) break; } } - if (SvREFCNT(sv) > 1) - Perl_sv_catpvf(aTHX_ t, "<%"UVuf"%s>", (UV)SvREFCNT(sv), - is_tmp ? "T" : ""); - else if (is_tmp) - sv_catpv(t, "<T>"); + if (SvREFCNT(sv) > 1) { + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "<%"UVuf"%s>", (UV)SvREFCNT(sv), + is_tmp ? "T" : ""); + _sv_catpv(t, s); + } else if (is_tmp) + _sv_catpv(t, "<T>"); } if (SvROK(sv)) { - sv_catpv(t, "\\"); - if (SvCUR(t) + unref > 10) { - SvCUR_set(t, unref + 3); - *SvEND(t) = '\0'; - sv_catpv(t, "..."); + _sv_catpv(t, "\\"); + if (strlen(t) + unref > 10) { + t[unref + 4] = '\0'; + _sv_catpv(t, "..."); goto finish; } sv = SvRV(sv); @@ -478,60 +481,71 @@ Perl_sv_peek(pTHX_ SV *sv) if (type == SVt_PVCV) { GV* gvcv = CvGV(sv); char *tmp; Newxz(tmp, DO_SV_DUMP_BUFSIZE, char); - Perl_sv_catpvf(aTHX_ t, "CV(%s)", gvcv + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "CV(%s)", gvcv ? generic_pv_escape( tmp, GvNAME(gvcv), GvNAMELEN(gvcv), GvNAMEUTF8(gvcv)) : ""); + _sv_catpv(t, s); Safefree(tmp); goto finish; } else if (type < SVt_LAST) { - sv_catpv(t, svshorttypenames[type]); + _sv_catpv(t, svshorttypenames[type]); if (type == SVt_NULL) goto finish; } else { - sv_catpv(t, "FREED"); + _sv_catpv(t, "FREED"); goto finish; } if (SvPOKp(sv)) { if (!SvPVX_const(sv)) - sv_catpv(t, "(null)"); + _sv_catpv(t, "(null)"); else { - SV * const tmp = newSVpvs(""); - sv_catpv(t, "("); + char *tmp; Newxz(tmp, DO_SV_DUMP_BUFSIZE, char); + _sv_catpv(t, "("); if (SvOOK(sv)) { STRLEN delta; SvOOK_offset(sv, delta); - Perl_sv_catpvf(aTHX_ t, "[%s]", pv_display(tmp, SvPVX_const(sv)-delta, delta, 0, 127)); + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "[%s]", S_pv_display(tmp, SvPVX_const(sv)-delta, delta, 0, 127)); + _sv_catpv(t, s); } - Perl_sv_catpvf(aTHX_ t, "%s)", pv_display(tmp, SvPVX_const(sv), SvCUR(sv), SvLEN(sv), 127)); - if (SvUTF8(sv)) - Perl_sv_catpvf(aTHX_ t, " [UTF8 \"%s\"]", - sv_uni_display(tmp, sv, 6 * SvCUR(sv), - UNI_DISPLAY_QQ)); - SvREFCNT_dec_NN(tmp); + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "%s)", S_pv_display(tmp, SvPVX_const(sv), SvCUR(sv), SvLEN(sv), 127)); + _sv_catpv(t, s); + if (SvUTF8(sv)) { + my_snprintf(s, DO_SV_DUMP_BUFSIZE, " [UTF8 \"%s\"]", + S_sv_uni_display(tmp, sv, 6 * SvCUR(sv), + UNI_DISPLAY_QQ)); + _sv_catpv(t, s); + } + Safefree(tmp); } } else if (SvNOKp(sv)) { STORE_NUMERIC_LOCAL_SET_STANDARD(); - Perl_sv_catpvf(aTHX_ t, "(%"NVgf")",SvNVX(sv)); + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "(%"NVgf")",SvNVX(sv)); + _sv_catpv(t, s); RESTORE_NUMERIC_LOCAL(); } else if (SvIOKp(sv)) { - if (SvIsUV(sv)) - Perl_sv_catpvf(aTHX_ t, "(%"UVuf")", (UV)SvUVX(sv)); - else - Perl_sv_catpvf(aTHX_ t, "(%"IVdf")", (IV)SvIVX(sv)); + if (SvIsUV(sv)) + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "(%"UVuf")", (UV)SvUVX(sv)); + else + my_snprintf(s, DO_SV_DUMP_BUFSIZE, "(%"IVdf")", (IV)SvIVX(sv)); + _sv_catpv(t, s); } else - sv_catpv(t, "()"); + _sv_catpv(t, "()"); finish: while (unref--) - sv_catpv(t, ")"); + _sv_catpv(t, ")"); if (TAINTING_get && sv && SvTAINTED(sv)) - sv_catpv(t, " [tainted]"); - return SvPV_nolen(t); + _sv_catpv(t, " [tainted]"); + + sv_setpv(out, t); + Safefree(s); + Safefree(t); + return SvPV_nolen(out); } /* @@ -1365,13 +1379,14 @@ Perl_do_magic_dump(pTHX_ I32 level, PerlIO *file, const MAGIC *mg, I32 nest, I32 PTR2UV(mg->mg_obj)); if (mg->mg_type == PERL_MAGIC_qr) { REGEXP* const re = (REGEXP *)mg->mg_obj; - SV * const dsv = sv_newmortal(); - const char * const s - = pv_pretty(dsv, RX_WRAPPED(re), RX_WRAPLEN(re), + char *s; + char *tmp; Newxz(tmp, DO_SV_DUMP_BUFSIZE, char); + s = S_pv_pretty(tmp, RX_WRAPPED(re), RX_WRAPLEN(re), 60, NULL, NULL, ( PERL_PV_PRETTY_QUOTE | PERL_PV_ESCAPE_RE | PERL_PV_PRETTY_ELLIPSES | (RX_UTF8(re) ? PERL_PV_ESCAPE_UNI : 0)) ); + Safefree(tmp); Perl_dump_indent(aTHX_ level+1, file, " PAT = %s\n", s); Perl_dump_indent(aTHX_ level+1, file, " REFCNT = %"IVdf"\n", (IV)RX_REFCNT(re)); @@ -1385,9 +1400,9 @@ Perl_do_magic_dump(pTHX_ I32 level, PerlIO *file, const MAGIC *mg, I32 nest, I32 Perl_dump_indent(aTHX_ level, file, " MG_PTR = 0x%"UVxf, PTR2UV(mg->mg_ptr)); if (mg->mg_len >= 0) { if (mg->mg_type != PERL_MAGIC_utf8) { - SV * const sv = newSVpvs(""); - PerlIO_printf(file, " %s", pv_display(sv, mg->mg_ptr, mg->mg_len, 0, pvlim)); - SvREFCNT_dec_NN(sv); + char *tmp; Newxz(tmp, DO_SV_DUMP_BUFSIZE, char); + PerlIO_printf(file, " %s", S_pv_display(tmp, mg->mg_ptr, mg->mg_len, 0, pvlim)); + Safefree(tmp); } } else if (mg->mg_len == HEf_SVKEY) { @@ -1806,7 +1821,7 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo Perl_dump_indent(aTHX_ level, file," PV = 0x%"UVxf" ", PTR2UV(ptr)); if (SvOOK(sv)) { PerlIO_printf(file, "( %s . ) ", - _pv_display(aTHX_ d, ptr - delta, delta, 0, + S_pv_display(aTHX_ d, ptr - delta, delta, 0, pvlim)); } if (type == SVt_INVLIST) { @@ -1815,12 +1830,12 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo _invlist_dump(file, level, " ", sv); } else { - PerlIO_printf(file, "%s", _pv_display(aTHX_ d, ptr, SvCUR(sv), + PerlIO_printf(file, "%s", S_pv_display(aTHX_ d, ptr, SvCUR(sv), re ? 0 : SvLEN(sv), pvlim)); if (SvUTF8(sv)) /* the 6? \x{....} */ PerlIO_printf(file, " [UTF8 \"%s\"]", - _sv_uni_display(aTHX_ d, sv, 6 * SvCUR(sv), + S_sv_uni_display(aTHX_ d, sv, 6 * SvCUR(sv), UNI_DISPLAY_QQ)); PerlIO_printf(file, "\n"); } @@ -2124,9 +2139,9 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo keypv = SvPV_const(keysv, len); elt = HeVAL(he); - Perl_dump_indent(aTHX_ level+1, file, "Elt %s ", _pv_display(aTHX_ d, keypv, len, 0, pvlim)); + Perl_dump_indent(aTHX_ level+1, file, "Elt %s ", S_pv_display(aTHX_ d, keypv, len, 0, pvlim)); if (SvUTF8(keysv)) - PerlIO_printf(file, "[UTF8 \"%s\"] ", _sv_uni_display(aTHX_ d, keysv, 6 * SvCUR(keysv), UNI_DISPLAY_QQ)); + PerlIO_printf(file, "[UTF8 \"%s\"] ", S_sv_uni_display(aTHX_ d, keysv, 6 * SvCUR(keysv), UNI_DISPLAY_QQ)); if (HvEITER_get(hv) == he) PerlIO_printf(file, "[CURRENT] "); PerlIO_printf(file, "HASH = 0x%"UVxf"\n", (UV) hash); @@ -2362,7 +2377,7 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo if (r->subbeg) Perl_dump_indent(aTHX_ level, file, " SUBBEG = 0x%"UVxf" %s\n", PTR2UV(r->subbeg), - _pv_display(aTHX_ d, r->subbeg, r->sublen, 50, pvlim)); + S_pv_display(aTHX_ d, r->subbeg, r->sublen, 50, pvlim)); else Perl_dump_indent(aTHX_ level, file, " SUBBEG = 0x0\n"); Perl_dump_indent(aTHX_ level, file, " MOTHER_RE = 0x%"UVxf"\n", -- 2.0.4
Download (untitled) / with headers
text/plain 137b
I have to admit I've made a hell of a mess within this ticket. Should I squash my commits & apply for a new ticket to supersede this one?
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 915b
On Wed Aug 13 01:41:56 2014, creaktive@gmail.com wrote: Show quoted text
> I have to admit I've made a hell of a mess within this ticket. > Should I squash my commits & apply for a new ticket to supersede this one?
Could you supply a new squashed patch for this ticket? Some of the issues I see so far: - many of the changes appear to be pure whitespace changes, which should be a separate patch - your naming patch left _sv_catpv(), which still uses a leading _ and doesn't operate on SVs At a higher level I'm a bit worried that you're going to run out of space displaying a complex SV. From reading Nicholas' note in Porting/todo.pod it reads like he's expecting a more complex buffer implementation, eg: struct bf { char *buf; size_t used; size_t alloced; }; with a small set of functions to allocate and update that - like a mini SV. I'm not sure how well that would integrate into PerlIO though. Tony
Subject: Re: [perl #122405] [PATCH] Started work on "Remove the use of SVs as temporaries in dump.c"
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Date: Wed, 8 Oct 2014 10:37:47 +0200
From: demerphq <demerphq [...] gmail.com>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
Download (untitled) / with headers
text/plain 1.1k
On 8 October 2014 02:40, Tony Cook via RT <perlbug-followup@perl.org> wrote:
Show quoted text
On Wed Aug 13 01:41:56 2014, creaktive@gmail.com wrote:
> I have to admit I've made a hell of a mess within this ticket.
> Should I squash my commits & apply for a new ticket to supersede this one?

Could you supply a new squashed patch for this ticket?

Some of the issues I see so far:

- many of the changes appear to be pure whitespace changes, which should be a separate patch

- your naming patch left _sv_catpv(), which still uses a leading _ and doesn't operate on SVs

At a higher level I'm a bit worried that you're going to run out of space displaying a complex SV.

From reading Nicholas' note in Porting/todo.pod it reads like he's expecting a more complex buffer implementation, eg:

  struct bf {
    char *buf;
    size_t used;
    size_t alloced;
  };

with a small set of functions to allocate and update that - like a mini SV.

I'm not sure how well that would integrate into PerlIO though.


Why are we doing this? I see no point in inventing a "mini-sv" which will essentially duplicate a real SV.

Yves
To: perl5-porters [...] perl.org
From: Father Chrysostomos <sprout [...] cpan.org>
Date: 8 Oct 2014 15:33:00 -0000
Subject: Re: [perl #122405] [PATCH] Started work on "Remove the use of SVs as temporaries in dump.c"
Download (untitled) / with headers
text/plain 223b
Yves Orton asked: Show quoted text
> Why are we doing this? I see no point in inventing a "mini-sv" which will > essentially duplicate a real SV.
I don't remember the details, but I think it is to make debugging SV- handling code easier.
Subject: Re: [perl #122405] [PATCH] Started work on "Remove the use of SVs as temporaries in dump.c"
Date: Wed, 8 Oct 2014 17:50:25 +0200
From: Jarkko Hietaniemi <jhi [...] iki.fi>
To: Perl5 Porters <perl5-porters [...] perl.org>
Download (untitled) / with headers
text/plain 809b
Show quoted text
>> Why are we doing this? I see no point in inventing a "mini-sv" which will >> essentially duplicate a real SV.
> > I don't remember the details, but I think it is to make debugging SV- > handling code easier.
I never even knew the details (so can't have forgotten them), but at a high-level having some sort abstraction somewhere between a PV and and and a SvPV doesn't sound too bad. The suggested tuple of {pointer, allocated, used} is such basic (as in "important", not as in "trivial") building block for ... well, everything. Such basic management doesn't need to duplicate a SV but instead form the ... ta-dah, basis. The usual caveats apply: you think I actually bothered to read the discussion so far? -- There is this special biologist word we use for 'stable'. It is 'dead'. -- Jack Cohen
Subject: Re: [perl #122405] [PATCH] Started work on "Remove the use of SVs as temporaries in dump.c"
From: Tony Cook <tony [...] develop-help.com>
Date: Fri, 10 Oct 2014 10:20:43 +1100
To: demerphq <demerphq [...] gmail.com>
CC: Perl RT Bug Tracker <perlbug-followup [...] perl.org>, Perl5 Porteros <perl5-porters [...] perl.org>
Download (untitled) / with headers
text/plain 1.2k
On Wed, Oct 08, 2014 at 10:37:47AM +0200, demerphq wrote: Show quoted text
> On 8 October 2014 02:40, Tony Cook via RT <perlbug-followup@perl.org> wrote:
> > From reading Nicholas' note in Porting/todo.pod it reads like he's > > expecting a more complex buffer implementation, eg: > > > > struct bf { > > char *buf; > > size_t used; > > size_t alloced; > > }; > > > > with a small set of functions to allocate and update that - like a mini SV. > > > > I'm not sure how well that would integrate into PerlIO though. > >
> > > Why are we doing this? I see no point in inventing a "mini-sv" which will > essentially duplicate a real SV.
Well, my example would be much simpler than an SV - the basics needed to manage a buffer. Allocation, append, destruction, maybe an append formatted to make it more useful for dump.c (through sn?printf(), not Perl_form()/sv_catpvf()). If you haven't, you might want to read "Remove the use of SVs as temporaries in dump.c" in Porting/todo.pod to see where this came from. I don't know that this removal is necessary, but I think that doing is with a fixed size buffer as in the supplied patch isn't correct. I think using a slightly more complex structure matches Nicholas's intent more closely. Tony
CC: Perl RT Bug Tracker <perlbug-followup [...] perl.org>, Perl5 Porteros <perl5-porters [...] perl.org>
Date: Fri, 10 Oct 2014 08:57:36 +0200
From: demerphq <demerphq [...] gmail.com>
To: Tony Cook <tony [...] develop-help.com>
Subject: Re: [perl #122405] [PATCH] Started work on "Remove the use of SVs as temporaries in dump.c"
Download (untitled) / with headers
text/plain 1.6k
On 10 October 2014 01:20, Tony Cook <tony@develop-help.com> wrote:
Show quoted text
On Wed, Oct 08, 2014 at 10:37:47AM +0200, demerphq wrote:
> On 8 October 2014 02:40, Tony Cook via RT <perlbug-followup@perl.org> wrote:
> > From reading Nicholas' note in Porting/todo.pod it reads like he's
> > expecting a more complex buffer implementation, eg:
> >
> >   struct bf {
> >     char *buf;
> >     size_t used;
> >     size_t alloced;
> >   };
> >
> > with a small set of functions to allocate and update that - like a mini SV.
> >
> > I'm not sure how well that would integrate into PerlIO though.
> >
>
>
> Why are we doing this? I see no point in inventing a "mini-sv" which will
> essentially duplicate a real SV.

Well, my example would be much simpler than an SV - the basics needed
to manage a buffer.  Allocation, append, destruction, maybe an append
formatted to make it more useful for dump.c (through sn?printf(), not
Perl_form()/sv_catpvf()).

If you haven't, you might want to read "Remove the use of SVs as
temporaries in dump.c" in Porting/todo.pod to see where this came
from.


Ok thanks that explains everything.
 
Show quoted text
I don't know that this removal is necessary, but I think that doing is
with a fixed size buffer as in the supplied patch isn't correct.  I
think using a slightly more complex structure matches Nicholas's
intent more closely.

Yep. I agree.  FWIW, Sereal uses a similar technique, creating and managing a buffer just like this.

I doubt our code is worth stealing tho, better to roll a new implementation.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 678b
On Tue Oct 07 17:40:08 2014, tonyc wrote: Show quoted text
> At a higher level I'm a bit worried that you're going to run out of > space displaying a complex SV. > > From reading Nicholas' note in Porting/todo.pod it reads like he's > expecting a more complex buffer implementation, eg: > > struct bf { > char *buf; > size_t used; > size_t alloced; > }; > > with a small set of functions to allocate and update that - like a > mini SV. > > I'm not sure how well that would integrate into PerlIO though.
There have been no updates to this ticket since October, and I don't think the patch as-is is suitable for blead. I'll close this ticket in a few days if there's no activity. Tony
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 773b
On Wed Jun 10 22:40:52 2015, tonyc wrote: Show quoted text
> On Tue Oct 07 17:40:08 2014, tonyc wrote:
> > At a higher level I'm a bit worried that you're going to run out of > > space displaying a complex SV. > > > > From reading Nicholas' note in Porting/todo.pod it reads like he's > > expecting a more complex buffer implementation, eg: > > > > struct bf { > > char *buf; > > size_t used; > > size_t alloced; > > }; > > > > with a small set of functions to allocate and update that - like a > > mini SV. > > > > I'm not sure how well that would integrate into PerlIO though.
> > There have been no updates to this ticket since October, and I don't think > the patch as-is is suitable for blead. > > I'll close this ticket in a few days if there's no activity.
Closed. Tony


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

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org