Bug #9961

TracePoint can skip c_return with rb_rescue()

Added by Koichi Sasada 10 months ago. Updated 8 months ago.

[ruby-dev:48299]
Status:Closed
Priority:Normal
Assignee:Koichi Sasada
ruby -v:2.2 Backport:2.0.0: DONE, 2.1: DONE

Description

下記のようなテストに失敗します。

  def test_rb_rescue
    events = []
    curr_thread = Thread.current
    TracePoint.new(:a_call, :a_return){|tp|
      next if curr_thread != Thread.current
      events << [tp.event, tp.method_id]
    }.enable do
      begin
        -Numeric.new
      rescue => e
        # ignore
      end
    end

    assert_equal [
    [:b_call, :test_rb_rescue],
      [:c_call, :new],
        [:c_call, :initialize],
        [:c_return, :initialize],
      [:c_return, :new],
      [:c_call, :-@],
        [:c_call, :coerce],
          [:c_call, :to_s],
          [:c_return, :to_s],
          [:c_call, :new],
            [:c_call, :initialize],
            [:c_return, :initialize],
          [:c_return, :new],
          [:c_call, :exception],
          [:c_return, :exception],
          [:c_call, :backtrace],
          [:c_return, :backtrace],
        [:c_return, :coerce],            # don't miss it!
        [:c_call, :to_s],
        [:c_return, :to_s],
        [:c_call, :to_s],
        [:c_return, :to_s],
        [:c_call, :new],
          [:c_call, :initialize],
          [:c_return, :initialize],
        [:c_return, :new],
        [:c_call, :exception],
        [:c_return, :exception],
        [:c_call, :backtrace],
        [:c_return, :backtrace],
      [:c_return, :-@],
      [:c_call, :===],
      [:c_return, :===],
    [:b_return, :test_rb_rescue]], events
  end

何が起きているかというと、

(1) -Numeric.new で rb_rescue() の後で rb_funcall() した先で例外が起こる
(2) rb_rescue() では、c_return を無視して cfp を切り詰める

ということにより、c_return がスキップしてしまう、という現象となりました。

次のパッチで解決します。

