Skip Menu |

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Sereal-Encoder CPAN distribution.

Report information
The Basics
Id: 93888
Status: open
Priority: 0/
Queue: Sereal-Encoder
People
Owner: Nobody in particular
Requestors: zefram [...] fysh.org
Cc:
AdminCc:
Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)

History Show all quoted text
  Sat Mar 15 11:53:24 2014 zefram [...] fysh.org - Ticket created
Subject: doesn't preserve special SV identity
Date: Sat, 15 Mar 2014 15:53:12 +0000
To: bug-Sereal-Encoder [...] rt.cpan.org
From: Zefram <zefram [...] fysh.org>
There are some special standard SVs that can be readily referenced by Perl code, and so can be readily distinguished from ordinary SVs that have the same value. Sereal doesn't preserve their identities. It arguably should. Demonstration program attached. -zefram

Message body is not shown because sender requested not to inline it.

  Sun Mar 16 06:56:25 2014 demerphq [...] gmail.com - Correspondence added
Subject: Re: [rt.cpan.org #93888] doesn't preserve special SV identity
Date: Sun, 16 Mar 2014 11:56:13 +0100
To: bug-Sereal-Encoder [...] rt.cpan.org
From: demerphq <demerphq [...] gmail.com>
On 15 March 2014 16:53, Zefram via RT <bug-Sereal-Encoder@rt.cpan.org> wrote: Show quoted text
> Sat Mar 15 11:53:24 2014: Request 93888 was acted upon. > Transaction: Ticket created by zefram@fysh.org > Queue: Sereal-Encoder > Subject: doesn't preserve special SV identity > Broken in: (no value) > Severity: (no value) > Owner: Nobody > Requestors: zefram@fysh.org > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=93888 > > > > There are some special standard SVs that can be readily referenced > by Perl code, and so can be readily distinguished from ordinary SVs > that have the same value. Sereal doesn't preserve their identities. > It arguably should. Demonstration program attached.
We have tags for undef/true/false, and code for detecting them, however as you observe it does not work very well. When I did the initial implementation of these I ran into this problem and filed a perlbug: perl #114838 https://rt.perl.org:443/rt3/Ticket/Display.html?id=114838 I will poke into this, but further investigation from you would be helpful. The existing logic for handling this in srl_encoder.c at line 1353: if (src == &PL_sv_yes) { srl_buf_cat_char(enc, SRL_HDR_TRUE); --enc->recursion_depth; return; } else if (src == &PL_sv_no) { srl_buf_cat_char(enc, SRL_HDR_FALSE); --enc->recursion_depth; return; } -- perl -Mre=debug -e "/just|another|perl|hacker/"
  Sun Mar 16 06:56:25 2014 The RT System itself - Status changed from 'new' to 'open'
  Sun Mar 16 07:48:50 2014 demerphq [...] gmail.com - Correspondence added
Subject: Re: [rt.cpan.org #93888] doesn't preserve special SV identity
Date: Sun, 16 Mar 2014 12:48:37 +0100
To: bug-Sereal-Encoder [...] rt.cpan.org
From: demerphq <demerphq [...] gmail.com>
On 16 March 2014 11:56, demerphq <demerphq@gmail.com> wrote: Show quoted text
> On 15 March 2014 16:53, Zefram via RT <bug-Sereal-Encoder@rt.cpan.org> wrote:
>> Sat Mar 15 11:53:24 2014: Request 93888 was acted upon. >> Transaction: Ticket created by zefram@fysh.org >> Queue: Sereal-Encoder >> Subject: doesn't preserve special SV identity >> Broken in: (no value) >> Severity: (no value) >> Owner: Nobody >> Requestors: zefram@fysh.org >> Status: new >> Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=93888 > >> >> >> There are some special standard SVs that can be readily referenced >> by Perl code, and so can be readily distinguished from ordinary SVs >> that have the same value. Sereal doesn't preserve their identities. >> It arguably should. Demonstration program attached.
> > We have tags for undef/true/false, and code for detecting them, > however as you observe it does not work very well. > > When I did the initial implementation of these I ran into this problem > and filed a perlbug: perl #114838 > https://rt.perl.org:443/rt3/Ticket/Display.html?id=114838 > > I will poke into this, but further investigation from you would be helpful. > > The existing logic for handling this in srl_encoder.c at line 1353:
This turns out to be a decoder issue. I can fix the decoder so it passes 5 of your six tests, one however does not pass \do { my $z = undef }, Which fails because perl says "not-special" and Sereal starts saying "undef". I think there is a conceptual problem with undef. We have the tag SRL_HDR_UNDEF which is used to represent any undef var, including true PL_sv_undef. If we want to track PL_sv_undef properly I think we have to introduce a new tag, SRL_HDR_PERL_UNDEF, which would be used when it really is PL_sv_undef. I am not sure that this is worth it. So I am inclined to keep this behavior for TRUE/FALSE, but not keep it for UNDEF. The patch with one failing test is in the custom_op branch as: commit b4ed5b29aba568ec0bde310eed2afcf3e56811a1 Author: Yves Orton <yves.orton@booking.com> Date: Sun Mar 16 12:48:00 2014 +0100 fix [rt.cpan.org #93888] does not preserve special SV identity Includes one failing test for SRL_HDR_UNDEF related to \do{my $x= undef} which suggests we may have to undo the UNDEF part of this patch and live with the fact that we dont round trip \undef properly, or we have to patch the protocol to support it (boo) -- perl -Mre=debug -e "/just|another|perl|hacker/"
  Sun Mar 16 07:54:48 2014 zefram [...] fysh.org - Correspondence added
Subject: Re: [rt.cpan.org #93888] doesn't preserve special SV identity
Date: Sun, 16 Mar 2014 11:54:32 +0000
To: demerphq via RT <bug-Sereal-Encoder [...] rt.cpan.org>
From: Zefram <zefram [...] fysh.org>
demerphq via RT wrote: Show quoted text
>I am not sure that this is worth it. So I am inclined to keep this >behavior for TRUE/FALSE, but not keep it for UNDEF.
If you were generating an ordinary undef when given PL_sv_undef then this would probably be OK. But generating PL_sv_undef when given an ordinary undef is liable to be a problem, because the user may be expecting that part of the data structure to be mutable, which PL_sv_undef isn't. Modifying an undef part of a data structure to some other value is probably much more common than relying on the identity of PL_sv_undef. -zefram
  Sun Mar 16 08:12:52 2014 demerphq [...] gmail.com - Correspondence added
Subject: Re: [rt.cpan.org #93888] doesn't preserve special SV identity
Date: Sun, 16 Mar 2014 13:12:34 +0100
To: bug-Sereal-Encoder [...] rt.cpan.org
From: demerphq <demerphq [...] gmail.com>
On 16 March 2014 12:54, Zefram via RT <bug-Sereal-Encoder@rt.cpan.org> wrote: Show quoted text
> Queue: Sereal-Encoder > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=93888 > > > demerphq via RT wrote:
>>I am not sure that this is worth it. So I am inclined to keep this >>behavior for TRUE/FALSE, but not keep it for UNDEF.
> > If you were generating an ordinary undef when given PL_sv_undef then this > would probably be OK. But generating PL_sv_undef when given an ordinary > undef is liable to be a problem, because the user may be expecting that > part of the data structure to be mutable, which PL_sv_undef isn't. > Modifying an undef part of a data structure to some other value is > probably much more common than relying on the identity of PL_sv_undef.
Ok, I applied the following: commit 7f6797eae3f61aa0b9415128518d9d241dbdd20a Author: Yves Orton <yves.orton@booking.com> Date: Sun Mar 16 13:09:53 2014 +0100 followup for [rt.cpan.org #93888], leave SRL_HDR_UNDEF unchanged SRL_HDR_UNDEF represents any undefined value. It does not represent PL_sv_undef. For that we really need a new tag. Also on the custom op branch. Which BTW, we need to finish by adding comments, and renaming the new subs to something more memorably different. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
  Mon Mar 17 07:19:33 2014 zefram [...] fysh.org - Correspondence added
Subject: Re: [rt.cpan.org #93888] doesn't preserve special SV identity
Date: Mon, 17 Mar 2014 11:19:21 +0000
To: demerphq via RT <bug-Sereal-Encoder [...] rt.cpan.org>
From: Zefram <zefram [...] fysh.org>
demerphq via RT wrote: Show quoted text
>Also on the custom op branch.
Code looks fine. -zefram