ruby-changes:26473
From: ko1 <ko1@a...>
Date: Fri, 21 Dec 2012 18:34:01 +0900 (JST)
Subject: [ruby-changes:26473] ko1:r38524 (trunk): * vm_core.h, vm_trace.c: fix multi-threading bug for tracing.
ko1 2012-12-21 18:33:44 +0900 (Fri, 21 Dec 2012) New Revision: 38524 http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=rev&revision=38524 Log: * vm_core.h, vm_trace.c: fix multi-threading bug for tracing. Move `trace_arg' from rb_tp_t::trace_arg to rb_thread_t::trace_arg. `trace_arg' may changed by multiple threads. rb_thread_t::trace_arg can represent rb_thread_t::trace_running (null or non-null) and rb_thread_t::trace_running is removed. After that, `rb_tp_t' is not needed to check tracing or not (A running thread knows tracing or not). This is why I remove tp_attr_check_active() and make new function get_trace_arg(). And this modification disable to work the following code: TracePoint.trace{|tp| Thread.new{p tp.event} # access `tp' from other threads. } I believe nobody mix threads at trace procedure. This is current limitation. * cont.c (fiber_switch, rb_cont_call): use rb_thread_t::trace_arg instead of rb_thread_t::trace_running. * test/ruby/test_settracefunc.rb: add a multi-threading test. Modified files: trunk/ChangeLog trunk/cont.c trunk/test/ruby/test_settracefunc.rb trunk/vm_core.h trunk/vm_trace.c Index: ChangeLog =================================================================== --- ChangeLog (revision 38523) +++ ChangeLog (revision 38524) @@ -1,3 +1,26 @@ https://github.com/ruby/ruby/blob/trunk/ChangeLog#L1 +Fri Dec 21 17:48:15 2012 Koichi Sasada <ko1@a...> + + * vm_core.h, vm_trace.c: fix multi-threading bug for tracing. + Move `trace_arg' from rb_tp_t::trace_arg to rb_thread_t::trace_arg. + `trace_arg' may changed by multiple threads. + rb_thread_t::trace_arg can represent rb_thread_t::trace_running + (null or non-null) and rb_thread_t::trace_running is removed. + After that, `rb_tp_t' is not needed to check tracing or not + (A running thread knows tracing or not). This is why I remove + tp_attr_check_active() and make new function get_trace_arg(). + + And this modification disable to work the following code: + TracePoint.trace{|tp| + Thread.new{p tp.event} # access `tp' from other threads. + } + I believe nobody mix threads at trace procedure. + This is current limitation. + + * cont.c (fiber_switch, rb_cont_call): use rb_thread_t::trace_arg + instead of rb_thread_t::trace_running. + + * test/ruby/test_settracefunc.rb: add a multi-threading test. + Fri Dec 21 16:38:08 2012 Nobuyoshi Nakada <nobu@r...> * template/id.h.tmpl (ID2ATTRSET): compile time constant macro for Index: vm_core.h =================================================================== --- vm_core.h (revision 38523) +++ vm_core.h (revision 38524) @@ -596,7 +596,7 @@ typedef struct rb_thread_struct { https://github.com/ruby/ruby/blob/trunk/vm_core.h#L596 /* tracer */ rb_hook_list_t event_hooks; - int trace_running; + struct rb_trace_arg_struct *trace_arg; /* trace information */ /* fiber */ VALUE fiber; Index: vm_trace.c =================================================================== --- vm_trace.c (revision 38523) +++ vm_trace.c (revision 38524) @@ -280,11 +280,11 @@ exec_hooks(rb_thread_t *th, rb_hook_list https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L280 } void -rb_threadptr_exec_event_hooks(rb_trace_arg_t *targ) +rb_threadptr_exec_event_hooks(rb_trace_arg_t *trace_arg) { - rb_thread_t *th = targ->th; - if (th->trace_running == 0 && - targ->self != rb_mRubyVMFrozenCore /* skip special methods. TODO: remove it. */) { + rb_thread_t *th = trace_arg->th; + if (th->trace_arg == 0 && + trace_arg->self != rb_mRubyVMFrozenCore /* skip special methods. TODO: remove it. */) { const int vm_tracing = th->vm->trace_running; const VALUE errinfo = th->errinfo; const int outer_state = th->state; @@ -292,27 +292,27 @@ rb_threadptr_exec_event_hooks(rb_trace_a https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L292 th->state = 0; th->vm->trace_running++; - th->trace_running = 1; + th->trace_arg = trace_arg; { rb_hook_list_t *list; /* thread local traces */ list = &th->event_hooks; - if (list->events & targ->event) { - state = exec_hooks(th, list, targ, TRUE); + if (list->events & trace_arg->event) { + state = exec_hooks(th, list, trace_arg, TRUE); if (state) goto terminate; } /* vm global traces */ list = &th->vm->event_hooks; - if (list->events & targ->event) { - state = exec_hooks(th, list, targ, !vm_tracing); + if (list->events & trace_arg->event) { + state = exec_hooks(th, list, trace_arg, !vm_tracing); if (state) goto terminate; } th->errinfo = errinfo; } terminate: - th->trace_running = 0; + th->trace_arg = 0; th->vm->trace_running--; if (state) { @@ -330,11 +330,12 @@ rb_suppress_tracing(VALUE (*func)(VALUE) https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L330 VALUE result = Qnil; rb_thread_t *th = GET_THREAD(); int state; - const int tracing = th->trace_running; + const int tracing = th->trace_arg ? 1 : 0; + rb_trace_arg_t dummy_trace_arg; + + if (!tracing) th->vm->trace_running++; + if (!th->trace_arg) th->trace_arg = &dummy_trace_arg; - if (!tracing) - th->vm->trace_running++; - th->trace_running = 1; raised = rb_threadptr_reset_raised(th); outer_state = th->state; th->state = 0; @@ -348,9 +349,9 @@ rb_suppress_tracing(VALUE (*func)(VALUE) https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L349 if (raised) { rb_threadptr_set_raised(th); } - th->trace_running = tracing; - if (!tracing) - th->vm->trace_running--; + + if (th->trace_arg == &dummy_trace_arg) th->trace_arg = 0; + if (!tracing) th->vm->trace_running--; if (state) { JUMP_TAG(state); @@ -576,7 +577,6 @@ typedef struct rb_tp_struct { https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L577 void (*func)(VALUE tpval, void *data); void *data; VALUE proc; - rb_trace_arg_t *trace_arg; int tracing; VALUE self; } rb_tp_t; @@ -647,20 +647,20 @@ tpptr(VALUE tpval) https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L647 return tp; } -static void -tp_attr_check_active(rb_tp_t *tp) +static rb_trace_arg_t * +get_trace_arg(void) { - if (tp->trace_arg == 0) { + rb_trace_arg_t *trace_arg = GET_THREAD()->trace_arg; + if (trace_arg == 0) { rb_raise(rb_eRuntimeError, "access from outside"); } + return trace_arg; } struct rb_trace_arg_struct * rb_tracearg_from_tracepoint(VALUE tpval) { - rb_tp_t *tp = tpptr(tpval); - tp_attr_check_active(tp); - return tp->trace_arg; + return get_trace_arg(); } VALUE @@ -793,9 +793,7 @@ rb_tracearg_raised_exception(rb_trace_ar https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L793 static VALUE tracepoint_attr_event(VALUE tpval) { - rb_tp_t *tp = tpptr(tpval); - tp_attr_check_active(tp); - return rb_tracearg_event(tp->trace_arg); + return rb_tracearg_event(get_trace_arg()); } /* @@ -804,9 +802,7 @@ tracepoint_attr_event(VALUE tpval) https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L802 static VALUE tracepoint_attr_lineno(VALUE tpval) { - rb_tp_t *tp = tpptr(tpval); - tp_attr_check_active(tp); - return rb_tracearg_lineno(tp->trace_arg); + return rb_tracearg_lineno(get_trace_arg()); } /* @@ -815,9 +811,7 @@ tracepoint_attr_lineno(VALUE tpval) https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L811 static VALUE tracepoint_attr_path(VALUE tpval) { - rb_tp_t *tp = tpptr(tpval); - tp_attr_check_active(tp); - return rb_tracearg_path(tp->trace_arg); + return rb_tracearg_path(get_trace_arg()); } /* @@ -826,9 +820,7 @@ tracepoint_attr_path(VALUE tpval) https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L820 static VALUE tracepoint_attr_method_id(VALUE tpval) { - rb_tp_t *tp = tpptr(tpval); - tp_attr_check_active(tp); - return rb_tracearg_method_id(tp->trace_arg); + return rb_tracearg_method_id(get_trace_arg()); } /* @@ -868,9 +860,7 @@ tracepoint_attr_method_id(VALUE tpval) https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L860 static VALUE tracepoint_attr_defined_class(VALUE tpval) { - rb_tp_t *tp = tpptr(tpval); - tp_attr_check_active(tp); - return rb_tracearg_defined_class(tp->trace_arg); + return rb_tracearg_defined_class(get_trace_arg()); } /* @@ -879,9 +869,7 @@ tracepoint_attr_defined_class(VALUE tpva https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L869 static VALUE tracepoint_attr_binding(VALUE tpval) { - rb_tp_t *tp = tpptr(tpval); - tp_attr_check_active(tp); - return rb_tracearg_binding(tp->trace_arg); + return rb_tracearg_binding(get_trace_arg()); } /* @@ -893,9 +881,7 @@ tracepoint_attr_binding(VALUE tpval) https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L881 static VALUE tracepoint_attr_self(VALUE tpval) { - rb_tp_t *tp = tpptr(tpval); - tp_attr_check_active(tp); - return rb_tracearg_self(tp->trace_arg); + return rb_tracearg_self(get_trace_arg()); } /* @@ -904,9 +890,7 @@ tracepoint_attr_self(VALUE tpval) https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L890 static VALUE tracepoint_attr_return_value(VALUE tpval) { - rb_tp_t *tp = tpptr(tpval); - tp_attr_check_active(tp); - return rb_tracearg_return_value(tp->trace_arg); + return rb_tracearg_return_value(get_trace_arg()); } /* @@ -915,35 +899,19 @@ tracepoint_attr_return_value(VALUE tpval https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L899 static VALUE tracepoint_attr_raised_exception(VALUE tpval) { - rb_tp_t *tp = tpptr(tpval); - tp_attr_check_active(tp); - return rb_tracearg_raised_exception(tp->trace_arg); + return rb_tracearg_raised_exception(get_trace_arg()); } static void tp_call_trace(VALUE tpval, rb_trace_arg_t *trace_arg) { rb_tp_t *tp = tpptr(tpval); - rb_thread_t *th = GET_THREAD(); - int state; - tp->trace_arg = trace_arg; - - TH_PUSH_TAG(th); - if ((state = TH_EXEC_TAG()) == 0) { - if (tp->func) { - (*tp->func)(tpval, tp->data); - } - else { - rb_proc_call_with_block((VALUE)tp->proc, 1, &tpval, Qnil); - } + if (tp->func) { + (*tp->func)(tpval, tp->data); } - TH_POP_TAG(); - - tp->trace_arg = 0; - - if (state) { - TH_JUMP_TAG(th, state); + else { + rb_proc_call_with_block((VALUE)tp->proc, 1, &tpval, Qnil); } } @@ -1172,6 +1140,16 @@ rb_tracepoint_new(VALUE target_thread, r https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L1140 * p tp.raised_exception * end * #=> RuntimeError: 'raised_exception' not supported by this event + * + * If the trace method is called outside block, a RuntimeError is raised. + * + * TracePoint.trace(:line) do |tp| + * $tp = tp + * end + * $tp.line #=> access from outside (RuntimeError) + * + * Access from other threads is also forbidden. + * */ static VALUE tracepoint_new_s(int argc, VALUE *argv, VALUE self) @@ -1215,19 +1193,20 @@ static VALUE https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L1193 tracepoint_inspect(VALUE self) { rb_tp_t *tp = tpptr(self); + rb_trace_arg_t *trace_arg = GET_THREAD()->trace_arg; - if (tp->trace_arg) { - switch (tp->trace_arg->event) { + if (trace_arg) { + switch (trace_arg->event) { case RUBY_EVENT_LINE: case RUBY_EVENT_SPECIFIED_LINE: { - VALUE sym = rb_tracearg_method_id(tp->trace_arg); + VALUE sym = rb_tracearg_method_id(trace_arg); if (NIL_P(sym)) goto default_inspect; return rb_sprintf("#<TracePoint:%"PRIsVALUE"@%"PRIsVALUE":%d in `%"PRIsVALUE"'>", - rb_tracearg_event(tp->trace_arg), - rb_tracearg_path(tp->trace_arg), - FIX2INT(rb_tracearg_lineno(tp->trace_arg)), + rb_tracearg_event(trace_arg), + rb_tracearg_path(trace_arg), + FIX2INT(rb_tracearg_lineno(trace_arg)), sym); } case RUBY_EVENT_CALL: @@ -1235,21 +1214,21 @@ tracepoint_inspect(VALUE self) https://github.com/ruby/ruby/blob/trunk/vm_trace.c#L1214 case RUBY_EVENT_RETURN: case RUBY_EVENT_C_RETURN: return rb_sprintf("#<TracePoint:%"PRIsVALUE" `%"PRIsVALUE"'@%"PRIsVALUE":%d>", - rb_tracearg_event(tp->trace_arg), - rb_tracearg_method_id(tp->trace_arg), - rb_tracearg_path(tp->trace_arg), - FIX2INT(rb_tracearg_lineno(tp->trace_arg))); + rb_tracearg_event(trace_arg), + rb_tracearg_method_id(trace_arg), + rb_tracearg_path(trace_arg), + FIX2INT(rb_tracearg_lineno(trace_arg))); case RUBY_EVENT_THREAD_BEGIN: case RUBY_EVENT_THREAD_END: return rb_sprintf("#<TracePoint:%"PRIsVALUE" %"PRIsVALUE">", - rb_tracearg_event(tp->trace_arg), - rb_tracearg_self(tp->trace_arg)); + rb_tracearg_event(trace_arg), + rb_tracearg_self(trace_arg)); default: default_inspect: return rb_sprintf("#<TracePoint:%"PRIsVALUE"@%"PRIsVALUE":%d>", - rb_tracearg_event(tp->trace_arg), - rb_tracearg_path(tp->trace_arg), - FIX2INT(rb_tracearg_lineno(tp->trace_arg))); + rb_tracearg_event(trace_arg), + rb_tracearg_path(trace_arg), + FIX2INT(rb_tracearg_lineno(trace_arg))); } } else { Index: cont.c =================================================================== --- cont.c (revision 38523) +++ cont.c (revision 38524) @@ -924,7 +924,7 @@ rb_cont_call(int argc, VALUE *argv, VALU https://github.com/ruby/ruby/blob/trunk/cont.c#L924 cont->value = make_passing_arg(argc, argv); /* restore `tracing' context. see [Feature #4347] */ - th->trace_running = cont->saved_thread.trace_running; + th->trace_arg = cont->saved_thread.trace_arg; cont_restore_0(cont, &contval); return Qnil; /* unreachable */ @@ -1318,7 +1318,7 @@ fiber_switch(VALUE fibval, int argc, VAL https://github.com/ruby/ruby/blob/trunk/cont.c#L1318 } else { /* restore `tracing' context. see [Feature #4347] */ - th->trace_running = cont->saved_thread.trace_running; + th->trace_arg = cont->saved_thread.trace_arg; } cont->argc = argc; Index: test/ruby/test_settracefunc.rb =================================================================== --- test/ruby/test_settracefunc.rb (revision 38523) +++ test/ruby/test_settracefunc.rb (revision 38524) @@ -829,4 +829,23 @@ class TestSetTraceFunc < Test::Unit::Tes https://github.com/ruby/ruby/blob/trunk/test/ruby/test_settracefunc.rb#L829 assert_normal_exit('def m; end; TracePoint.new(:return) {raise}.enable {m}', '', timeout: 3) end end + + def test_tracepoint_with_multithreads + assert_nothing_raised do + TracePoint.new{ + 10.times{ + Thread.pass + } + }.enable do + (1..10).map{ + Thread.new{ + 1000.times{ + } + } + }.each{|th| + th.join + } + end + end + end end -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/