Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blead Breaks CPAN: Devel::Cover #16516

Closed
p5pRT opened this issue Apr 20, 2018 · 12 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Apr 20, 2018

Migrated from rt.perl.org#133131 (status was 'resolved')

Searchable as RT133131$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 20, 2018

From @arc

Created by @arc

Commit 7c11486 introduced an
optimisation in pp_iter(). Before the optimisation, pp_iter() pushed
either &PL_SV_yes or &PL_sv_no to the stack, and returned the op_next
in the obvious way.

The optimisation takes advantage of the fact that the op_next of an
OP_ITER always points to an OP_AND node, so pp_iter() now directly
jumps to either the op_next or the op_other of the OP_AND as
appropriate. The commit message also says this​:

  It's possible that some weird optree-munging XS module may break this
  assumption. For now I've just added asserts that the next op is OP_AND
  with an op_ppaddr of Perl_pp_and; if that assertion fails, it may be
  necessary to convert pp_iter()s' asserts into conditional statements.

However, Devel​::Cover does change the op_ppaddr of the ops it can see,
so the assertions on op_ppaddr were being tripped when Devel​::Cover
was run under a -DDEBUGGING Perl. But even if the asserts didn't trip,
skipping the OP_AND nodes would prevent Devel​::Cover from determining
branch coverage in the way that it wants.

The attached patch converts the asserts into conditional statements,
as outlined in the commit message above, and undoes the optimisation
when the op_ppaddr doesn't match.

With this change, tests pass for me on a DEBUGGING Perl (and allow the
Devel​::Cover tests to pass also). Tests also pass when the
optimisation is fully disabled (so I have some confidence that my
deoptimisation is working correctly). However, I'd be grateful for
further review of the change — Dave, I'm hoping you're well placed to
do that.

I believe a fix for this issue should be applied despite the state of
the freeze, so I will add this ticket to the 5.28 blockers.

Perl Info

Flags:
    category=core
    severity=high

Site configuration information for perl 5.27.11:

Configured by arc at Fri Apr 20 15:05:50 CEST 2018.

Summary of my perl5 (revision 5 version 27 subversion 11) configuration:
  Derived from: affe54fad590821990edb8bd8db47d6e898c9114
  Platform:
    osname=darwin
    osvers=13.4.0
    archname=darwin-2level
    uname='darwin daybreak 13.4.0 darwin kernel version 13.4.0: mon
jan 11 18:17:34 pst 2016; root:xnu-2422.115.15~1release_x86_64 x86_64
i386 macbookpro11,3 darwin '
    config_args='-des -Dusedevel -Dprefix=/tmp/blead -DEBUGGING -Dcc=gcc-mp-4.8'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
    bincompat5005=undef
  Compiler:
    cc='gcc-mp-4.8'
    ccflags ='-fno-common -DPERL_DARWIN -mmacosx-version-min=10.9
-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include -I/opt/local/include -DPERL_USE_SAFE_PUTENV'
    optimize='-O3 -g'
    cppflags='-fno-common -DPERL_DARWIN -mmacosx-version-min=10.9
-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include -I/opt/local/include'
    ccversion=''
    gccversion='4.8.5'
    gccosandvers=''
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='off_t'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='gcc-mp-4.8'
    ldflags =' -mmacosx-version-min=10.9 -fstack-protector
-L/opt/local/lib -L/usr/local/lib -L/opt/local/lib/libgcc'
    libpth=/opt/local/lib
/opt/local/lib/gcc48/gcc/x86_64-apple-darwin13/4.8.5/include-fixed
/usr/lib /usr/local/lib /opt/local/lib/libgcc
    libs=-lpthread -lgdbm -ldbm -ldl -lm -lutil -lc
    perllibs=-lpthread -ldl -lm -lutil -lc
    libc=
    so=dylib
    useshrplib=false
    libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=bundle
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags=' '
    lddlflags=' -mmacosx-version-min=10.9 -bundle -undefined
dynamic_lookup -L/opt/local/lib -L/usr/local/lib
-L/opt/local/lib/libgcc -fstack-protector'

Locally applied patches:
    uncommitted-changes


@INC for perl 5.27.11:
    /tmp/blead/lib/perl5/site_perl/5.27.11/darwin-2level
    /tmp/blead/lib/perl5/site_perl/5.27.11
    /tmp/blead/lib/perl5/5.27.11/darwin-2level
    /tmp/blead/lib/perl5/5.27.11


