Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
From 5d95d27fb46bc347b042436a7d70356086391d67 Mon Sep 17 00:00:00 2001
From: Florian Gross <flgr@ccan.de>
Date: Mon, 6 Apr 2009 01:36:24 +0200
Subject: [PATCH] Introduced a new trace event vm-insn for tracing through VM instruction execution
---
include/ruby/ruby.h | 3 +-
test/test_trace.rb | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++
test/trace.rb | 56 -----------------------
thread.c | 11 ++++-
tool/instruction.rb | 1 +
vm_core.h | 1 +
vm_exec.h | 39 ++++++++++++++++
7 files changed, 174 insertions(+), 58 deletions(-)
create mode 100644 test/test_trace.rb
delete mode 100644 test/trace.rb
diff --git a/include/ruby/ruby.h b/include/ruby/ruby.h
index 1e619da..ce71774 100644
--- a/include/ruby/ruby.h
+++ b/include/ruby/ruby.h
@@ -1186,7 +1186,8 @@ int ruby_native_thread_p(void);
#define RUBY_EVENT_C_CALL 0x0020
#define RUBY_EVENT_C_RETURN 0x0040
#define RUBY_EVENT_RAISE 0x0080
-#define RUBY_EVENT_ALL 0xffff
+#define RUBY_EVENT_INSN 0x0100
+#define RUBY_EVENT_ALL (0xffff & ~RUBY_EVENT_INSN)
#define RUBY_EVENT_VM 0x10000
#define RUBY_EVENT_SWITCH 0x20000
#define RUBY_EVENT_COVERAGE 0x40000
diff --git a/test/test_trace.rb b/test/test_trace.rb
new file mode 100644
index 0000000..37b9231
--- /dev/null
+++ b/test/test_trace.rb
@@ -0,0 +1,121 @@
+require 'test/unit'
+
+def buggy(x, y)
+ return x / y * y
+end
+
+def something(c)
+ b=0 if c==6.1
+ begin
+ return buggy(5, b)
+ rescue
+ end
+ return 0
+end
+
+class TestTrace < Test::Unit::TestCase
+
+ @@msgs = []
+ def setup
+ @@msgs = []
+ end
+
+ def trace_func(event, file, line, id, binding, klass, *)
+ puts "#{file}:#{line} - #{event}" if $DEBUG
+ @@msgs << [event, file, line]
+ end
+
+ def test_trace_func_nil
+ assert_equal(nil, set_trace_func(nil), "set_trace_func(nil)")
+ end
+
+ def test_trace_mask_and_all
+ set_trace_func(method(:trace_func).to_proc, 0x0018) # Calls/returns
+ something(6.1)
+ set_trace_func(nil)
+ assert_equal(true,
+ @@msgs.inject do |result, triple|
+ result && ['call', 'return'].member?(triple[0])
+ end,
+ "trace - only call/return events"
+ )
+ call_return_event_count = @@msgs.size
+ @@msgs = []
+ puts '=' * 20 if $DEBUG
+ set_trace_func(method(:trace_func).to_proc)
+ something(6.1)
+ set_trace_func(nil)
+ assert_equal(true, @@msgs.size > call_return_event_count,
+ 'trace - all events. Should record more events.')
+ assert_equal(1, @@msgs.select {|x| x[0] == 'raise'}.size,
+ 'trace - all events. Should get a raise event')
+ end
+
+ def test_trace_insn_not_by_default
+ msgs = []
+
+ set_trace_func(lambda { |*x| msgs << x })
+ something(6.1)
+ set_trace_func(nil)
+
+ assert_equal(true, !msgs.any? { |type, *| type == "vm-insn" },
+ "trace - no vm-insn events by default")
+ end
+
+ def test_trace_insn
+ msgs = []
+
+ t_line = __LINE__; set_trace_func(lambda { |*x| msgs << x }, 0x0100)
+ something(6.1)
+ set_trace_func(nil)
+
+ assert_equal(true, msgs.all? { |type, *| type == "vm-insn" },
+ "trace - only vm-insn events")
+
+ assert_equal(true, msgs.all? { |type, file, line, *| line != t_line },
+ "trace - should not emit vm-insn events for set_trace_func")
+
+ events = msgs.map { |type, file, line, insn, *| insn }.uniq
+
+ [:putobject, :send, :jump].each do |event|
+ assert_equal(true, events.include?(event),
+ "trace - insn events should include %p" % event)
+ end
+ end
+
+ # This tests that the logic for omitting vm-insn events for the
+ # set_trace_func call itself even works when using multiple threads
+ def test_trace_insn_threaded
+ msgs1, msgs2 = [], []
+ t_line1 = t_line2 = nil
+
+ Thread.new do
+ t_line1 = __LINE__; set_trace_func(lambda { |*x| msgs1 << x }, 0x0100)
+
+ Thread.new do
+ # This will replace the trace_func set in the outer thread
+ t_line2 = __LINE__; set_trace_func(lambda { |*x| p x; msgs2 << x }, 0x0100)
+
+ Thread.new do
+ something(6.1)
+ end.join
+
+ something(6.1)
+ set_trace_func(nil)
+ end.join
+
+ something(6.1)
+ set_trace_func(nil)
+ end.join
+
+ [[msgs1, t_line1], [msgs2, t_line2]].each do |msgs, t_line|
+ assert_equal(true, msgs.all? { |type, *| type == "vm-insn" },
+ "trace - only vm-insn events")
+
+ assert_equal(true, msgs.all? { |type, file, line, *| line != t_line },
+ "trace - should not emit vm-insn events for set_trace_func")
+ end
+ end
+end
+
+
diff --git a/test/trace.rb b/test/trace.rb
deleted file mode 100644
index fda610d..0000000
--- a/test/trace.rb
+++ /dev/null
@@ -1,56 +0,0 @@
-require 'test/unit'
-
-def buggy(x, y)
- return x / y * y
-end
-
-def something(c)
- b=0 if c==6.1
- begin
- return buggy(5, b)
- rescue
- end
- return 0
-end
-
-class TestTrace < Test::Unit::TestCase
-
- @@msgs = []
- def setup
- @@msgs = []
- end
-
- def trace_func(event, file, line, id, binding, klass, *)
- puts "#{file}:#{line} - #{event}" if $DEBUG
- @@msgs << [event, file, line]
- end
-
- def test_trace_func_nil
- assert_equal(nil, set_trace_func(nil), "set_trace_func(nil)")
- end
-
- def test_trace_mask_and_all
- set_trace_func(method(:trace_func).to_proc, 0x0018) # Calls/returns
- something(6.1)
- set_trace_func(nil)
- assert_equal(true,
- @@msgs.inject do |result, triple|
- result && ['call', 'return'].member?(triple[0])
- end,
- "trace - only call/return events"
- )
- call_return_event_count = @@msgs.size
- @@msgs = []
- puts '=' * 20 if $DEBUG
- set_trace_func(method(:trace_func).to_proc)
- something(6.1)
- set_trace_func(nil)
- assert_equal(true, @@msgs.size > call_return_event_count,
- 'trace - all events. Should record more events.')
- assert_equal(1, @@msgs.select {|x| x[0] == 'raise'}.size,
- 'trace - all events. Should get a raise event')
-
- end
-end
-
-
diff --git a/thread.c b/thread.c
index ab6d0da..1f6af9c 100644
--- a/thread.c
+++ b/thread.c
@@ -3607,6 +3607,7 @@ set_trace_func(int argc, VALUE *argv)
rb_raise(rb_eTypeError, "trace_func needs to be Proc");
}
+ GET_THREAD()->trace_skip_insn_count = 2;
rb_add_event_hook(call_trace_func, mask, trace);
return trace;
}
@@ -3664,6 +3665,8 @@ get_event_name(rb_event_flag_t event)
return "c-return";
case RUBY_EVENT_RAISE:
return "raise";
+ case RUBY_EVENT_INSN:
+ return "vm-insn";
default:
return "unknown";
}
@@ -3685,6 +3688,11 @@ call_trace_proc(VALUE args, int tracing)
struct call_trace_func_args *p = (struct call_trace_func_args *)args;
const char *srcfile = rb_sourcefile();
VALUE eventname = rb_str_new2(get_event_name(p->event));
+
+ if (p->event == RUBY_EVENT_INSN &&
+ GET_THREAD()->trace_skip_insn_count-- > 0)
+ return Qnil;
+
VALUE filename = srcfile ? rb_str_new2(srcfile) : Qnil;
VALUE argv[6];
int line = rb_sourceline();
@@ -3692,7 +3700,8 @@ call_trace_proc(VALUE args, int tracing)
VALUE klass = 0;
if (p->event == RUBY_EVENT_C_CALL ||
- p->event == RUBY_EVENT_C_RETURN) {
+ p->event == RUBY_EVENT_C_RETURN ||
+ p->event == RUBY_EVENT_INSN) {
id = p->id;
klass = p->klass;
}
diff --git a/tool/instruction.rb b/tool/instruction.rb
index 7ecc84b..759ba13 100644
--- a/tool/instruction.rb
+++ b/tool/instruction.rb
@@ -847,6 +847,7 @@ class RubyVM
commit "INSN_ENTRY(#{insn.name}){"
make_header_prepare_stack insn
commit "{"
+ commit " TRACE_INSN(#{insn.name});"
make_header_stack_val insn
make_header_default_operands insn
make_header_operands insn
diff --git a/vm_core.h b/vm_core.h
index 511c1e3..4111b86 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -418,6 +418,7 @@ typedef struct rb_thread_struct
rb_event_hook_t *event_hooks;
rb_event_flag_t event_flags;
int tracing;
+ int trace_skip_insn_count;
/* fiber */
VALUE fiber;
diff --git a/vm_exec.h b/vm_exec.h
index 9ae8179..c802360 100644
--- a/vm_exec.h
+++ b/vm_exec.h
@@ -18,6 +18,45 @@ typedef unsigned long dindex_t;
typedef rb_num_t GENTRY;
typedef rb_iseq_t *ISEQ;
+#include "insns_info.inc"
+
+#define TRACE_INSN_EXTRA_INFO 1
+
+#if !TRACE_INSN_EXTRA_INFO
+
+#define TRACE_INSN_SET_EXTRA_INFO(insn, var) do { var = Qnil; } while (0)
+
+#else
+
+#define TRACE_INSN_SET_EXTRA_INFO(insn, var) do { \
+ var = rb_ary_new2(insn_len(BIN(insn)) - 1); \
+ int trace_i__ = 0; \
+ while (trace_i__ < insn_len(BIN(insn)) - 1) { \
+ VALUE trace_push_val__ = Qnil; \
+ switch (insn_op_type(BIN(insn), trace_i__)) { \
+ case 'V': trace_push_val__ = GET_OPERAND(trace_i__ + 1); break; \
+ case 'N': trace_push_val__ = INT2NUM(GET_OPERAND(trace_i__ + 1)); break; \
+ case 'L': trace_push_val__ = INT2NUM((int) (GET_LFP() - GET_OPERAND(trace_i__ + 1))); break; \
+ case 'D': trace_push_val__ = INT2NUM((int) (GET_DFP() - GET_OPERAND(trace_i__ + 1))); break; \
+ case 'I': trace_push_val__ = ID2SYM(GET_OPERAND(trace_i__ + 1)); break; \
+ } \
+ if (!SPECIAL_CONST_P(trace_push_val__)) \
+ trace_push_val__ = rb_type(trace_push_val__) == T_STRING ? \
+ rb_str_new_cstr(RSTRING_PTR(trace_push_val__)) : Qnil; \
+ rb_ary_push(var, trace_push_val__); \
+ trace_i__++; \
+ } \
+} while (0)
+
+#endif
+
+#define TRACE_INSN(insn) do { \
+ VALUE trace_ary__; \
+ TRACE_INSN_SET_EXTRA_INFO(insn, trace_ary__); \
+ EXEC_EVENT_HOOK(th, RUBY_EVENT_INSN, GET_SELF(), \
+ rb_intern(#insn), trace_ary__); \
+} while (0)
+
#ifdef COLLECT_USAGE_ANALYSIS
#define USAGE_ANALYSIS_INSN(insn) vm_analysis_insn(insn)
#define USAGE_ANALYSIS_OPERAND(insn, n, op) vm_analysis_operand(insn, n, (VALUE)op)
--
1.6.2.1+GitX
diff --git a/include/ruby/ruby.h b/include/ruby/ruby.h
index 1e619da..ce71774 100644
--- a/include/ruby/ruby.h
+++ b/include/ruby/ruby.h
@@ -1186,7 +1186,8 @@ int ruby_native_thread_p(void);
#define RUBY_EVENT_C_CALL 0x0020
#define RUBY_EVENT_C_RETURN 0x0040
#define RUBY_EVENT_RAISE 0x0080
-#define RUBY_EVENT_ALL 0xffff
+#define RUBY_EVENT_INSN 0x0100
+#define RUBY_EVENT_ALL (0xffff & ~RUBY_EVENT_INSN)
#define RUBY_EVENT_VM 0x10000
#define RUBY_EVENT_SWITCH 0x20000
#define RUBY_EVENT_COVERAGE 0x40000
diff --git a/test/test_trace.rb b/test/test_trace.rb
new file mode 100644
index 0000000..37b9231
--- /dev/null
+++ b/test/test_trace.rb
@@ -0,0 +1,121 @@
+require 'test/unit'
+
+def buggy(x, y)
+ return x / y * y
+end
+
+def something(c)
+ b=0 if c==6.1
+ begin
+ return buggy(5, b)
+ rescue
+ end
+ return 0
+end
+
+class TestTrace < Test::Unit::TestCase
+
+ @@msgs = []
+ def setup
+ @@msgs = []
+ end
+
+ def trace_func(event, file, line, id, binding, klass, *)
+ puts "#{file}:#{line} - #{event}" if $DEBUG
+ @@msgs << [event, file, line]
+ end
+
+ def test_trace_func_nil
+ assert_equal(nil, set_trace_func(nil), "set_trace_func(nil)")
+ end
+
+ def test_trace_mask_and_all
+ set_trace_func(method(:trace_func).to_proc, 0x0018) # Calls/returns
+ something(6.1)
+ set_trace_func(nil)
+ assert_equal(true,
+ @@msgs.inject do |result, triple|
+ result && ['call', 'return'].member?(triple[0])
+ end,
+ "trace - only call/return events"
+ )
+ call_return_event_count = @@msgs.size
+ @@msgs = []
+ puts '=' * 20 if $DEBUG
+ set_trace_func(method(:trace_func).to_proc)
+ something(6.1)
+ set_trace_func(nil)
+ assert_equal(true, @@msgs.size > call_return_event_count,
+ 'trace - all events. Should record more events.')
+ assert_equal(1, @@msgs.select {|x| x[0] == 'raise'}.size,
+ 'trace - all events. Should get a raise event')
+ end
+
+ def test_trace_insn_not_by_default
+ msgs = []
+
+ set_trace_func(lambda { |*x| msgs << x })
+ something(6.1)
+ set_trace_func(nil)
+
+ assert_equal(true, !msgs.any? { |type, *| type == "vm-insn" },
+ "trace - no vm-insn events by default")
+ end
+
+ def test_trace_insn
+ msgs = []
+
+ t_line = __LINE__; set_trace_func(lambda { |*x| msgs << x }, 0x0100)
+ something(6.1)
+ set_trace_func(nil)
+
+ assert_equal(true, msgs.all? { |type, *| type == "vm-insn" },
+ "trace - only vm-insn events")
+
+ assert_equal(true, msgs.all? { |type, file, line, *| line != t_line },
+ "trace - should not emit vm-insn events for set_trace_func")
+
+ events = msgs.map { |type, file, line, insn, *| insn }.uniq
+
+ [:putobject, :send, :jump].each do |event|
+ assert_equal(true, events.include?(event),
+ "trace - insn events should include %p" % event)
+ end
+ end
+
+ # This tests that the logic for omitting vm-insn events for the
+ # set_trace_func call itself even works when using multiple threads
+ def test_trace_insn_threaded
+ msgs1, msgs2 = [], []
+ t_line1 = t_line2 = nil
+
+ Thread.new do
+ t_line1 = __LINE__; set_trace_func(lambda { |*x| msgs1 << x }, 0x0100)
+
+ Thread.new do
+ # This will replace the trace_func set in the outer thread
+ t_line2 = __LINE__; set_trace_func(lambda { |*x| p x; msgs2 << x }, 0x0100)
+
+ Thread.new do
+ something(6.1)
+ end.join
+
+ something(6.1)
+ set_trace_func(nil)
+ end.join
+
+ something(6.1)
+ set_trace_func(nil)
+ end.join
+
+ [[msgs1, t_line1], [msgs2, t_line2]].each do |msgs, t_line|
+ assert_equal(true, msgs.all? { |type, *| type == "vm-insn" },
+ "trace - only vm-insn events")
+
+ assert_equal(true, msgs.all? { |type, file, line, *| line != t_line },
+ "trace - should not emit vm-insn events for set_trace_func")
+ end
+ end
+end
+
+
diff --git a/thread.c b/thread.c
index dacd85b..1f6af9c 100644
--- a/thread.c
+++ b/thread.c
@@ -3533,8 +3533,9 @@ static void call_trace_func(rb_event_flag_t, VALUE data, VALUE self, ID id, VALU
/*
* call-seq:
- * set_trace_func(proc) => proc
- * set_trace_func(nil) => nil
+ * set_trace_func(proc) => proc
+ * set_trace_func(proc, mask) => proc
+ * set_trace_func(nil) => nil
*
* Establishes _proc_ as the handler for tracing, or disables
* tracing if the parameter is +nil+. _proc_ takes up
@@ -3548,6 +3549,8 @@ static void call_trace_func(rb_event_flag_t, VALUE data, VALUE self, ID id, VALU
* <code>line</code> (execute code on a new line), <code>raise</code>
* (raise an exception), and <code>return</code> (return from a Ruby
* method). Tracing is disabled within the context of _proc_.
+ * _mask_ is an optional bitmask of events to trigger on, See ruby.h
+ * for the integer values. If no mask is specified all events are triggered.
*
* class Test
* def test
@@ -3572,11 +3575,28 @@ static void call_trace_func(rb_event_flag_t, VALUE data, VALUE self, ID id, VALU
* line prog.rb:3 test Test
* line prog.rb:4 test Test
* return prog.rb:4 test Test
+ *
+ * set_trace_func(proc { |event, file, line, id, binding, classname|
+ * printf "%8s %s:%-2d %10s %8s\n", event, file, line, id, classname
+ * }, 0x018) # 0x018 == calls and returns only
+ * t = Test.new
+ * t.test
+ *
+ * call prog.rb:2 test Test
+ * return prog.rb:4 test Test
+
*/
static VALUE
-set_trace_func(VALUE obj, VALUE trace)
+set_trace_func(int argc, VALUE *argv)
{
+ VALUE vmask;
+ VALUE trace;
+ int mask=RUBY_EVENT_ALL;
+ if (2 == rb_scan_args(argc, argv, "11", &trace, &vmask)) {
+ mask = NUM2INT(vmask);
+ }
+
rb_remove_event_hook(call_trace_func);
if (NIL_P(trace)) {
@@ -3587,7 +3607,8 @@ set_trace_func(VALUE obj, VALUE trace)
rb_raise(rb_eTypeError, "trace_func needs to be Proc");
}
- rb_add_event_hook(call_trace_func, RUBY_EVENT_ALL, trace);
+ GET_THREAD()->trace_skip_insn_count = 2;
+ rb_add_event_hook(call_trace_func, mask, trace);
return trace;
}
@@ -3644,6 +3665,8 @@ get_event_name(rb_event_flag_t event)
return "c-return";
case RUBY_EVENT_RAISE:
return "raise";
+ case RUBY_EVENT_INSN:
+ return "vm-insn";
default:
return "unknown";
}
@@ -3665,6 +3688,11 @@ call_trace_proc(VALUE args, int tracing)
struct call_trace_func_args *p = (struct call_trace_func_args *)args;
const char *srcfile = rb_sourcefile();
VALUE eventname = rb_str_new2(get_event_name(p->event));
+
+ if (p->event == RUBY_EVENT_INSN &&
+ GET_THREAD()->trace_skip_insn_count-- > 0)
+ return Qnil;
+
VALUE filename = srcfile ? rb_str_new2(srcfile) : Qnil;
VALUE argv[6];
int line = rb_sourceline();
@@ -3672,7 +3700,8 @@ call_trace_proc(VALUE args, int tracing)
VALUE klass = 0;
if (p->event == RUBY_EVENT_C_CALL ||
- p->event == RUBY_EVENT_C_RETURN) {
+ p->event == RUBY_EVENT_C_RETURN ||
+ p->event == RUBY_EVENT_INSN) {
id = p->id;
klass = p->klass;
}
@@ -3832,7 +3861,7 @@ Init_Thread(void)
rb_eThreadError = rb_define_class("ThreadError", rb_eStandardError);
/* trace */
- rb_define_global_function("set_trace_func", set_trace_func, 1);
+ rb_define_global_function("set_trace_func", set_trace_func, -1);
rb_define_method(rb_cThread, "set_trace_func", thread_set_trace_func_m, 1);
rb_define_method(rb_cThread, "add_trace_func", thread_add_trace_func_m, 1);
diff --git a/tool/instruction.rb b/tool/instruction.rb
index 7ecc84b..759ba13 100644
--- a/tool/instruction.rb
+++ b/tool/instruction.rb
@@ -847,6 +847,7 @@ class RubyVM
commit "INSN_ENTRY(#{insn.name}){"
make_header_prepare_stack insn
commit "{"
+ commit " TRACE_INSN(#{insn.name});"
make_header_stack_val insn
make_header_default_operands insn
make_header_operands insn
diff --git a/vm_core.h b/vm_core.h
index 511c1e3..4111b86 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -418,6 +418,7 @@ typedef struct rb_thread_struct
rb_event_hook_t *event_hooks;
rb_event_flag_t event_flags;
int tracing;
+ int trace_skip_insn_count;
/* fiber */
VALUE fiber;
diff --git a/vm_exec.h b/vm_exec.h
index 9ae8179..c802360 100644
--- a/vm_exec.h
+++ b/vm_exec.h
@@ -18,6 +18,45 @@ typedef unsigned long dindex_t;
typedef rb_num_t GENTRY;
typedef rb_iseq_t *ISEQ;
+#include "insns_info.inc"
+
+#define TRACE_INSN_EXTRA_INFO 1
+
+#if !TRACE_INSN_EXTRA_INFO
+
+#define TRACE_INSN_SET_EXTRA_INFO(insn, var) do { var = Qnil; } while (0)
+
+#else
+
+#define TRACE_INSN_SET_EXTRA_INFO(insn, var) do { \
+ var = rb_ary_new2(insn_len(BIN(insn)) - 1); \
+ int trace_i__ = 0; \
+ while (trace_i__ < insn_len(BIN(insn)) - 1) { \
+ VALUE trace_push_val__ = Qnil; \
+ switch (insn_op_type(BIN(insn), trace_i__)) { \
+ case 'V': trace_push_val__ = GET_OPERAND(trace_i__ + 1); break; \
+ case 'N': trace_push_val__ = INT2NUM(GET_OPERAND(trace_i__ + 1)); break; \
+ case 'L': trace_push_val__ = INT2NUM((int) (GET_LFP() - GET_OPERAND(trace_i__ + 1))); break; \
+ case 'D': trace_push_val__ = INT2NUM((int) (GET_DFP() - GET_OPERAND(trace_i__ + 1))); break; \
+ case 'I': trace_push_val__ = ID2SYM(GET_OPERAND(trace_i__ + 1)); break; \
+ } \
+ if (!SPECIAL_CONST_P(trace_push_val__)) \
+ trace_push_val__ = rb_type(trace_push_val__) == T_STRING ? \
+ rb_str_new_cstr(RSTRING_PTR(trace_push_val__)) : Qnil; \
+ rb_ary_push(var, trace_push_val__); \
+ trace_i__++; \
+ } \
+} while (0)
+
+#endif
+
+#define TRACE_INSN(insn) do { \
+ VALUE trace_ary__; \
+ TRACE_INSN_SET_EXTRA_INFO(insn, trace_ary__); \
+ EXEC_EVENT_HOOK(th, RUBY_EVENT_INSN, GET_SELF(), \
+ rb_intern(#insn), trace_ary__); \
+} while (0)
+
#ifdef COLLECT_USAGE_ANALYSIS
#define USAGE_ANALYSIS_INSN(insn) vm_analysis_insn(insn)
#define USAGE_ANALYSIS_OPERAND(insn, n, op) vm_analysis_operand(insn, n, (VALUE)op)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment