develooper Front page | perl.perl5.porters | Postings from May 2008

Re: [perl #51370] Clearing magic (was: [#perl 51370] length($@)>0 for empty $@ if utf8 is in use)

Thread Previous | Thread Next
From:
Nicholas Clark
Date:
May 4, 2008 10:03
Subject:
Re: [perl #51370] Clearing magic (was: [#perl 51370] length($@)>0 for empty $@ if utf8 is in use)
Message ID:
20080504170302.GG42280@plum.flirble.org
On Sat, May 03, 2008 at 10:05:10PM +0200, Bram wrote:
> Quoting Nicholas Clark <nick@ccl4.org>:
> 
> >
> >On Wed, Apr 30, 2008 at 01:36:40PM +0200, Bram wrote:
> >>>> And: if it has to change everywhere then what would the best approach
> >>>> be?
> >>>> Placing it in a separate function? (If so, in what file, what
> >>>> name, ...)
> >>
> >>My current plan:
> >>
> >>Adding Perl_sv_setpvn_clearmg in sv.c which basically contains:
> >>  sv_setpvn(sv,ptr,len);
> >>  if (SvMAGICAL(sv)) {
> >>    mg_free(sv);
> >>    mg_clear(sv);
> >>  }
> >>  SvPOK_only(sv);
> >
> >In general, are you sure you want that order? Wouldn't it be   
> >conceptually more
> >logical to free up any magic, then do the assignment to the SV?
> 
> I'm not sure about it at all...
> Hence the questions. :)

Well, actually nor am I, so it comes to detective work...

> >Also, your order mg_free() then mg_clear() is backwards, as mg_clear() will
> >have to be a no-op this way round, because mg_free will have just freed all
> >the magic.
> 
> Copy from an earlier:
> 
> if (flags & G_KEEPERR)
>   PL_in_eval |= EVAL_KEEPERR;
> else {
>   sv_setpvn_mg(ERRSV,"",0);
>   if (SvMAGICAL(ERRSV)) {
>     mg_free(ERRSV);
>   }
>   SvPOK_only(ERRSV);
> }
> 
> 
> Output:
> 
> SV = PV(0x814e3c8) at 0x8150518
>   REFCNT = 1
>   FLAGS = (POK,pPOK)
>   PV = 0x815cbe0 ""\0
>   CUR = 0
>   LEN = 240
> SV = PVMG(0x815c8b0) at 0x8150518
>   REFCNT = 1
>   FLAGS = (SMG,POK,pPOK)
>   IV = 0
>   NV = 0
>   PV = 0x815cbe0 ""\0
>   CUR = 0
>   LEN = 240
> 
> 
> With the mg_clear added after the mg_free:
> 
> if (flags & G_KEEPERR)
>   PL_in_eval |= EVAL_KEEPERR;
> else {xs
>   sv_setpvn_mg(ERRSV,"",0);
>   if (SvMAGICAL(ERRSV)) {
>     mg_free(ERRSV);
>     mg_clear(ERRSV);
>   }
>   SvPOK_only(ERRSV);
> }
> 
> 
> SV = PV(0x814e3e8) at 0x8150538
>   REFCNT = 1
>   FLAGS = (POK,pPOK)
>   PV = 0x815cc00 ""\0
>   CUR = 0
>   LEN = 240
> SV = PVMG(0x815c8d0) at 0x8150538
>   REFCNT = 1
>   FLAGS = (POK,pPOK)
>   IV = 0
>   NV = 0
>   PV = 0x815cc00 ""\0
>   CUR = 0
>   LEN = 240
> 
> Whichs shows that it is not a no-op (the SMG flag is gone).
> Or would that be a bug in mg_clear?

Well, I don't know, but I grep'ed the core for all uses of mg_free() and
mg_clear() and a had a think. And I think the bug would be in mg_free(),
which currently reads like this:

int
Perl_mg_free(pTHX_ SV *sv)
{
    MAGIC* mg;
    MAGIC* moremagic;

    PERL_ARGS_ASSERT_MG_FREE;

    for (mg = SvMAGIC(sv); mg; mg = moremagic) {
        const MGVTBL* const vtbl = mg->mg_virtual;
	moremagic = mg->mg_moremagic;
	if (vtbl && vtbl->svt_free)
	    CALL_FPTR(vtbl->svt_free)(aTHX_ sv, mg);
	if (mg->mg_ptr && mg->mg_type != PERL_MAGIC_regex_global) {
	    if (mg->mg_len > 0 || mg->mg_type == PERL_MAGIC_utf8)
		Safefree(mg->mg_ptr);
	    else if (mg->mg_len == HEf_SVKEY)
		SvREFCNT_dec((SV*)mg->mg_ptr);
	}
	if (mg->mg_flags & MGf_REFCOUNTED)
	    SvREFCNT_dec(mg->mg_obj);
	Safefree(mg);
	SvMAGIC_set(sv, moremagic);
    }
    SvMAGIC_set(sv, NULL);
    return 0;
}

and probably should have a line added at the end to reset all the SvFLAGS
relating to magic. (which mg_clear() must do somehow, but after 5 minutes of
trying to trace the code, I can't work out where. Also, I'm now not sure
which Perl code "1" liner you're using to test this)

And then fix up any callers of mg_free() that clear all the SV magic flags
already, now that mg_free() does it for them. (I found at least one plausible
place.)

> >>Defining sv_setpvn_clearmg in embed.h
> >>#define sv_setpvn_clearmg            Perl_sv_setpvn_clearmg
> >>
> >>Changing: sv_setpvn(ERRSV,"",0) into sv_setpvn_clearmg(ERRSV,"",0);
> >
> >How many of these are there to change?
> 
> There are 9.

Ah right. I'm not convinced that it's worth it as a one liner function.
A macro for resetting ERRSV might seem more appropriate.

> Suggestions for the name of this_clearing_thing?

#define clear_errsv() STMT_START { ... do stuff ... } STMT_END

?

> What version/revision is that? (It never happend with me.)

blead, built with -g and therefore -DDEBUGGING and I think by default UTF-8
cache checking.

> But note, this is what would be fixed by clearing the magic...

I'm a bit confused. mg_clear() clearing the magic?
> The problem is that length still uses the old utf8 length and not the new 
> one.
> So either a new panic was added for it or something else is different...

$ ./perl -e '${^UTF8CACHE} = 0; eval { 1 };  eval { die "\x{a10d};"; }; $_ = length $@; eval { 1 }; warn length $@'
0 at -e line 1.
$ ./perl -e '${^UTF8CACHE} = 1; eval { 1 };  eval { die "\x{a10d};"; }; $_ = length $@; eval { 1 }; warn length $@'
17 at -e line 1.
$ ./perl -e '${^UTF8CACHE} = -1; eval { 1 };  eval { die "\x{a10d};"; }; $_ = length $@; eval { 1 }; warn length $@'
panic: sv_len_utf8 cache 17 real 0 for  at -e line 1.


(0 disables the caching, 1 is caching mode (the non -DDEBUGGING default), -1
is caching assertion mode, where the cache store code is enabled, but the
answers cross checked every time.)


Mmm, in the end, reading the documentation and the source, should every
sv_setpvn(ERRSV, ...); become sv_setpvn_mg(ERRSV, ...), and
sv_catsv(ERRSV, ...) become sv_catsv_mg(ERRSV, ...) ?
Will that solve it?

Nicholas Clark

Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About