Merge pull request #3040 from xmcclure/debugger-step-recursive
authorAndi McClure <andi.mcclure@xamarin.com>
Fri, 3 Jun 2016 17:08:23 +0000 (13:08 -0400)
committerAndi McClure <andi.mcclure@xamarin.com>
Fri, 3 Jun 2016 17:08:23 +0000 (13:08 -0400)
Debugger stepping does not understand recursion (bug 38025)

1  2 
mcs/class/Mono.Debugger.Soft/Test/dtest.cs
mono/mini/debugger-agent.c

index cd0b6ae51ba33ef260f685dae6b0930e66c324ee,c07ad14848e2a0736a223c504e3af671a1806fd9..c0af12cb38ac15d9f023d11cfffa62408e2fb559
@@@ -42,6 -42,17 +42,17 @@@ public class DebuggerTest
        public static string runtime = Environment.GetEnvironmentVariable ("DBG_RUNTIME");
        public static string agent_args = Environment.GetEnvironmentVariable ("DBG_AGENT_ARGS");
  
+       // Not currently used, but can be useful when debugging individual tests.
+       void StackTraceDump (Event e)
+       {
+               int i = 0;
+               foreach (var frame in e.Thread.GetFrames ())
+               {
+                       i++;
+                       Console.WriteLine ("Frame " + i + ", " + frame.Method.Name);
+               }
+       }
        Event GetNextEvent () {
                var es = vm.GetNextEventSet ();
                Assert.AreEqual (1, es.Events.Length);
                return (e as BreakpointEvent);
        }
  
