Feature #4347

Tracing cannot be re-enabled after callcc [patch]

Added by James M. Lawrence about 4 years ago. Updated over 2 years ago.

[ruby-core:34998]
Status:Closed
Priority:Normal
Assignee:Koichi Sasada

Description

=begin
% patch -p1 < test_continuation_tracing.patch
patching file test/ruby/test_continuation.rb
% ./ruby -v test/ruby/test_continuation.rb
ruby 1.9.3dev (2011-01-30 trunk 30735) [i386-darwin9.8.0]
Run options:

# Running tests:

......FF

Finished tests in 0.080788s, 99.0246 tests/s, 148.5369 assertions/s.

1) Failure:
test_tracing_with_set_trace_func(TestContinuation) [test/ruby/test_continuation.rb:99]:
expected but was
.

2) Failure:
test_tracing_with_thread_set_trace_func(TestContinuation) [test/ruby/test_continuation.rb:121]:
expected but was
.

In thread.c (thread_suppress_tracing) the code after (*func)(arg, running)
is not executed, causing th->tracing to not be cleared.

This two-line patch works, though it may only address a symptom.
=end

test_continuation_tracing.patch Magnifier (1.22 KB) James M. Lawrence, 01/31/2011 05:32 AM

continuation_tracing.patch Magnifier (553 Bytes) James M. Lawrence, 01/31/2011 05:32 AM

192_continuation_tracing.patch Magnifier - patch for 1.9.2 r30696 (514 Bytes) James M. Lawrence, 02/08/2011 04:30 AM

Associated revisions