Environment for perl 5.27.11:
    DYLD_LIBRARY_PATH (unset)
    HOME=/Users/arc
    LANG=en_GB.UTF-8
    LANGUAGE (unset)
    LC_COLLATE=C
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/X11R6/bin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 20, 2018

From @arc

0001-pp_hot.c-conditionally-deoptimise-pp_iter-when-non-s.patch
From 074f775584feb9a2c905c7d80be2aaca98ce0ee3 Mon Sep 17 00:00:00 2001
From: Aaron Crane <arc@cpan.org>
Date: Fri, 20 Apr 2018 17:45:04 +0200
Subject: [PATCH] pp_hot.c: conditionally deoptimise pp_iter() when
 non-standard OP_AND op_ppaddr

Commit 7c114860c0fa8ade5e00a4b609d2fbd11d5a494c introduced an optimisation
in pp_iter(). Before the optimisation, pp_iter() pushed either &PL_SV_yes or
&PL_sv_no to the stack, and returned the op_next in the obvious way.

The optimisation takes advantage of the fact that the op_next of an OP_ITER
always points to an OP_AND node, so pp_iter() now directly jumps to either
the op_next or the op_other of the OP_AND as appropriate.

The commit message also says this:

    It's possible that some weird optree-munging XS module may break this
    assumption. For now I've just added asserts that the next op is OP_AND
    with an op_ppaddr of Perl_pp_and; if that assertion fails, it may be
    necessary to convert pp_iter()s' asserts into conditional statements.

However, Devel::Cover does change the op_ppaddr of the ops it can see, so
the assertions on op_ppaddr were being tripped when Devel::Cover was run
under a -DDEBUGGING Perl. But even if the asserts didn't trip, skipping the
OP_AND nodes would prevent Devel::Cover from determining branch coverage in
the way that it wants.

This commit converts the asserts into conditional statements, as outlined in
the commit message above, and undoes the optimisation when the op_ppaddr
doesn't match.
---
 pp_hot.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/pp_hot.c b/pp_hot.c
index ae81e940df..56e3cbe6e1 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -3998,25 +3998,41 @@ PP(pp_iter)
 	DIE(aTHX_ "panic: pp_iter, type=%u", CxTYPE(cx));
     }
 
-    /* Bypass pushing &PL_sv_yes and calling pp_and(); instead
+    /* Try to bypass pushing &PL_sv_yes and calling pp_and(); instead
      * jump straight to the AND op's op_other */
     assert(PL_op->op_next->op_type == OP_AND);
-    assert(PL_op->op_next->op_ppaddr == Perl_pp_and);
-    return cLOGOPx(PL_op->op_next)->op_other;
+    if (PL_op->op_next->op_ppaddr == Perl_pp_and) {
+        return cLOGOPx(PL_op->op_next)->op_other;
+    }
+    else {
+        /* An XS module has replaced the op_ppaddr, so fall back to the slow,
+         * obvious way. */
+        /* pp_enteriter should have pre-extended the stack */
+        EXTEND_SKIP(PL_stack_sp, 1);
+        *++PL_stack_sp = &PL_sv_yes;
+        return PL_op->op_next;
+    }
 
   retno:
-    /* Bypass pushing &PL_sv_no and calling pp_and(); instead
+    /* Try to bypass pushing &PL_sv_no and calling pp_and(); instead
      * jump straight to the AND op's op_next */
     assert(PL_op->op_next->op_type == OP_AND);
-    assert(PL_op->op_next->op_ppaddr == Perl_pp_and);
     /* pp_enteriter should have pre-extended the stack */
     EXTEND_SKIP(PL_stack_sp, 1);
     /* we only need this for the rare case where the OP_AND isn't
      * in void context, e.g. $x = do { for (..) {...} };
-     * but its cheaper to just push it rather than testing first
+     * (or for when an XS module has replaced the op_ppaddr)
+     * but it's cheaper to just push it rather than testing first
      */
     *++PL_stack_sp = &PL_sv_no;
-    return PL_op->op_next->op_next;
+    if (PL_op->op_next->op_ppaddr == Perl_pp_and) {
+        return PL_op->op_next->op_next;
+    }
+    else {
+        /* An XS module has replaced the op_ppaddr, so fall back to the slow,
+         * obvious way. */
+        return PL_op->op_next;
+    }
 }
 
 
-- 
2.14.1

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2018

From @jkeenan

On Fri, 20 Apr 2018 16​:19​:31 GMT, arc wrote​:

