develooper Front page | perl.perl5.porters | Postings from March 2013

Re: SV_CHECK_THINKFIRST() needs to be rethought and further COW woes....

Thread Previous | Thread Next
From:
Leon Timmermans
Date:
March 25, 2013 20:33
Subject:
Re: SV_CHECK_THINKFIRST() needs to be rethought and further COW woes....
Message ID:
CAHhgV8jyweavE7mRW4d1VMax2HLw6ztymguzydxT18CNiAXKtA@mail.gmail.com
On Mon, Mar 25, 2013 at 10:14 AM, demerphq <demerphq@gmail.com> wrote:
> Following up on recent mails about HTML::Entities, supposedly
> read-only keys in 5.14, SV_CHECK_THINKFIRST() and etc...
>
> I think SV_CHECK_THINKFIRST() is broken for some definition of broken.
>
> First, its name is *terrible*.  It is named after the wrong thing, and
> as such does not from the name suggest that it has side effects, and
> that in fact the side effects are why it is being called.
>
> #define SV_CHECK_THINKFIRST(sv) if (SvTHINKFIRST(sv)) \
>                                     sv_force_normal_flags(sv, 0)
>
> It should be named after "sv_force_normal_flags" and called something
> like SV_FORCE_NORMAL_FLAGS().

Yeah, and the darn thing should actually take those flags.

Also, I suspect user defined callbacks (essentially pre-set magic)
could prove to be rather useful.

> Second, I question the implementation in general.
>
> For instance the original bug in HTML::Entities is that if I feed it a
> key under 5.14 it improperly dies with the message "Can't inline
> decode readonly string".
>
> If I make the following patch:
>
> @@ -616,8 +616,13 @@ decode_entities(...)
>         for (i = 0; i < items; i++) {
>             if (GIMME_V != G_VOID)
>                 ST(i) = sv_2mortal(newSVsv(ST(i)));
> -           else if (SvREADONLY(ST(i)))
> -               croak("Can't inline decode readonly string");
> +           else {
> +#ifdef SV_CHECK_THINKFIRST
> +                SV_CHECK_THINKFIRST(ST(i));
> +#endif
> +                if (SvREADONLY(ST(i)))
> +                   croak("Can't inline decode readonly string in
> decode_entities()");
> +            }
>
> It fixes the problem with keys() as arguments (IOW, READONLY+FAKE).
>
> However, it breaks the old behavior of
>
> decode_entities("thing");
>
> which SHOULD trigger the error, but instead execution never gets
> there, instead it produces:
>
> $ perl -Mblib -MHTML::Entities=decode_entities -e'decode_entities("foo")'
> Modification of a read-only value attempted at -e line 1.
>
> The value according to Devel::Peek is as follows:
>
> SV = PV(0x15b6be8) at 0x15dd9a8
>   REFCNT = 1
>   FLAGS = (PADTMP,POK,READONLY,pPOK)
>   PV = 0x15cf980 "foo"\0
>   CUR = 3
>   LEN = 8
>
> I don't see how this behavior of SV_CHECK_THINKFIRST() can be sound.
> We can't recommend to XS authors that they use it, as it means they
> cant intercept this case and do something sensible with it.
>
> This mechanism seems like it has a rushed, poorly thought through
> aspect to it that is going to cause a lot problems down the road.

Adding a LEAVE_READONLY flag could fix that, right?

> To make things worse btw, IMO we have a real issue with COW, in that
> a) COW strings no longer have the READONLY flag set, code that wants
> to do an inplace modification based on this flag not being set will
> silently start modifying COW strings.

Yeah, that could result in some interesting issues.

> Lastly, I thought COW was disabled, but I see lots of code related to
> it. Devel::Peek still reports keys() as being COW, etc.
>
> I think we have a real mess on our hands here. I feel really pessimistic.

Possibly…

Leon

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