Front page | perl.perl5.porters |
Postings from January 2014
Re: [perl #117273] [PATCH] e4f39c0 binary safety when dumping svs and ops
Thread Previous
From:
Brian Fraser
Date:
January 7, 2014 05:38
Subject:
Re: [perl #117273] [PATCH] e4f39c0 binary safety when dumping svs and ops
Message ID:
CA+nL+nYZmA=1zhZxNG0P8vFxMQAyC0OybdbdM0ukx0t8ooLVGg@mail.gmail.com
On Sun, Mar 24, 2013 at 6:51 AM, Brian Fraser <fraserbn@gmail.com> wrote:
> On Sat, Mar 23, 2013 at 8:47 PM, Brian Fraser <fraserbn@gmail.com> wrote:
> > On Sat, Mar 23, 2013 at 4:20 PM, Brian Fraser <fraserbn@gmail.com>
> wrote:
> >> On Sat, Mar 23, 2013 at 11:34 AM, Dave Mitchell <davem@iabyn.com>
> wrote:
> >>> On Thu, Mar 21, 2013 at 04:40:10PM -0700, Reini Urban via RT wrote:
> >>>> print embedded control chars in names in dump.c
> >>> ...
> >>>> The first patch falsely decremented TEMP SVs. Attached is the fixed
> >>>> version, sorry
> >>>
> >>> Hi, thanks for this. This has always annoyed me about dump, and is long
> >>> overdue to be fixed. A couple of things;
> >>>
> >>> You haven't included any tests to check this new feature.
> >>>
> >>> Also it doesn't seem complete, e.g.
> >>>
> >>> $ pd -e'Dump *{"a\001b"}'
> >>> SV = PVGV(0x25fac08) at 0x25c3d10
> >>> REFCNT = 1
> >>> FLAGS = (MULTI)
> >>> NAME = "a b" # a raw \001 here
> >>> ..
> >>>
> >>
> >> This is actually my fault. I never updated dump.c after the
> >> utf8/nul-safe work. I have a half-finished branch somewhere, but
> >> testing dump.c is a pain (see ext/Devel-Peek/t/Peek.t). So put "not
> >> escaping things properly" outside of the scope of this bug report.
> >>
> >> That being said, perhaps this patch would be superfluous if I revived
> >> that branch. I'm not at the right computer at the moment, but I'll
> >> update this ticket with details tonight -- If I recall correctly, my
> >> blocker was testing DumpProg(), since Test::More doesn't have a
> >> fresh_perl() equivalent.
> >>
> >
> > https://github.com/Hugmeir/utf8mess/tree/dump_c_cleanliness
> >
> > WIP and not fully tested, but:
> > $ ./perl -Ilib -MDevel::Peek -e'Dump *{"a\001b"}; Dump *{"a\0b"}; Dump
> > *{"\xdf\1::\x{30cd}\1b"}; Dump *{"\x{30cd}\0::\0asd\xdf"}'
> > SV = PVGV(0x1c43f50) at 0x1be7fc8
> > REFCNT = 1
> > FLAGS = (MULTI)
> > NAME = "a\x01b"
> > NAMELEN = 3
> > GvSTASH = 0x1be7e30 "main"
> > GP = 0x1c22e10
> > SV = 0x0
> > REFCNT = 1
> > IO = 0x0
> > FORM = 0x0
> > AV = 0x0
> > HV = 0x0
> > CV = 0x0
> > CVGEN = 0x0
> > LINE = 1
> > FILE = "-e"
> > FLAGS = 0x2
> > EGV = 0x1be7fc8 "a\x01b"
> > SV = PVGV(0x1c43f80) at 0x1bfc570
> > REFCNT = 1
> > FLAGS = (MULTI)
> > NAME = "a\0b"
> > NAMELEN = 3
> > GvSTASH = 0x1be7e30 "main"
> > GP = 0x1c01390
> > SV = 0x0
> > REFCNT = 1
> > IO = 0x0
> > FORM = 0x0
> > AV = 0x0
> > HV = 0x0
> > CV = 0x0
> > CVGEN = 0x0
> > LINE = 1
> > FILE = "-e"
> > FLAGS = 0x2
> > EGV = 0x1bfc570 "a\0b"
> > SV = PVGV(0x1c43fe0) at 0x1bfc588
> > REFCNT = 1
> > FLAGS = (MULTI)
> > NAME = "\x{30cd}\x{01}b"
> > NAMELEN = 5
> > GvSTASH = 0x1bfc5a0 "\xdf\x01"
> > GP = 0x1bf5690
> > SV = 0x0
> > REFCNT = 1
> > IO = 0x0
> > FORM = 0x0
> > AV = 0x0
> > HV = 0x0
> > CV = 0x0
> > CVGEN = 0x0
> > LINE = 1
> > FILE = "-e"
> > FLAGS = 0x2
> > EGV = 0x1bfc588 "\x{30cd}\x{01}b"
> > SV = PVGV(0x1c494a0) at 0x1c08da8
> > REFCNT = 1
> > FLAGS = ()
> > NAME = "\0asd\xdf"
> > NAMELEN = 5
> > GvSTASH = 0x1be80e8 "\x{30cd}\0"
> > GP = 0x1cbf610
> > SV = 0x0
> > REFCNT = 1
> > IO = 0x0
> > FORM = 0x0
> > AV = 0x0
> > HV = 0x0
> > CV = 0x0
> > CVGEN = 0x0
> > LINE = 1
> > FILE = "-e"
> > FLAGS = 0x0
> > EGV = 0x1c08da8 "\0asd\xdf"
> >
> > The DWIM format came from a discussion that happened last year,
> > although it's probably buggy at the moment.
> >
> >>> (although that's no reason not to apply the current work.)
> >>>
> >>> Finally, gcc is giving this warning:
> >>>
> >>>
> >>> dump.c: In function ‘Perl_do_sv_dump’:
> >>> dump.c:1864:22: warning: null argument where non-null required
> (argument 3) [-Wnonnull]
> >>>
> >>> which corresponds to this line:
> >>>
> >>> pv_display(tmp, HvENAME_get(sv),
> HvENAMELEN_get(sv), 0, pvlim));
> >>>
> >>>
> >>> presumably because HvENAME_get() is capable of returning null.
> >>>
> >>>
> >
> > Heh, the branch has the same thing.
>
> Here's a couple of issues that came up while tweaking with dump.c:
>
> * gv_dump() looks unused by the core & CPAN
> (http://grep.cpan.me/?q=\b%28Perl_%29%3Fgv_dump+-file%3A%22ppport\.h%22<http://grep.cpan.me/?q=%5Cb%28Perl_%29%3Fgv_dump+-file%3A%22ppport%5C.h%22>
> )
> * watch() also looks unused
> * Nothing in the test suite triggers the CvNAMED() codepath.
> * This might be a 5.18 blocker: OUTSIDE hasn't been working since
> somewhere between 5.17.1 and 5.17.2. Broken perls will show 'OUTSIDE =
> 0x0 (null)' for the following: ./perl -Ilib -MDevel::Peek -le 'sub
> doof { my $x; return sub { $x } } Dump(doof())'
> * The prototype for lexical subs loses UTF8ness. Patch for this attached
> below.
> * Unsurprisingly, the MAD code is bitrotten
>
> Besides that, I'm not sure how to test the CvAUTOLOAD case, since that
> requires XS. Ditch Peek.t and go for XS::APITest + sv_dump()?
>
> Meanwhile, I have created a monster: Peek.t now uses both test.pl and
> Test::More. Madness? Maybe, but fresh_perl_like() is awesome.
>
This fix is now on blead, so I'm closing the issue.
Thread Previous