This is a bug report for perl from arc@​cpan.org,
generated with the help of perlbug 1.41 running under perl 5.27.11.

config\_args='\-des \-Dusedevel \-Dprefix=/tmp/blead \-DEBUGGING

-Dcc=gcc-mp-4.8'

Can you explain what 'gcc-mp' is? Internet searching hasn't been all that helpful, and I can't recall seeing it mentioned in this list previously. Is it something we should be regularly smoke-testing with?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2018

The RT System itself - Status changed from 'new' to 'open'

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2018

From @jkeenan

On Fri, 20 Apr 2018 16​:19​:31 GMT, arc wrote​:

This is a bug report for perl from arc@​cpan.org,
generated with the help of perlbug 1.41 running under perl 5.27.11.

-----------------------------------------------------------------
[Please describe your issue here]

Commit 7c11486 introduced an
optimisation in pp_iter(). Before the optimisation, pp_iter() pushed
either &PL_SV_yes or &PL_sv_no to the stack, and returned the op_next
in the obvious way.

The optimisation takes advantage of the fact that the op_next of an
OP_ITER always points to an OP_AND node, so pp_iter() now directly
jumps to either the op_next or the op_other of the OP_AND as
appropriate. The commit message also says this​:

It's possible that some weird optree-munging XS module may break this
assumption. For now I've just added asserts that the next op is OP_AND
with an op_ppaddr of Perl_pp_and; if that assertion fails, it may be
necessary to convert pp_iter()s' asserts into conditional statements.

However, Devel​::Cover does change the op_ppaddr of the ops it can see,
so the assertions on op_ppaddr were being tripped when Devel​::Cover
was run under a -DDEBUGGING Perl. But even if the asserts didn't trip,
skipping the OP_AND nodes would prevent Devel​::Cover from determining
branch coverage in the way that it wants.

The attached patch converts the asserts into conditional statements,
as outlined in the commit message above, and undoes the optimisation
when the op_ppaddr doesn't match.

With this change, tests pass for me on a DEBUGGING Perl (and allow the
Devel​::Cover tests to pass also). Tests also pass when the
optimisation is fully disabled (so I have some confidence that my
deoptimisation is working correctly). However, I'd be grateful for
further review of the change — Dave, I'm hoping you're well placed to
do that.

I believe a fix for this issue should be applied despite the state of
the freeze, so I will add this ticket to the 5.28 blockers.

I can confirm that Devel​::Cover fails many tests when tested on a -DDEBUGGING build of blead. Output like this​:

#####
t/e2e/aeval_merge_0.t .......
Dubious, test returned 134 (wstat 34304, 0x8600)
Failed 110/110 subtests
Cannot close ... blead/bin/perl -I./ -I./blib/lib -I./blib/arch -MDevel​::Cover=-db,./t/e2e/cover_db_eval_merge_1/,-select,/tests/eval_merge_1\b,-ignore,blib,Devel/Cover,-merge,0,-coverage,statement,branch,condition,subroutine ./tests/eval_merge_1 : at blib/lib/Devel/Cover/Test.pm line 185.
# Looks like your test exited with 134 before it could output anything.
#####

A couple of weeks back I gave a talk at ny.pm. In the discussion period, I remarked that, because Devel​::Cover (along with Devel​::NYTProf) digs deep into the Perl guts, we expect it to break at least once during each dev cycle.

Surprisingly, at least when built on non-DEBUGGING builds on Linux, Devel​::Cover was graded PASS at each monthly dev release up thru 5.27.10. (I hope to have 5.27.11 results within 2 days.) But of course I don't do that testing on DEBUGGING builds.

Do you know whether any other major CPAN libraries break on DEBUGGING builds?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2018

From @arc

James E Keenan via RT <perlbug-followup@​perl.org> wrote​:

On Fri, 20 Apr 2018 16​:19​:31 GMT, arc wrote​:

This is a bug report for perl from arc@​cpan.org,
generated with the help of perlbug 1.41 running under perl 5.27.11.

config\_args='\-des \-Dusedevel \-Dprefix=/tmp/blead \-DEBUGGING

-Dcc=gcc-mp-4.8'

Can you explain what 'gcc-mp' is? Internet searching hasn't been all that helpful, and I can't recall seeing it mentioned in this list previously. Is it something we should be regularly smoke-testing with?

The "mp" stands for "MacPorts", which is a software installation tool
for Mac OS. This in particular just a very ordinary GCC 4.8 build that
I've been using out of habit; there's no need to consider it a
separate smoke-testing target.