+       class ReusableBreakpoint {
+               DebuggerTests owner;
+               public string method_name;
+               public BreakpointEventRequest req;
+               public BreakpointEvent lastEvent = null;
+               public ReusableBreakpoint (DebuggerTests owner, string method_name)
+               {
+                       this.owner = owner;
+                       this.method_name = method_name;
+                       MethodMirror m = owner.entry_point.DeclaringType.GetMethod (method_name);
+                       Assert.IsNotNull (m);
+                       req = owner.vm.SetBreakpoint (m, m.ILOffsets [0]);
+               }
+               public void Continue ()
+               {
+                       bool survived = false;
+                       try {
+                               Event e = null;
+                               while (true) {
+                                       owner.vm.Resume ();
+                                       e = owner.GetNextEvent ();
+                                       if (e is BreakpointEvent)
+                                               break;
+                               }
+                               Assert.IsInstanceOfType (typeof (BreakpointEvent), e);
+                               Assert.AreEqual (method_name, (e as BreakpointEvent).Method.Name);
+                               lastEvent = e as BreakpointEvent;
+                               survived = true;
+                       } finally {
+                               if (!survived) { // Ensure cleanup if we triggered an assert
+                                       Disable ();
+                               }
+                       }
+               }
+               public void Disable ()
+               {
+                       req.Disable ();
+               }
+       }
+       /* One of the tests executes a complex tree of recursive functions.
+          The only good way to specify how its behavior should appear from this side
+          is to just run the function tree once over here and record what it does. */
+       public struct RecursiveChaoticPoint
+       {
+               public bool breakpoint;
+               public string name;
+               public int depth;
+               public RecursiveChaoticPoint (bool breakpoint, string name, int depth)
+               {
+                       this.breakpoint = breakpoint;
+                       this.name = name;
+                       this.depth = depth;
+               }
+       }
+       // The breakpoint is placed here in dtest-app.cs
+       public static void ss_recursive_chaotic_trap (int n, List<RecursiveChaoticPoint> trace, ref bool didLast, ref bool didAny)
+       {
+               // Depth is calculated as:
+               // Main + single_stepping + ss_recursive_chaotic + (n is 5 at outermost frame and 0 at innermost frame) + ss_recursive_chaotic_trap
+               trace.Add (new RecursiveChaoticPoint (true, "ss_recursive_chaotic_trap", 5 - n + 5));
+               didLast = true;
+       }
+       public static void ss_recursive_chaotic_at (string at, int n, List<RecursiveChaoticPoint> trace, ref bool didLast, ref bool didAny)
+       {
+               // This will be called after every return from a function. The other function will return whether "step out" is currently active, and it will be passed in here as didLast.
+               if (didLast) {
+                       // Depth is calculated as:
+                       // Main + single_stepping + ss_recursive_chaotic + (n is 5 at outermost frame and 0 at innermost frame)
+                       trace.Add (new RecursiveChaoticPoint (false, "ss_recursive_chaotic_" + at, 5 - n + 4));
+                       didAny = true;
+                       didLast = false;
+               }
+       }
+       public static bool ss_recursive_chaotic_fizz (int n, List<RecursiveChaoticPoint> trace)
+       {
+               bool didLast = false, didAny = false;
+               if (n > 0) {
+                       int next = n - 1;
+                       didLast = ss_recursive_chaotic_buzz (next, trace);
+                       ss_recursive_chaotic_at ("fizz", n, trace, ref didLast, ref didAny);
+                       didLast = ss_recursive_chaotic_fizzbuzz (next, trace);
+                       ss_recursive_chaotic_at ("fizz", n, trace, ref didLast, ref didAny);
+               } else {
+                       ss_recursive_chaotic_trap (n, trace, ref didLast, ref didAny);
+                       ss_recursive_chaotic_at ("fizz", n, trace, ref didLast, ref didAny);
+               }
+               return didAny;
+       }
+       public static bool ss_recursive_chaotic_buzz (int n, List<RecursiveChaoticPoint> trace)
+       {
+               bool didLast = false, didAny = false;
+               if (n > 0) {
+                       int next = n - 1;
+                       didLast = ss_recursive_chaotic_fizz (next, trace);
+                       ss_recursive_chaotic_at ("buzz", n, trace, ref didLast, ref didAny);
+                       didLast = ss_recursive_chaotic_fizzbuzz (next, trace);
+                       ss_recursive_chaotic_at ("buzz", n, trace, ref didLast, ref didAny);
+               }
+               return didAny;
+       }
+       public static bool ss_recursive_chaotic_fizzbuzz (int n, List<RecursiveChaoticPoint> trace)
+       {
+               bool didLast = false, didAny = false;
+               if (n > 0) {
+                       int next = n - 1;
+                       didLast = ss_recursive_chaotic_fizz (next, trace);
+                       ss_recursive_chaotic_at ("fizzbuzz", n, trace, ref didLast, ref didAny);
+                       didLast = ss_recursive_chaotic_buzz (next, trace);
+                       ss_recursive_chaotic_at ("fizzbuzz", n, trace, ref didLast, ref didAny);
+                       didLast = ss_recursive_chaotic_fizzbuzz (next, trace);
+                       ss_recursive_chaotic_at ("fizzbuzz", n, trace, ref didLast, ref didAny);
+               }
+               return didAny;
+       }
+       public static void trace_ss_recursive_chaotic (List<RecursiveChaoticPoint> trace)
+       {
+               ss_recursive_chaotic_fizz (5, trace);
+       }
        Event single_step (ThreadMirror t) {
                var req = vm.CreateStepRequest (t);
                req.Enable ();
                Assert.AreEqual (m2.Name, (e as BreakpointEvent).Method.Name);
        }
  
+       // Assert we have stepped to a location
        void assert_location (Event e, string method) {
                Assert.IsTrue (e is StepEvent);
                Assert.AreEqual (method, (e as StepEvent).Method.Name);
        }
  
