[ruby-core:109822] [Ruby master Bug#18991] False LocalJumpError when branch coverage is enabled

[ruby-core:109822] [Ruby master Bug#18991] False LocalJumpError when branch coverage is enabled

[qnighy (Masaki Hara)]

Issue #18991 has been reported by qnighy (Masaki Hara).

----------------------------------------
Bug #18991: False LocalJumpError when branch coverage is enabled
https://bugs.ruby-lang.org/issues/18991

* Author: qnighy (Masaki Hara)
* Status: Open
* Priority: Normal
* ruby -v: ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21]
* Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN
----------------------------------------
Enabling branch coverage leads to a false LocalJumpError where it should not be raised.

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

load "./test2.rb"
```

```ruby
# 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>'
```



-- 
https://bugs.ruby-lang.org/

[ruby-core:109824] [Ruby master Bug#18991] False LocalJumpError when branch coverage is enabled

[mame (Yusuke Endoh)]

Issue #18991 has been updated by 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_BREAK`exactly 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
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 @ko1

----------------------------------------
Bug #18991: False LocalJumpError when branch coverage is enabled
https://bugs.ruby-lang.org/issues/18991#change-99062

* Author: qnighy (Masaki Hara)
* Status: Open
* Priority: Normal
* ruby -v: ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21]
* Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN
----------------------------------------
Enabling branch coverage leads to a false LocalJumpError where it should not be raised.

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

load "./test2.rb"
```

```ruby
# 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>'
```



-- 
https://bugs.ruby-lang.org/

[ruby-core:110649] [Ruby master Bug#18991] False LocalJumpError when branch coverage is enabled

[mame (Yusuke Endoh)]

Issue #18991 has been updated by mame (Yusuke Endoh).

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

After talking with @nobu and @ko1, we decided to put my patch.

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

----------------------------------------
Bug #18991: False LocalJumpError when branch coverage is enabled
https://bugs.ruby-lang.org/issues/18991#change-99990

* Author: qnighy (Masaki Hara)
* Status: Assigned
* Priority: Normal
* Assignee: mame (Yusuke Endoh)
* ruby -v: ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21]
* Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN
----------------------------------------
Enabling branch coverage leads to a false LocalJumpError where it should not be raised.

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

load "./test2.rb"
```

```ruby
# 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>'
```



-- 
https://bugs.ruby-lang.org/

[ruby-core:115173] [Ruby master Bug#18991] False LocalJumpError when branch coverage is enabled

[kjtsanaktsidis (KJ Tsanaktsidis) via ruby-core]

Issue #18991 has been updated by 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.

----------------------------------------
Bug #18991: False LocalJumpError when branch coverage is enabled
https://bugs.ruby-lang.org/issues/18991#change-105084

* Author: qnighy (Masaki Hara)
* Status: Closed
* Priority: Normal
* Assignee: mame (Yusuke Endoh)
* ruby -v: ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21]
* Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN
----------------------------------------
Enabling branch coverage leads to a false LocalJumpError where it should not be raised.

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

load "./test2.rb"
```

```ruby
# 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>'
```



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:115262] [Ruby master Bug#18991] False LocalJumpError when branch coverage is enabled

[usa (Usaku NAKAMURA) via ruby-core]

Issue #18991 has been updated by 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.

----------------------------------------
Bug #18991: False LocalJumpError when branch coverage is enabled
https://bugs.ruby-lang.org/issues/18991#change-105178

* Author: qnighy (Masaki Hara)
* Status: Closed
* Priority: Normal
* Assignee: mame (Yusuke Endoh)
* ruby -v: ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21]
* Backport: 3.0: UNKNOWN, 3.1: DONE, 3.2: DONTNEED
----------------------------------------
Enabling branch coverage leads to a false LocalJumpError where it should not be raised.

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

load "./test2.rb"
```

```ruby
# 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>'
```



-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/postorius/lists/ruby-core.ml.ruby-lang.org/