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