+       // Assert we have breakpointed at a location
+       void assert_location_at_breakpoint (Event e, string method) {
+               Assert.IsTrue (e is BreakpointEvent);
+               Assert.AreEqual (method, (e as BreakpointEvent).Method.Name);
+       }
+       // Assert we have stepped to or breakpointed at a location
+       void assert_location_allow_breakpoint (Event e, string method) {
+               if (e is StepEvent)
+                       Assert.AreEqual (method, (e as StepEvent).Method.Name);
+               else if (e is BreakpointEvent)
+                       Assert.AreEqual (method, (e as BreakpointEvent).Method.Name);
+               else
+                       Assert.Fail ("Neither step nor breakpoint event");
+       }
        StepEventRequest create_step (Event e) {
                var req = vm.CreateStepRequest (e.Thread);
                step_req = req;
                AssertValue (1, f.GetValue (f.Method.GetLocal ("n")));
                req.Disable ();
  
+               // Check that step-over stops correctly when inner frames with recursive functions contain breakpoints
+               e = run_until ("ss_recursive2");
+               ReusableBreakpoint breakpoint = new ReusableBreakpoint (this, "ss_recursive2_trap");
+               try {
+                       breakpoint.Continue ();
+                       e = breakpoint.lastEvent;
+                       req = create_step (e);
+                       for (int c = 1; c <= 4; c++) {
+                               // The first five times we try to step over this function, the breakpoint will stop us
+                               assert_location_at_breakpoint (e, "ss_recursive2_trap");
+                               req.Disable ();
+                               req = create_step (e);
+                               req.Size = StepSize.Line;
+                               e = step_out ();
+                               assert_location (e, "ss_recursive2");
+                               // Stack should consist of Main + single_stepping + (1 ss_recursive2 frame per loop iteration)
+                               Assert.AreEqual (c+2, e.Thread.GetFrames ().Length);
+                               e = step_over_or_breakpoint ();
+                       }
+                       // At this point we should have escaped the breakpoints and this will be a normal step stop
+                       assert_location (e, "ss_recursive2");
+                       Assert.AreEqual (6, e.Thread.GetFrames ().Length);
+               } finally {
+                       req.Disable ();
+                       breakpoint.Disable ();
+               }
+               // Check that step-out stops correctly when inner frames with recursive functions contain breakpoints
+               e = run_until ("ss_recursive2");
+               breakpoint = new ReusableBreakpoint (this, "ss_recursive2_trap");
+               try {
+                       breakpoint.Continue ();
+                       e = breakpoint.lastEvent;
+                       req = create_step (e);
+                       for (int c = 1; c <= 4; c++) {
+                               // The first five times we try to step over this function, the breakpoint will stop us
+                               assert_location_at_breakpoint (e, "ss_recursive2_trap");
+                               req.Disable ();
+                               req = create_step (e);
+                               req.Size = StepSize.Line;
+                               e = step_out ();
+                               assert_location (e, "ss_recursive2");
+                               // Stack should consist of Main + single_stepping + (1 ss_recursive2 frame per loop iteration)
+                               Assert.AreEqual (c+2, e.Thread.GetFrames ().Length);
+                               e = step_out_or_breakpoint ();
+                       }
+                       for (int c = 3; c >= 1; c--) {
+                               assert_location (e, "ss_recursive2");
+                               Assert.AreEqual (c + 2, e.Thread.GetFrames ().Length);
+                               e = step_out ();
+                       }
+               } finally {
+                       req.Disable ();
+                       breakpoint.Disable ();
+               }
+               // Test step out with a really complicated call tree
+               List<RecursiveChaoticPoint> trace = new List<RecursiveChaoticPoint>();
+               trace_ss_recursive_chaotic (trace);
+               e = run_until ("ss_recursive_chaotic");
+               try {
+                       breakpoint = new ReusableBreakpoint (this, "ss_recursive_chaotic_trap");
+                       breakpoint.Continue ();
+                       e = breakpoint.lastEvent;
+                       foreach (RecursiveChaoticPoint point in trace)
+                       {
+                               if (point.breakpoint)
+                                       assert_location_at_breakpoint (e, point.name);
+                               else
+                                       assert_location (e, point.name);
+                               Assert.AreEqual (point.depth, e.Thread.GetFrames ().Length);
+                               req.Disable ();
+                               req = create_step (e);
+                               req.Size = StepSize.Line;
+                               e = step_out_or_breakpoint ();
+                       }
+               } finally {
+                       req.Disable ();
+                       breakpoint.Disable ();
+               }
                // Check that single stepping doesn't clobber fp values
                e = run_until ("ss_fp_clobber");
                req = create_step (e);
  
                TypeMirror t = o.Type;
  
 -              Assert.AreEqual ("MonoType", t.GetTypeObject ().Type.Name);
 +              Assert.AreEqual ("RuntimeType", t.GetTypeObject ().Type.Name);
        }
  
        [Test]
                return step_once ();
        }
  