--
Aaron Crane

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2018

From @arc

James E Keenan via RT <perlbug-followup@​perl.org> wrote​:

A couple of weeks back I gave a talk at ny.pm. In the discussion period, I remarked that, because Devel​::Cover (along with Devel​::NYTProf) digs deep into the Perl guts, we expect it to break at least once during each dev cycle.

Surprisingly, at least when built on non-DEBUGGING builds on Linux, Devel​::Cover was graded PASS at each monthly dev release up thru 5.27.10. (I hope to have 5.27.11 results within 2 days.) But of course I don't do that testing on DEBUGGING builds.

Assertions are normally enabled only in DEBUGGING builds, so
non-DEBUGGING builds won't display problems of this sort.

Do you know whether any other major CPAN libraries break on DEBUGGING builds?

I haven't investigated; it was coincidence that I tripped over this
assertion failure.

--
Aaron Crane

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2018

From @iabyn

On Fri, Apr 20, 2018 at 09​:19​:32AM -0700, Aaron Crane wrote​:

Commit 7c11486 introduced an
optimisation in pp_iter(). Before the optimisation, pp_iter() pushed
either &PL_SV_yes or &PL_sv_no to the stack, and returned the op_next
in the obvious way.

The optimisation takes advantage of the fact that the op_next of an
OP_ITER always points to an OP_AND node, so pp_iter() now directly
jumps to either the op_next or the op_other of the OP_AND as
appropriate. The commit message also says this​:

It's possible that some weird optree-munging XS module may break this
assumption. For now I've just added asserts that the next op is OP_AND
with an op_ppaddr of Perl_pp_and; if that assertion fails, it may be
necessary to convert pp_iter()s' asserts into conditional statements.

However, Devel​::Cover does change the op_ppaddr of the ops it can see,
so the assertions on op_ppaddr were being tripped when Devel​::Cover
was run under a -DDEBUGGING Perl. But even if the asserts didn't trip,
skipping the OP_AND nodes would prevent Devel​::Cover from determining
branch coverage in the way that it wants.

The attached patch converts the asserts into conditional statements,
as outlined in the commit message above, and undoes the optimisation
when the op_ppaddr doesn't match.

With this change, tests pass for me on a DEBUGGING Perl (and allow the
Devel​::Cover tests to pass also). Tests also pass when the
optimisation is fully disabled (so I have some confidence that my
deoptimisation is working correctly). However, I'd be grateful for
further review of the change — Dave, I'm hoping you're well placed to
do that.

I believe a fix for this issue should be applied despite the state of
the freeze, so I will add this ticket to the 5.28 blockers.

Looks good to me. I agree it should go in 5.28.

Also, is it possible for you supply a test patch for Devel​::Cover
so that such breakage gets spotted in future?

--
But Pity stayed his hand. "It's a pity I've run out of bullets",
he thought. -- "Bored of the Rings"

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2018

From @arc

Dave Mitchell <davem@​iabyn.com> wrote​:

Looks good to me. I agree it should go in 5.28.

Thanks. Sawyer has confirmed in-person that this should go in, so I'll
merge it today.

Also, is it possible for you supply a test patch for Devel​::Cover
so that such breakage gets spotted in future?

This morning, at Ilmari's suggestion, I tried running Devel​::Cover on
a trivial loop using non-DEBUGGING builds of blead releases either
side of the commit that introduced the optimisation, and the coverage
data was identical (except for timings). As far as I can tell,
Devel​::Cover only measures coverage of the loop condition and body,
not the OP_AND that does the conditional branching, so this specific
difference isn't visible to it.

--
Aaron Crane

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2018

From @arc

Merged as f75ab29

--
Aaron Crane

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2018

@arc - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this Apr 21, 2018
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 21, 2018

From @iabyn

On Sat, Apr 21, 2018 at 12​:01​:48PM +0100, Aaron Crane wrote​:

Also, is it possible for you supply a test patch for Devel​::Cover
so that such breakage gets spotted in future?

This morning, at Ilmari's suggestion, I tried running Devel​::Cover on
a trivial loop using non-DEBUGGING builds of blead releases either
side of the commit that introduced the optimisation, and the coverage
data was identical (except for timings). As far as I can tell,
Devel​::Cover only measures coverage of the loop condition and body,
not the OP_AND that does the conditional branching, so this specific
difference isn't visible to it.

Ah, ok.

--
Overhead, without any fuss, the stars were going out.
  -- Arthur C Clarke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.