develooper Front page | perl.perl5.porters | Postings from April 2018

Re: [perl #133131] Blead Breaks CPAN: Devel::Cover

Thread Previous | Thread Next
Dave Mitchell
April 21, 2018 10:53
Re: [perl #133131] Blead Breaks CPAN: Devel::Cover
Message ID:
On Fri, Apr 20, 2018 at 09:19:32AM -0700, Aaron Crane wrote:
> 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"

Thread Previous | Thread Next Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at | Group listing | About