Bug #17868: Strange result of Coverage for while-in-while

• Status: Closed
• Assignee: mame (Yusuke Endoh)
• Target version: -
• Backport: 2.6: REQUIRED, 2.7: REQUIRED, 3.0: DONE

[universato (Yoshimine Sato)]

n = 3
while n > 0
  n -= 1 while n > 0
end

This while_in_while.rb is the file to be measured.

require "coverage"
Coverage.start
load "while_in_while.rb"
pp  Coverage.result
# {"while_in_while.rb"=>[1, 1, 0, nil]}

The value of this lines coverage result should be [1, 1, 1, nil].

require "coverage"
Coverage.start(oneshot_lines: true)
load "while_in_while.rb"
pp  Coverage.result
# {"while_in_while.rb"=>{:oneshot_lines=>[1, 2]}}

The value of this oneshot lines coverage result should be [1, 2, 3].

require "coverage"
Coverage.start(lines: true)
load "while_in_while.rb"
pp  Coverage.result
# {"while_in_while.rb"=>{:lines=>[1, 1, 0, nil]}}

The value of this lines coverage result should be [1, 1, 1, nil].

require "coverage"
Coverage.start(:all)
load "while_in_while.rb"
pp  Coverage.result
# {"while_in_while.rb"=>
#   {:lines=>[1, 1, 1, nil],
#    :branches=>
#     {[:while, 0, 2, 0, 4, 3]=>{[:body, 1, 3, 2, 3, 20]=>1},
#      [:while, 2, 3, 2, 3, 20]=>{[:body, 3, 3, 2, 3, 8]=>3}},
#    :methods=>{}}}

I think the value of this lines coverage result is right.

[mame (Yusuke Endoh)]

Thank you for your report. This is caused by the peephole optimization.

% ruby --dump=i while_in_while.rb
...
0010 jump                                   23                        (   3)[Li]
...
0033 getlocal_WC_0                          n@0                       (   2)
0035 putobject_INT2FIX_0_
0036 opt_gt                                 <calldata!mid:>, argc:1, ARGS_SIMPLE>
0038 branchif                               23
...

The original branchif target is 10. Because the instruction at 0010 is jump, the peephole optimization replaces the branchif target with 23. However, the skipped jump instruction has a line event, so it is ignored.

This is a naive patch to disable the peephole optimization when the skipped instruction has any event. But I'm unsure how much the performance degradation is. @ko1 (Koichi Sasada) what do you think?

diff --git a/compile.c b/compile.c
index a9c2f96781..e8bb023b78 100644
--- a/compile.c
+++ b/compile.c
@@ -2926,7 +2926,8 @@ iseq_peephole_optimize(rb_iseq_t *iseq, LINK_ELEMENT *list, const int do_tailcal
        }
         else if (iobj != diobj && IS_INSN(&diobj->link) &&
                  IS_INSN_ID(diobj, jump) &&
-                OPERAND_AT(iobj, 0) != OPERAND_AT(diobj, 0)) {
+                OPERAND_AT(iobj, 0) != OPERAND_AT(diobj, 0) &&
+                 diobj->insn_info.events == 0) {
            /*
             *  useless jump elimination:
             *     jump LABEL1

[mame (Yusuke Endoh)]

• Status changed from Open to Assigned
• Assignee set to mame (Yusuke Endoh)

@ko1 (Koichi Sasada) agreed with my patch. I'll commit it soon.

[universato (Yoshimine Sato)]

Thank you for the quick response!
I have confirmed that the original code can be measured with this patch.

[mame (Yusuke Endoh)]

• Status changed from Assigned to Closed

Applied in changeset git|5026f9a5d5012248729a0052cd6cec811748291b.

---

compile.c: stop the jump-jump optimization if the second has any event

Fixes [Bug #17868]

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN to 2.6: REQUIRED, 2.7: REQUIRED, 3.0: REQUIRED

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.6: REQUIRED, 2.7: REQUIRED, 3.0: REQUIRED to 2.6: REQUIRED, 2.7: REQUIRED, 3.0: DONE

ruby_3_0 ddd720f8dcd98de6a250a152fa40c3c044d62383 merged revision(s) 5026f9a5d5012248729a0052cd6cec811748291b.