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
Comments
From @arcCreated by @arcCommit 7c11486 introduced an The optimisation takes advantage of the fact that the op_next of an It's possible that some weird optree-munging XS module may break this However, Devel::Cover does change the op_ppaddr of the ops it can see, The attached patch converts the asserts into conditional statements, With this change, tests pass for me on a DEBUGGING Perl (and allow the I believe a fix for this issue should be applied despite the state of Perl Info
|
From @arc0001-pp_hot.c-conditionally-deoptimise-pp_iter-when-non-s.patchFrom 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
|
From @jkeenanOn Fri, 20 Apr 2018 16:19:31 GMT, arc wrote:
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. -- |
The RT System itself - Status changed from 'new' to 'open' |
From @jkeenanOn Fri, 20 Apr 2018 16:19:31 GMT, arc wrote:
I can confirm that Devel::Cover fails many tests when tested on a -DDEBUGGING build of blead. Output like this: ##### 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. -- |
From @arcJames E Keenan via RT <perlbug-followup@perl.org> wrote:
The "mp" stands for "MacPorts", which is a software installation tool -- |
From @arcJames E Keenan via RT <perlbug-followup@perl.org> wrote:
Assertions are normally enabled only in DEBUGGING builds, so
I haven't investigated; it was coincidence that I tripped over this -- |
From @iabynOn Fri, Apr 20, 2018 at 09:19:32AM -0700, Aaron Crane wrote:
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 -- |
From @arcDave Mitchell <davem@iabyn.com> wrote:
Thanks. Sawyer has confirmed in-person that this should go in, so I'll
This morning, at Ilmari's suggestion, I tried running Devel::Cover on -- |
@arc - Status changed from 'open' to 'resolved' |
From @iabynOn Sat, Apr 21, 2018 at 12:01:48PM +0100, Aaron Crane wrote:
Ah, ok. -- |
Migrated from rt.perl.org#133131 (status was 'resolved')
Searchable as RT133131$
The text was updated successfully, but these errors were encountered: