Bug #18991: False LocalJumpError when branch coverage is enabled

• Status: Closed
• Assignee: mame (Yusuke Endoh)
• Target version: -
• ruby -v: ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21]
• Backport: 3.0: UNKNOWN, 3.1: DONE, 3.2: DONTNEED

[qnighy (Masaki Hara)]

Enabling branch coverage leads to a false LocalJumpError where it should not be raised.

# test.rb
require "coverage"
Coverage.start(branches: true)
# Coverage.start(lines: true)

load "./test2.rb"

# test2.rb
1&.tap do break end

Output:

$ ruby test.rb
/Users/qnighy/workdir/branch-coverage-bug/test2.rb:1:in `block in <top (required)>': break from proc-closure (LocalJumpError)
	from <internal:kernel>:90:in `tap'
	from /Users/qnighy/workdir/branch-coverage-bug/test2.rb:1:in `<top (required)>'
	from test.rb:5:in `load'
	from test.rb:5:in `<main>'

[mame (Yusuke Endoh)]

Good catch, thank you for your report. I could recreate the issue without coverage.

RubyVM::InstructionSequence.compile_option = false

eval("1&.tap { break }") #=> break from proc-closure (LocalJumpError)

Here is my analysis.
throw TAG_BREAK instruction makes a jump only if the continuation of catch of TAG_BREAKexactly matches the instruction immediately following the "send" instruction that is currently being executed. Otherwise, it seems to determine break from proc-closure.

https://github.com/ruby/ruby/blob/0d2422cf63ff330e372613894995e762d122e6b7/vm_insnhelper.c#L1484

This is very fragile. In the case in question, some instructions for recoding branch coverage were inserted immediately after the "send" instruction, which made the condition above unmatch. Also, when the compiler optimization is disabled, a "jump" instruction seems to be inserted after "send" for some reason, resulting in the same error.

Here is a proof-of-concept, an extremely dirty patch to force to move the continuation of catch of TAG_BREAK immediately after "send" (or "invokesuper"):

diff --git a/compile.c b/compile.c
index e906bd1e10..6497a8d64e 100644
--- a/compile.c
+++ b/compile.c
@@ -7353,7 +7353,30 @@ compile_iter(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node, in
                            ISEQ_TYPE_BLOCK, line);
         CHECK(COMPILE(ret, "iter caller", node->nd_iter));
     }
-    ADD_LABEL(ret, retry_end_l);
+
+    {
+        // We need to put the label "retry_end_l" immediately after the last "send" instruction.
+        // This because vm_throw checks if the break cont is equal to the index of next insn of the "send".
+        // (Otherwise, it is considered "break from proc-closure". See "TAG_BREAK" handling in "vm_throw_start".)
+        //
+        // Normally, "send" instruction is at the last.
+        // However, qcall under branch coverage measurement adds some instructions after the "send".
+        //
+        // Note that "invokesuper" appears instead of "send".
+        INSN *iobj;
+        LINK_ELEMENT *last_elem = LAST_ELEMENT(ret);
+        iobj = IS_INSN(last_elem) ? (INSN*) last_elem : (INSN*) get_prev_insn((INSN*) last_elem);
+        while (INSN_OF(iobj) != BIN(send) && INSN_OF(iobj) != BIN(invokesuper)) {
+            iobj = (INSN*) get_prev_insn(iobj);
+        }
+        ELEM_INSERT_NEXT(&iobj->link, (LINK_ELEMENT*) retry_end_l);
+
+        // LINK_ANCHOR has a pointer to the last element, but ELEM_INSERT_NEXT does not update it
+        // even if we add an insn to the last of LINK_ANCHOR. So this updates it manually.
+        if (&iobj->link == LAST_ELEMENT(ret)) {
+            ret->last = (LINK_ELEMENT*) retry_end_l;
+        }
+    }

     if (popped) {
         ADD_INSN(ret, line_node, pop);

I want to change the break handling itself, but I haven't come up with a good fix.

What do you think? @nobu (Nobuyoshi Nakada) @ko1 (Koichi Sasada)

[mame (Yusuke Endoh)]

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

After talking with @nobu (Nobuyoshi Nakada) and @ko1 (Koichi Sasada), we decided to put my patch.

https://github.com/ruby/ruby/pull/6688

[mame (Yusuke Endoh)]

• Status changed from Assigned to Closed

Applied in changeset git|4a7d6c2852aa734506be83c932168e8f974687b5.

---

Fix false LocalJumpError when branch coverage is enabled

throw TAG_BREAK instruction makes a jump only if the continuation of
catch of TAG_BREAK exactly matches the instruction immediately following
the "send" instruction that is currently being executed. Otherwise, it
seems to determine break from proc-closure.

Branch coverage may insert some recording instructions after "send"
instruction, which broke the conditions for TAG_BREAK to work properly.

This change forces to set the continuation of catch of TAG_BREAK
immediately after "send" (or "invokesuper") instruction.

[Bug #18991]

[kjtsanaktsidis (KJ Tsanaktsidis)]

Thank you for the fix mame! Would it be possible for this to be backported to the 3.1 branch? The fix seems to apply cleanly and resolves our issue.

I opened a backport PR here: https://github.com/ruby/ruby/pull/8768.

[hsbt (Hiroshi SHIBATA)]

• Backport changed from 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN to 3.0: UNKNOWN, 3.1: REQUIRED, 3.2: DONTNEED

[usa (Usaku NAKAMURA)]

• Backport changed from 3.0: UNKNOWN, 3.1: REQUIRED, 3.2: DONTNEED to 3.0: UNKNOWN, 3.1: DONE, 3.2: DONTNEED

ruby_3_1 d494cf4ddababb80660381e963f910ccacc3f7bc merged revision(s) 4a7d6c2852aa734506be83c932168e8f974687b5.