Index: eval.c
===================================================================
--- eval.c  (revision 46464)
+++ eval.c  (working copy)
@@ -803,7 +803,7 @@ rb_rescue2(VALUE (* b_proc) (ANYARGS), V
    }
     }
     else {
-   th->cfp = cfp; /* restore */
+   rb_vm_rewind_cfp(th, cfp);

    if (state == TAG_RAISE) {
        int handle = FALSE;
@@ -862,7 +862,7 @@ rb_protect(VALUE (* proc) (VALUE), VALUE
    SAVE_ROOT_JMPBUF(th, result = (*proc) (data));
     }
     else {
-   th->cfp = cfp;
+   rb_vm_rewind_cfp(th, cfp);
     }
     MEMCPY(&(th)->root_jmpbuf, &org_jmpbuf, rb_jmpbuf_t, 1);
     th->protect_tag = protect_tag.prev;
Index: vm.c
===================================================================
--- vm.c    (revision 46464)
+++ vm.c    (working copy)
@@ -288,6 +288,23 @@ rb_vm_pop_cfunc_frame(void)
     vm_pop_frame(th);
 }

+void
+rb_vm_rewind_cfp(rb_thread_t *th, rb_control_frame_t *cfp)
+{
+    /* check skipped frame */
+    while (th->cfp != cfp) {
+#if VMDEBUG
+   printf("skipped frame: %s\n", vm_frametype_name(th->cfp));
+#endif
+   if (VM_FRAME_TYPE(th->cfp) != VM_FRAME_MAGIC_CFUNC) {
+       vm_pop_frame(th);
+   }
+   else { /* unlikely path */
+       rb_vm_pop_cfunc_frame();
+   }
+    }
+}
+
 /* obsolete */
 void
 rb_frame_pop(void)
Index: vm_core.h
===================================================================
--- vm_core.h   (revision 46464)
+++ vm_core.h   (working copy)
@@ -901,6 +901,7 @@ VALUE rb_name_err_mesg_new(VALUE obj, VA
 void rb_vm_stack_to_heap(rb_thread_t *th);
 void ruby_thread_init_stack(rb_thread_t *th);
 int rb_vm_control_frame_id_and_class(const rb_control_frame_t *cfp, ID *idp, VALUE *klassp);
+void rb_vm_rewind_cfp(rb_thread_t *th, rb_control_frame_t *cfp);

 void rb_gc_mark_machine_stack(rb_thread_t *th);

Index: vm_eval.c
===================================================================
--- vm_eval.c   (revision 46464)
+++ vm_eval.c   (working copy)
@@ -1093,18 +1093,7 @@ rb_iterate(VALUE (* it_proc) (VALUE), VA
        th->errinfo = Qnil;
        retval = GET_THROWOBJ_VAL(err);

-       /* check skipped frame */
-       while (th->cfp != cfp) {
-#if VMDEBUG
-           printf("skipped frame: %s\n", vm_frametype_name(th->cfp));
-#endif
-           if (VM_FRAME_TYPE(th->cfp) != VM_FRAME_MAGIC_CFUNC) {
-           vm_pop_frame(th);
-           }
-           else { /* unlikely path */
-           rb_vm_pop_cfunc_frame();
-           }
-       }
+       rb_vm_rewind_cfp(th, cfp);
        }
        else{
        /* SDR(); printf("%p, %p\n", cdfp, escape_dfp); */

例によって、2.1, 2.0 でもおきます。

(Numeric.new なんてできるとは知らなかった)


Related issues

Related to Ruby trunk - Bug #9962: Numeric.new Open 06/19/2014

Associated revisions

Revision 46465
Added by Koichi Sasada 10 months ago

  • vm.c (rb_vm_rewind_cfp): add new function to rewind specified cfp with invoking RUBY_EVENT_C_RETURN. [Bug #9961]
  • vm_core.h: ditto.
  • eval.c (rb_protect): use it.
  • eval.c (rb_rescue2): ditto.
  • vm_eval.c (rb_iterate): ditto.
  • test/ruby/test_settracefunc.rb: add a test.
  • vm_core.h (rb_name_err_mesg_new):

Revision 46465
Added by Koichi Sasada 10 months ago

  • vm.c (rb_vm_rewind_cfp): add new function to rewind specified cfp with invoking RUBY_EVENT_C_RETURN. [Bug #9961]
  • vm_core.h: ditto.
  • eval.c (rb_protect): use it.
  • eval.c (rb_rescue2): ditto.
  • vm_eval.c (rb_iterate): ditto.
  • test/ruby/test_settracefunc.rb: add a test.
  • vm_core.h (rb_name_err_mesg_new):

Revision 46469
Added by Koichi Sasada 10 months ago

  • vm_eval.c (rb_catch_protect): fix same problem of [Bug #9961].
  • vm_eval.c (rb_iterate): ditto.

Revision 46469
Added by Koichi Sasada 10 months ago

  • vm_eval.c (rb_catch_protect): fix same problem of [Bug #9961].
  • vm_eval.c (rb_iterate): ditto.

Revision 47051
Added by Tomoyuki Chikanaga 9 months ago

merge revision(s) r46465,r46469,r46484: [Backport #9961]

* vm.c (rb_vm_rewind_cfp): add new function to rewind specified cfp
  with invoking RUBY_EVENT_C_RETURN.
  [Bug #9961]

* vm_core.h: ditto.

* eval.c (rb_protect): use it.

* eval.c (rb_rescue2): ditto.

* vm_eval.c (rb_iterate): ditto.

* test/ruby/test_settracefunc.rb: add a test.

* vm_core.h (rb_name_err_mesg_new): 

* vm_eval.c (rb_catch_protect): fix same problem of [Bug #9961].

* vm_eval.c (rb_iterate): ditto.

* vm_core.h (rb_vm_rewind_cfp): add the prototype declaration.

Revision 47342
Added by Usaku NAKAMURA 8 months ago

merge revision(s) 46465,46469,46484: [Backport #9961]

* vm.c (rb_vm_rewind_cfp): add new function to rewind specified cfp
  with invoking RUBY_EVENT_C_RETURN.
  [Bug #9961]

* vm_core.h: ditto.

* eval.c (rb_protect): use it.

* eval.c (rb_rescue2): ditto.

* vm_eval.c (rb_iterate): ditto.

* test/ruby/test_settracefunc.rb: add a test.

* vm_core.h (rb_name_err_mesg_new): 

* vm_eval.c (rb_catch_protect): fix same problem of [Bug #9961].

* vm_eval.c (rb_iterate): ditto.

* vm_core.h (rb_vm_rewind_cfp): add the prototype declaration.

History

#1 Updated by Koichi Sasada 10 months ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

Applied in changeset r46465.


  • vm.c (rb_vm_rewind_cfp): add new function to rewind specified cfp with invoking RUBY_EVENT_C_RETURN. [Bug #9961]
  • vm_core.h: ditto.
  • eval.c (rb_protect): use it.
  • eval.c (rb_rescue2): ditto.
  • vm_eval.c (rb_iterate): ditto.
  • test/ruby/test_settracefunc.rb: add a test.
  • vm_core.h (rb_name_err_mesg_new):

#2 Updated by Usaku NAKAMURA 10 months ago

#3 Updated by Tomoyuki Chikanaga 9 months ago

バックポート時のメモ。テストでチェックしている TracePoint のフック呼び出しに影響していたため r44610, r44617 も r47050 でバックポートしました。

#4 Updated by Tomoyuki Chikanaga 9 months ago

  • Backport changed from 2.0.0: REQUIRED, 2.1: REQUIRED to 2.0.0: REQUIRED, 2.1: DONE

もうひとつ [:c_call, :coerce] の直後の [:c_call :to_s], [:c_return, :to_s] が 2.1 では検出されないのですが、こちらは出所がわからなかったのでテストから削って r47051 でバックポートしました。 [:c_return, :coerce] はあるので本テストの主旨は確認できていると思います。

#5 Updated by Usaku NAKAMURA 8 months ago

  • Backport changed from 2.0.0: REQUIRED, 2.1: DONE to 2.0.0: DONE, 2.1: DONE

backported into ruby_2_0_0 at r47342.

note:
* ruby 2.0.0にはa_call/a_returnがありません。
* ruby 2.0.0だとcoerceの冒頭でto_sが呼ばれるますね。2.1だけ違う?

Also available in: Atom PDF