Skip Menu |
Report information
Id: 133131
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: arc <arc [at] cpan.org>
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: High
Type: core
Perl Version: 5.27.11
Fixed In: (no value)

Attachments
0001-pp_hot.c-conditionally-deoptimise-pp_iter-when-non-s.patch



To: perlbug [...] perl.org
Subject: Blead Breaks CPAN: Devel::Cover
From: Aaron Crane <arc [...] cpan.org>
Date: Fri, 20 Apr 2018 16:51:51 +0100
Download (untitled) / with headers
text/plain 4.9k
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 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. 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. [Please do not change anything below this line] ----------------------------------------------------------------- --- 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

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

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 523b
On Fri, 20 Apr 2018 16:19:31 GMT, arc wrote: Show quoted text
> 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)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 3.2k
On Fri, 20 Apr 2018 16:19:31 GMT, arc wrote: Show quoted text
> 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 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. > > 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)
To: James E Keenan via RT <perlbug-followup [...] perl.org>
From: Aaron Crane <arc [...] cpan.org>
Subject: Re: [perl #133131] Blead Breaks CPAN: Devel::Cover
CC: Perl5 Porters <perl5-porters [...] perl.org>
Date: Sat, 21 Apr 2018 08:57:41 +0100
Download (untitled) / with headers
text/plain 782b
James E Keenan via RT <perlbug-followup@perl.org> wrote: Show quoted text
> 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
Date: Sat, 21 Apr 2018 09:03:43 +0100
To: James E Keenan via RT <perlbug-followup [...] perl.org>
Subject: Re: [perl #133131] Blead Breaks CPAN: Devel::Cover
CC: Perl5 Porters <perl5-porters [...] perl.org>
From: Aaron Crane <arc [...] cpan.org>
Download (untitled) / with headers
text/plain 849b
James E Keenan via RT <perlbug-followup@perl.org> wrote: Show quoted text
> 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. Show quoted text
> 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
Date: Sat, 21 Apr 2018 11:53:21 +0100
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #133131] Blead Breaks CPAN: Devel::Cover
To: perl5-porters [...] perl.org
On Fri, Apr 20, 2018 at 09:19:32AM -0700, Aaron Crane wrote: Show quoted text
> 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. > > 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"
To: Dave Mitchell <davem [...] iabyn.com>
From: Aaron Crane <arc [...] cpan.org>
Subject: Re: [perl #133131] Blead Breaks CPAN: Devel::Cover
CC: Perl5 Porters <perl5-porters [...] perl.org>
Date: Sat, 21 Apr 2018 12:01:48 +0100
Download (untitled) / with headers
text/plain 751b
Dave Mitchell <davem@iabyn.com> wrote: Show quoted text
> 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. Show quoted text
> 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
RT-Send-CC: perl5-porters [...] perl.org
Merged as f75ab299e2c1f32844ff278938ff12a96931649a -- Aaron Crane
To: Aaron Crane <arc [...] cpan.org>
CC: Perl5 Porters <perl5-porters [...] perl.org>
Subject: Re: [perl #133131] Blead Breaks CPAN: Devel::Cover
From: Dave Mitchell <davem [...] iabyn.com>
Date: Sat, 21 Apr 2018 13:16:19 +0100
Download (untitled) / with headers
text/plain 728b
On Sat, Apr 21, 2018 at 12:01:48PM +0100, Aaron Crane wrote: Show quoted text
> > 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


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org