+       Event step_once_or_breakpoint () {
+               vm.Resume ();
+               var e = GetNextEvent ();
+               Assert.IsTrue (e is StepEvent || e is BreakpointEvent);
+               return e;
+       }
+       Event step_over_or_breakpoint () {
+               step_req.Disable ();
+               step_req.Depth = StepDepth.Over;
+               step_req.Enable ();
+               return step_once_or_breakpoint ();
+       }
+       Event step_out_or_breakpoint () {
+               step_req.Disable ();
+               step_req.Depth = StepDepth.Out;
+               step_req.Enable ();
+               return step_once_or_breakpoint ();
+       }
        [Test]
        public void Locals () {
                var be = run_until ("locals1");
index f7b4426bad376351151d3d9f15b2280ecf62b6ad,017315fe02f61be11d1d21b8fd66a520fdd4ebe5..e567ed30fa797879ff151a594832f666603638f2
@@@ -1953,7 -1953,7 +1953,7 @@@ get_objref (MonoObject *obj
        int hash = 0;
  
        if (obj == NULL)
 -              return 0;
 +              return NULL;
  
        if (suspend_count) {
                /*
@@@ -2028,10 -2028,7 +2028,10 @@@ clear_suspended_objs (void
  static inline int
  get_objid (MonoObject *obj)
  {
 -      return get_objref (obj)->id;
 +      if (!obj)
 +              return 0;
 +      else
 +              return get_objref (obj)->id;
  }
  
  /*
@@@ -2681,7 -2678,7 +2681,7 @@@ notify_thread (gpointer key, gpointer v
  
        DEBUG_PRINTF (1, "[%p] Interrupting %p...\n", (gpointer) (gsize) mono_native_thread_id_get (), (gpointer)tid);
  
 -      /* This is _not_ equivalent to ves_icall_System_Threading_Thread_Abort () */
 +      /* This is _not_ equivalent to mono_thread_internal_abort () */
        InterruptData interrupt_data = { 0 };
        interrupt_data.tls = tls;
  
@@@ -3629,15 -3626,11 +3629,15 @@@ process_event (EventKind event, gpointe
  
                ecount ++;
  
 -              if (!thread)
 -                      thread = mono_thread_current ();
 +              if (event == EVENT_KIND_VM_DEATH) {
 +                      thread = NULL;
 +              } else {
 +                      if (!thread)
 +                              thread = mono_thread_current ();
  
 -              if (event == EVENT_KIND_VM_START && arg != NULL)
 -                      thread = (MonoThread *)arg;
 +                      if (event == EVENT_KIND_VM_START && arg != NULL)
 +                              thread = (MonoThread *)arg;
 +              }
  
                buffer_add_objid (&buf, (MonoObject*)thread); // thread
  
@@@ -3799,7 -3792,7 +3799,7 @@@ runtime_initialized (MonoProfiler *prof
  static void
  runtime_shutdown (MonoProfiler *prof)
  {
 -      process_profiler_event (EVENT_KIND_VM_DEATH, mono_thread_current ());
 +      process_profiler_event (EVENT_KIND_VM_DEATH, NULL);
  
        mono_debugger_agent_cleanup ();
  }
@@@ -4327,12 -4320,9 +4327,12 @@@ set_bp_in_method (MonoDomain *domain, M
  
        code = mono_jit_find_compiled_method_with_jit_info (domain, method, &ji);
        if (!code) {
 +              MonoError oerror;
 +
                /* Might be AOTed code */
 -              code = mono_aot_get_method (domain, method);
 +              code = mono_aot_get_method_checked (domain, method, &oerror);
                g_assert (code);
 +              mono_error_assert_ok (&oerror);
                ji = mono_jit_info_table_find (domain, (char *)code);
                g_assert (ji);
        }
@@@ -4515,6 -4505,18 +4515,18 @@@ clear_breakpoints_for_domain (MonoDomai
        mono_loader_unlock ();
  }
  
+ /*
+  * ss_calculate_framecount:
+  *
+  * Ensure DebuggerTlsData fields are filled out.
+  */
+ static void ss_calculate_framecount (DebuggerTlsData *tls, MonoContext *ctx)
+ {
+       if (!tls->context.valid)
+               mono_thread_state_init_from_monoctx (&tls->context, ctx);
+       compute_frame_info (tls->thread, tls);
+ }
  /*
   * ss_update:
   *
@@@ -4536,22 -4538,24 +4548,24 @@@ ss_update (SingleStepReq *req, MonoJitI
                return FALSE;
        }
  
-       if (req->depth == STEP_DEPTH_OVER && hit) {
-               if (!tls->context.valid)
-                       mono_thread_state_init_from_monoctx (&tls->context, ctx);
-               compute_frame_info (tls->thread, tls);
-               if (req->nframes && tls->frame_count && tls->frame_count > req->nframes) {
-                       /* Hit the breakpoint in a recursive call */
-                       DEBUG_PRINTF (1, "[%p] Breakpoint at lower frame while stepping over, continuing single stepping.\n", (gpointer) (gsize) mono_native_thread_id_get ());
+       if ((req->depth == STEP_DEPTH_OVER || req->depth == STEP_DEPTH_OUT) && hit) {
+               gboolean is_step_out = req->depth == STEP_DEPTH_OUT;
+               ss_calculate_framecount (tls, ctx);
+               // Because functions can call themselves recursively, we need to make sure we're stopping at the right stack depth.
+               // In case of step out, the target is the frame *enclosing* the one where the request was made.
+               int target_frames = req->nframes + (is_step_out ? -1 : 0);
+               if (req->nframes > 0 && tls->frame_count > 0 && tls->frame_count > target_frames) {
+                       /* Hit the breakpoint in a recursive call, don't halt */
+                       DEBUG_PRINTF (1, "[%p] Breakpoint at lower frame while stepping %s, continuing single stepping.\n", (gpointer) (gsize) mono_native_thread_id_get (), is_step_out ? "out" : "over");
                        return FALSE;
                }
        }
  
        if (req->depth == STEP_DEPTH_INTO && req->size == STEP_SIZE_MIN && (sp->flags & MONO_SEQ_POINT_FLAG_NONEMPTY_STACK) && ss_req->start_method){
                method = jinfo_get_method (ji);
-               if (!tls->context.valid)
-                       mono_thread_state_init_from_monoctx (&tls->context, ctx);
-               compute_frame_info (tls->thread, tls);
+               ss_calculate_framecount (tls, ctx);
                if (ss_req->start_method == method && req->nframes && tls->frame_count == req->nframes) {//Check also frame count(could be recursion)
                        DEBUG_PRINTF (1, "[%p] Seq point at nonempty stack %x while stepping in, continuing single stepping.\n", (gpointer) (gsize) mono_native_thread_id_get (), sp->il_offset);
                        return FALSE;
                ss_req->last_method = method;
                hit = FALSE;
        } else if (loc && method == ss_req->last_method && loc->row == ss_req->last_line) {
-               DEBUG_PRINTF (1, "[%p] Same source line (%d), continuing single stepping.\n", (gpointer) (gsize) mono_native_thread_id_get (), loc->row);
-               hit = FALSE;
+               ss_calculate_framecount (tls, ctx);
+               if (tls->frame_count == req->nframes) { // If the frame has changed we're clearly not on the same source line.
+                       DEBUG_PRINTF (1, "[%p] Same source line (%d), continuing single stepping.\n", (gpointer) (gsize) mono_native_thread_id_get (), loc->row);
+                       hit = FALSE;
+               }
        }
                                
        if (loc) {
@@@ -5080,6 -5087,85 +5097,85 @@@ ss_stop (SingleStepReq *ss_req
        }
  }
  
+ /*
+  * ss_bp_is_unique:
+  *
+  * Reject breakpoint if it is a duplicate of one already in list or hash table.
+  */
+ static gboolean
+ ss_bp_is_unique (GSList *bps, GHashTable *ss_req_bp_cache, MonoMethod *method, guint32 il_offset)
+ {
+       if (ss_req_bp_cache) {
+               MonoBreakpoint dummy = {method, il_offset, NULL, NULL};
+               return !g_hash_table_lookup (ss_req_bp_cache, &dummy);
+       }
+       for (GSList *l = bps; l; l = l->next) {
+               MonoBreakpoint *bp = (MonoBreakpoint *)l->data;
+               if (bp->method == method && bp->il_offset == il_offset)
+                       return FALSE;
+       }
+       return TRUE;
+ }
+ /*
+  * ss_bp_eq:
+  *
+  * GHashTable equality for a MonoBreakpoint (only care about method and il_offset fields)
+  */
+ static gint
+ ss_bp_eq (gconstpointer ka, gconstpointer kb)
+ {
+       const MonoBreakpoint *s1 = (const MonoBreakpoint *)ka;
+       const MonoBreakpoint *s2 = (const MonoBreakpoint *)kb;
+       return (s1->method == s2->method && s1->il_offset == s2->il_offset) ? 1 : 0;
+ }
+ /*
+  * ss_bp_eq:
+  *
+  * GHashTable hash for a MonoBreakpoint (only care about method and il_offset fields)
+  */
+ static guint
+ ss_bp_hash (gconstpointer data)
+ {
+       const MonoBreakpoint *s = (const MonoBreakpoint *)data;
+       guint hash = (guint) (uintptr_t) s->method;
+       hash ^= ((guint)s->il_offset) << 16; // Assume low bits are more interesting
+       hash ^= ((guint)s->il_offset) >> 16;
+       return hash;
+ }
+ #define MAX_LINEAR_SCAN_BPS 7
+ /*
+  * ss_bp_add_one:
+  *
+  * Create a new breakpoint and add it to a step request.
+  * Will adjust the bp count and cache used by ss_start.
+  */
+ static void
+ ss_bp_add_one (SingleStepReq *ss_req, int *ss_req_bp_count, GHashTable **ss_req_bp_cache,
+                 MonoMethod *method, guint32 il_offset)
+ {
+       // This list is getting too long, switch to using the hash table
+       if (!*ss_req_bp_cache && *ss_req_bp_count > MAX_LINEAR_SCAN_BPS) {
+               *ss_req_bp_cache = g_hash_table_new (ss_bp_hash, ss_bp_eq);
+               for (GSList *l = ss_req->bps; l; l = l->next)
+                       g_hash_table_insert (*ss_req_bp_cache, l->data, l->data);
+       }
+       if (ss_bp_is_unique (ss_req->bps, *ss_req_bp_cache, method, il_offset)) {
+               // Create and add breakpoint
+               MonoBreakpoint *bp = set_breakpoint (method, il_offset, ss_req->req, NULL);
+               ss_req->bps = g_slist_append (ss_req->bps, bp);
+               if (*ss_req_bp_cache)
+                       g_hash_table_insert (*ss_req_bp_cache, bp, bp);
+               (*ss_req_bp_count)++;
+       } else {
+               DEBUG_PRINTF (1, "[dbg] Candidate breakpoint at %s:[il=0x%x] is a duplicate for this step request, will not add.\n", mono_method_full_name (method, TRUE), (int)il_offset);
+       }
+ }
  /*
   * ss_start:
   *
@@@ -5096,11 -5182,15 +5192,15 @@@ ss_start (SingleStepReq *ss_req, MonoMe
        SeqPoint *next_sp, *parent_sp = NULL;
        SeqPoint local_sp, local_parent_sp;
        gboolean found_sp;
-       MonoBreakpoint *bp;
        MonoSeqPointInfo *parent_info;
        MonoMethod *parent_sp_method = NULL;
        gboolean enable_global = FALSE;
  
+       // When 8 or more entries are in bps, we build a hash table to serve as a set of breakpoints.
+       // Recreating this on each pass is a little wasteful but at least keeps behavior linear.
+       int ss_req_bp_count = g_slist_length (ss_req->bps);
+       GHashTable *ss_req_bp_cache = NULL;
        /* Stop the previous operation */
        ss_stop (ss_req);
  
         * Implement single stepping using breakpoints if possible.
         */
        if (step_to_catch) {
-               bp = set_breakpoint (method, sp->il_offset, ss_req->req, NULL);
-               ss_req->bps = g_slist_append (ss_req->bps, bp);
+               ss_bp_add_one (ss_req, &ss_req_bp_count, &ss_req_bp_cache, method, sp->il_offset);
        } else {
                frame_index = 1;
  
                        for (i = 0; i < sp->next_len; i++) {
                                next_sp = &next[i];
  
-                               bp = set_breakpoint (method, next_sp->il_offset, ss_req->req, NULL);
-                               ss_req->bps = g_slist_append (ss_req->bps, bp);
+                               ss_bp_add_one (ss_req, &ss_req_bp_count, &ss_req_bp_cache, method, next_sp->il_offset);
                        }
                        g_free (next);
                }
                        for (i = 0; i < parent_sp->next_len; i++) {
                                next_sp = &next[i];
  
-                               bp = set_breakpoint (parent_sp_method, next_sp->il_offset, ss_req->req, NULL);
-                               ss_req->bps = g_slist_append (ss_req->bps, bp);
+                               ss_bp_add_one (ss_req, &ss_req_bp_count, &ss_req_bp_cache, parent_sp_method, next_sp->il_offset);
                        }
                        g_free (next);
                }
  
                                                found_sp = mono_find_next_seq_point_for_native_offset (frame->domain, frame->method, (char*)ei->handler_start - (char*)jinfo->code_start, NULL, &local_sp);
                                                sp = (found_sp)? &local_sp : NULL;
-                                               if (sp) {
-                                                       bp = set_breakpoint (frame->method, sp->il_offset, ss_req->req, NULL);
-                                                       ss_req->bps = g_slist_append (ss_req->bps, bp);
-                                               }
+                                               ss_bp_add_one (ss_req, &ss_req_bp_count, &ss_req_bp_cache, frame->method, sp->il_offset);
                                        }
                                }
                        }
        } else {
                ss_req->global = FALSE;
        }
+       if (ss_req_bp_cache)
+               g_hash_table_destroy (ss_req_bp_cache);
  }
  
  /*
@@@ -5437,7 -5525,6 +5535,7 @@@ ss_clear_for_assembly (SingleStepReq *r
  void
  mono_debugger_agent_debug_log (int level, MonoString *category, MonoString *message)
  {
 +      MonoError error;
        int suspend_policy;
        GSList *events;
        EventInfo ei;
        mono_loader_unlock ();
  
        ei.level = level;
 -      ei.category = category ? mono_string_to_utf8 (category) : NULL;
 -      ei.message = message ? mono_string_to_utf8 (message) : NULL;
 +      ei.category = NULL;
 +      if (category) {
 +              ei.category = mono_string_to_utf8_checked (category, &error);
 +              mono_error_cleanup (&error);
 +      }
 +      ei.message = NULL;
 +      if (message) {
 +              ei.message = mono_string_to_utf8_checked (message, &error);
 +              mono_error_cleanup  (&error);
 +      }
  
        process_event (EVENT_KIND_USER_LOG, &ei, 0, NULL, events, suspend_policy);
  
@@@ -6821,7 -6900,7 +6919,7 @@@ invoke_method (void
        /*
         * Take the loader lock to avoid race conditions with CMD_VM_ABORT_INVOKE:
         *
 -       * It is possible that ves_icall_System_Threading_Thread_Abort () was called
 +       * It is possible that mono_thread_internal_abort () was called
         * after the mono_runtime_invoke_checked() already returned, but it doesn't matter
         * because we reset the abort here.
         */
@@@ -7133,7 -7212,7 +7231,7 @@@ vm_commands (int command, int id, guint
  
                tls->abort_requested = TRUE;
  
 -              ves_icall_System_Threading_Thread_Abort (THREAD_TO_INTERNAL (thread), NULL);
 +              mono_thread_internal_abort (THREAD_TO_INTERNAL (thread));
                mono_loader_unlock ();
                break;
        }
@@@ -7803,7 -7882,7 +7901,7 @@@ field_commands (int command, guint8 *p
  static void
  buffer_add_cattr_arg (Buffer *buf, MonoType *t, MonoDomain *domain, MonoObject *val)
  {
 -      if (val && val->vtable->klass == mono_defaults.monotype_class) {
 +      if (val && val->vtable->klass == mono_defaults.runtimetype_class) {
                /* Special case these so the client doesn't have to handle Type objects */
                
                buffer_add_byte (buf, VALUE_TYPE_ID_TYPE);
@@@ -8685,15 -8764,13 +8783,15 @@@ method_commands_internal (int command, 
                // FIXME: Generics
                switch (mono_metadata_token_code (token)) {
                case MONO_TOKEN_STRING: {
 +                      MonoError error;
                        MonoString *s;
                        char *s2;
  
 -                      s = mono_ldstr (domain, method->klass->image, mono_metadata_token_index (token));
 -                      g_assert (s);
 +                      s = mono_ldstr_checked (domain, method->klass->image, mono_metadata_token_index (token), &error);
 +                      mono_error_assert_ok (&error); /* FIXME don't swallow the error */
  
 -                      s2 = mono_string_to_utf8 (s);
 +                      s2 = mono_string_to_utf8_checked (s, &error);
 +                      mono_error_assert_ok (&error);
  
                        buffer_add_byte (buf, TOKEN_TYPE_STRING);
                        buffer_add_string (buf, s2);
                        break;
                }
                default: {
 +                      MonoError error;
                        gpointer val;
                        MonoClass *handle_class;
  
                                        break;
                                }
                        } else {
 -                              MonoError error;
                                val = mono_ldtoken_checked (method->klass->image, token, &handle_class, NULL, &error);
                                if (!val)
                                        g_error ("Could not load token due to %s", mono_error_get_message (&error));
                        } else if (handle_class == mono_defaults.string_class) {
                                char *s;
  
 -                              s = mono_string_to_utf8 ((MonoString *)val);
 +                              s = mono_string_to_utf8_checked ((MonoString *)val, &error);
 +                              mono_error_assert_ok (&error);
                                buffer_add_byte (buf, TOKEN_TYPE_STRING);
                                buffer_add_string (buf, s);
                                g_free (s);
@@@ -9252,9 -9328,7 +9350,9 @@@ string_commands (int command, guint8 *p
                        buffer_add_int (buf, mono_string_length (str) * 2);
                        buffer_add_data (buf, (guint8*)mono_string_chars (str), mono_string_length (str) * 2);
                } else {
 -                      s = mono_string_to_utf8 (str);
 +                      MonoError error;
 +                      s = mono_string_to_utf8_checked (str, &error);
 +                      mono_error_assert_ok (&error);
                        buffer_add_string (buf, s);
                        g_free (s);
                }
@@@ -9719,17 -9793,18 +9817,17 @@@ debugger_thread (void *arg
        ErrorCode err;
        gboolean no_reply;
        gboolean attach_failed = FALSE;
 -      gpointer attach_cookie, attach_dummy;
  
        DEBUG_PRINTF (1, "[dbg] Agent thread started, pid=%p\n", (gpointer) (gsize) mono_native_thread_id_get ());
  
        debugger_thread_id = mono_native_thread_id_get ();
  
 -      attach_cookie = mono_jit_thread_attach (mono_get_root_domain (), &attach_dummy);
 -      MonoInternalThread *thread = mono_thread_internal_current ();
 -      mono_thread_set_name_internal (thread, mono_string_new (mono_get_root_domain (), "Debugger agent"), TRUE, &error);
 +      MonoThread *thread = mono_thread_attach (mono_get_root_domain ());
 +      mono_thread_set_name_internal (thread->internal_thread, mono_string_new (mono_get_root_domain (), "Debugger agent"), TRUE, &error);
        mono_error_assert_ok (&error);
  
 -      thread->flags |= MONO_THREAD_FLAG_DONT_MANAGE;
 +      thread->internal_thread->state |= ThreadState_Background;
 +      thread->internal_thread->flags |= MONO_THREAD_FLAG_DONT_MANAGE;
  
        mono_set_is_debugger_attached (TRUE);
        
                start_debugger_thread ();
        }
  
 -      mono_jit_thread_detach (attach_cookie, &attach_dummy);
 -
        return 0;
  }