Revision 32597
Added by Yusuke Endoh almost 4 years ago

  • thread.c (set_trace_func, thread_set_trace_func_m): reset tracing
    state when set_trace_func hook is removed. This is workaround patch
    to force to reset tracing state that is broken by continuation call.
    a patch from James M. Lawrence. [Feature #4347]

  • test/ruby/test_continuation.rb (class TestContinuation): add a test
    for above. a patch from James M. Lawrence.

Revision 32597
Added by Yusuke Endoh almost 4 years ago

  • thread.c (set_trace_func, thread_set_trace_func_m): reset tracing
    state when set_trace_func hook is removed. This is workaround patch
    to force to reset tracing state that is broken by continuation call.
    a patch from James M. Lawrence. [Feature #4347]

  • test/ruby/test_continuation.rb (class TestContinuation): add a test
    for above. a patch from James M. Lawrence.

Revision 36715
Added by Koichi Sasada over 2 years ago

  • vm_trace.c, vm_core.h: simplify tracing mechanism. (1) add rb_hook_list_t data structure which includes hooks, events (flag) and need_clean' flag. If the last flag is true, then clean the hooks list. In other words, deleted hooks are contained byhooks'. Cleanup process should run before traversing the list. (2) Change check mechanism See EXEC_EVENT_HOOK() in vm_core.h. (3) Add raw' hooks APIs Normal hooks are guarded from exception by rb_protect(). However, this protection is overhead for too simple functions which never cause exceptions.raw' hooks are executed without protection and faster. Now, we only provide registration APIs. All `raw' hooks are kicked under protection (same as normal hooks).
  • include/ruby/ruby.h: remove internal data definition and macros.
  • internal.h (ruby_suppress_tracing), vm_trace.c: rename ruby_suppress_tracing() to rb_suppress_tracing() and remove unused function parameter.
  • parse.y: fix to use renamed rb_suppress_tracing().
  • thread.c (thread_create_core): no need to set RUBY_VM_VM.
  • vm.c (mark_event_hooks): move definition to vm_trace.c.
  • vm.c (ruby_vm_event_flags): add a global variable. This global variable represents all of Threads and VM's event masks (T1#events | T2#events | ... | VM#events). You can check the possibility kick trace func or not with ruby_vm_event_flags. ruby_vm_event_flags is maintained by vm_trace.c.
  • cont.c (fiber_switch, rb_cont_call): restore tracing status. [Feature #4347]
  • test/ruby/test_continuation.rb: ditto.

Revision 36715
Added by Koichi Sasada over 2 years ago

  • vm_trace.c, vm_core.h: simplify tracing mechanism. (1) add rb_hook_list_t data structure which includes hooks, events (flag) and need_clean' flag. If the last flag is true, then clean the hooks list. In other words, deleted hooks are contained byhooks'. Cleanup process should run before traversing the list. (2) Change check mechanism See EXEC_EVENT_HOOK() in vm_core.h. (3) Add raw' hooks APIs Normal hooks are guarded from exception by rb_protect(). However, this protection is overhead for too simple functions which never cause exceptions.raw' hooks are executed without protection and faster. Now, we only provide registration APIs. All `raw' hooks are kicked under protection (same as normal hooks).
  • include/ruby/ruby.h: remove internal data definition and macros.
  • internal.h (ruby_suppress_tracing), vm_trace.c: rename ruby_suppress_tracing() to rb_suppress_tracing() and remove unused function parameter.
  • parse.y: fix to use renamed rb_suppress_tracing().
  • thread.c (thread_create_core): no need to set RUBY_VM_VM.
  • vm.c (mark_event_hooks): move definition to vm_trace.c.
  • vm.c (ruby_vm_event_flags): add a global variable. This global variable represents all of Threads and VM's event masks (T1#events | T2#events | ... | VM#events). You can check the possibility kick trace func or not with ruby_vm_event_flags. ruby_vm_event_flags is maintained by vm_trace.c.
  • cont.c (fiber_switch, rb_cont_call): restore tracing status. [Feature #4347]
  • test/ruby/test_continuation.rb: ditto.

History

#1 Updated by James M. Lawrence about 4 years ago

=begin

=end

#2 Updated by Yui NARUSE almost 4 years ago

  • Status changed from Open to Assigned
  • Assignee set to Koichi Sasada

#3 Updated by Hiroshi Nakamura almost 4 years ago

  • Target version changed from 2.0.0 to 1.9.3

#4 Updated by Yusuke Endoh almost 4 years ago

  • Assignee changed from Koichi Sasada to Yusuke Endoh

Hello,

2011/1/31 James M. Lawrence redmine@ruby-lang.org:

In thread.c (thread_suppress_tracing) the code after (*func)(arg, running)
is not executed, causing th->tracing to not be cleared.

This two-line patch works, though it may only address a symptom.

Thank you for your reporting and providing patch.

The root cause of this issue is that Continuation#call ignores
ensure clause:

callcc do |c|
begin
c.call
ensure
p :foo
end
end #=> nothing output

When Continuation#call is executed in the proc of set_trace_func,
it skips the post-processing of set_trace_func, resulting in
corrupt state of rb_thread_t#tracing.

Your patch forces to reset the state when set_trace_func(nil). But
to address this issue fundamentally, Continuation#call must call
current ensure clauses. It needs very hard work, though.

As you know, your patch is not essintial fix, but it works actually
in your example, and looks benign. So I'll import it.

But note that there is still other issues, and that this issue may
have a relapse in future. If you want not to hit a land mine, do
not use callcc seriously. It is very fun, joke feature, I think.

Thanks,

Yusuke Endoh mame@tsg.ne.jp

#5 Updated by Magnus Holm almost 4 years ago

But note that there is still other issues, and that this issue may
have a relapse in future.  If you want not to hit a land mine, do
not use callcc seriously.  It is very fun, joke feature, I think.

Gotos are a joke feature too… Any chance we can get them in a separate
library like callcc too? :D

#6 Updated by Motohiro KOSAKI almost 4 years ago

  • Tracker changed from Bug to Feature

#7 Updated by Motohiro KOSAKI almost 4 years ago

  • Target version changed from 1.9.3 to 2.0.0

#8 Updated by Yusuke Endoh almost 4 years ago

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

This issue was solved with changeset r32597.
James, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • thread.c (set_trace_func, thread_set_trace_func_m): reset tracing
    state when set_trace_func hook is removed. This is workaround patch
    to force to reset tracing state that is broken by continuation call.
    a patch from James M. Lawrence. [Feature #4347]

  • test/ruby/test_continuation.rb (class TestContinuation): add a test
    for above. a patch from James M. Lawrence.

#9 Updated by Yusuke Endoh almost 4 years ago

  • Status changed from Closed to Assigned
  • Assignee changed from Yusuke Endoh to Motohiro KOSAKI

Hello,

@James
Sorry, I didn't make the deadline to import your patch.
Kosaki, who is the "virtual" release manager, changed this ticket to
1.9.x feature.
I've just imported it to trunk. It is up to a release manager to
decide whether this is imported to 1.9.3 or not.

@Magnus
If anyone write a patch. :-)

Yusuke Endoh mame@tsg.ne.jp

#10 Updated by Motohiro KOSAKI almost 4 years ago

  • Assignee changed from Motohiro KOSAKI to Yusuke Endoh

@James
Sorry, I didn't make the deadline to import your patch.
Kosaki, who is the "virtual" release manager, changed this ticket to
1.9.x feature.
I've just imported it to trunk. It is up to a release manager to
decide whether this is imported to 1.9.3 or not.

Of course, I'd respect your decision. Please commit it to ruby_1_9_3 branch. ;-)

#11 Updated by Yusuke Endoh almost 4 years ago

  • Status changed from Assigned to Closed

Done. Thank you very much!

Yusuke Endoh mame@tsg.ne.jp

#12 Updated by Koichi Sasada over 2 years ago

  • Status changed from Closed to Assigned
  • Assignee changed from Yusuke Endoh to Koichi Sasada

I reopen this ticket. As you say, this is only workaround.

I solve this problem with restoring 'tracing state' (running trace func or not) at Continuation#call. I believe it is not a work around, but essential solution. I revert your proposal and set_trace_func(nil) does not clear tracing state any more.

With this change, your test code go to infinite loop:

example code

require 'continuation'

cont = nil

func = lambda{|*args|
p [1, args]
cont.call(nil)
}

3.times{
cont = callcc{|c| c}
# -> (1) call trace func (c-return), (2) call cont.call(nil) and come here!

p cont

if cont
set_trace_func(func)
else
set_trace_func(nil)
end
}

With my patch, this example work fine:

require 'continuation'

cont = nil
func = lambda{|*args|
p [1, args]
cont, c = nil, cont
c.call(nil) if c
}

3.times{|i|
p i
cont = callcc{|c| c}
p cont

if cont
set_trace_func(func)
end
}

I quote mame-san's comment, again.

But note that there is still other issues, and that this issue may have a relapse in future. If you want not to hit a land mine, do not use callcc seriously. It is very fun, joke feature, I think.

#13 Updated by Koichi Sasada over 2 years ago

  • Status changed from Assigned to Closed

This issue was solved with changeset r36715.
James, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • vm_trace.c, vm_core.h: simplify tracing mechanism. (1) add rb_hook_list_t data structure which includes hooks, events (flag) and need_clean' flag. If the last flag is true, then clean the hooks list. In other words, deleted hooks are contained byhooks'. Cleanup process should run before traversing the list. (2) Change check mechanism See EXEC_EVENT_HOOK() in vm_core.h. (3) Add raw' hooks APIs Normal hooks are guarded from exception by rb_protect(). However, this protection is overhead for too simple functions which never cause exceptions.raw' hooks are executed without protection and faster. Now, we only provide registration APIs. All `raw' hooks are kicked under protection (same as normal hooks).
  • include/ruby/ruby.h: remove internal data definition and macros.
  • internal.h (ruby_suppress_tracing), vm_trace.c: rename ruby_suppress_tracing() to rb_suppress_tracing() and remove unused function parameter.
  • parse.y: fix to use renamed rb_suppress_tracing().
  • thread.c (thread_create_core): no need to set RUBY_VM_VM.
  • vm.c (mark_event_hooks): move definition to vm_trace.c.
  • vm.c (ruby_vm_event_flags): add a global variable. This global variable represents all of Threads and VM's event masks (T1#events | T2#events | ... | VM#events). You can check the possibility kick trace func or not with ruby_vm_event_flags. ruby_vm_event_flags is maintained by vm_trace.c.
  • cont.c (fiber_switch, rb_cont_call): restore tracing status. [Feature #4347]
  • test/ruby/test_continuation.rb: ditto.

Also available in: Atom PDF