Re: [RFA] Refactor breakpoint_re_set_one

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

Hi Joel,

Thanks for the review!

On Mon, 2011-03-28 at 11:46 -0700, Joel Brobecker wrote:
> > 2011-03-27  Thiago Jung Bauermann  <bauerman@br.ibm.com>
> > 
> > 	* breakpoint.c (breakpoint_re_set_one): Factor out breakpoint-resetting
> > 	code from here ...
> > 	(reset_breakpoint): ... to here ...
> > 	(addr_string_to_sals): ... and here.
> 
> I didn't really verify line by line that you just extracted out
> the code without actually changing something, but I think we can
> trust your copy/paste tool :-).

:-)

> No real issue on my end of things (just a few little nits).
> But I'm no longer a specialist of this area, so I'd wait a little
> to see if anyone else might have some comments (say, until the
> end of the week?).

Sure, I'll wait.

> My comments below:
> 
> > +/* Find the SaL locations corresponding to the given addr_string.  */
> 
> By convention `addr_string' should be capitalized. It's one of these
> things I really wonder why we do it, but anyways...

Right. I forgot about that.

> > +  /* For pending breakpoints, it's expected that parsing will
> > +     fail until the right shared library is loaded.  User has
> > +     already told to create pending breakpoints and don't need
>                                                       ^^^^ doesn't
> > +     extra messages.  If breakpoint is in bp_shlib_disabled
> > +     state, then user already saw the message about that
> > +     breakpoint being disabled, and don't want to see more
>                                        ^^^^^ doesn't
> > +     errors.  */

Fixed.

> > +/* Reset a hardware or software breakpoint.  */
> > +
> > +static void
> > +reset_breakpoint (struct breakpoint *b)
> 
> My only comment is that `reset' is a little ambiguous, but maybe
> that's just me. I often think of "reset" as set back to original
> value.  I like the use of `re_set' in breakpoint_re_set_one, so
> what do you think of doing the same here? Also, a more descriptive
> description of the function would be useful, IMO.

You have a point. I renamed it to re_set_breakpoint, and expanded the
description a bit. What do you think?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2011-03-28  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	* breakpoint.c (breakpoint_re_set_one): Factor out breakpoint-resetting
	code from here ...
	(re_set_breakpoint): ... to here ...
	(addr_string_to_sals): ... and here.

Index: gdb.git/gdb/breakpoint.c
===================================================================
--- gdb.git.orig/gdb/breakpoint.c	2011-03-28 18:13:43.000000000 -0300
+++ gdb.git/gdb/breakpoint.c	2011-03-28 18:14:28.000000000 -0300
@@ -10484,6 +10484,116 @@ update_breakpoint_locations (struct brea
   update_global_location_list (1);
 }
 
+/* Find the SaL locations corresponding to the given ADDR_STRING.
+   On return, FOUND will be 1 if any SaL was found, zero otherwise.  */
+
+static struct symtabs_and_lines
+addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
+{
+  char *s;
+  int marker_spec, not_found;
+  struct symtabs_and_lines sals;
+  struct gdb_exception e;
+
+  s = addr_string;
+  marker_spec = b->type == bp_static_tracepoint && is_marker_spec (s);
+
+  TRY_CATCH (e, RETURN_MASK_ERROR)
+    {
+      if (marker_spec)
+	{
+	  sals = decode_static_tracepoint_spec (&s);
+	  if (sals.nelts > b->static_trace_marker_id_idx)
+	    {
+	      sals.sals[0] = sals.sals[b->static_trace_marker_id_idx];
+	      sals.nelts = 1;
+	    }
+	  else
+	    error (_("marker %s not found"), b->static_trace_marker_id);
+	}
+      else
+	sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0,
+			      NULL, &not_found);
+    }
+
+  /* For pending breakpoints, it's expected that parsing will
+     fail until the right shared library is loaded.  User has
+     already told to create pending breakpoints and don't need
+     extra messages.  If breakpoint is in bp_shlib_disabled
+     state, then user already saw the message about that
+     breakpoint being disabled, and doesn't want to see more
+     errors.  */
+  if (e.reason < 0 && (!not_found || (!b->condition_not_parsed
+				      && !(b->loc && b->loc->shlib_disabled)
+				      && b->enable_state != bp_disabled)))
+    {
+      /* We surely don't want to warn about the same breakpoint
+	 10 times.  One solution, implemented here, is disable
+	 the breakpoint on error.  Another solution would be to
+	 have separate 'warning emitted' flag.  Since this
+	 happens only when a binary has changed, I don't know
+	 which approach is better.  */
+      b->enable_state = bp_disabled;
+      throw_exception (e);
+    }
+
+  if (!not_found)
+    {
+      gdb_assert (sals.nelts == 1);
+
+      resolve_sal_pc (&sals.sals[0]);
+      if (b->condition_not_parsed && s && s[0])
+	{
+	  char *cond_string = 0;
+	  int thread = -1;
+	  int task = 0;
+
+	  find_condition_and_thread (s, sals.sals[0].pc,
+				     &cond_string, &thread, &task);
+	  if (cond_string)
+	    b->cond_string = cond_string;
+	  b->thread = thread;
+	  b->task = task;
+	  b->condition_not_parsed = 0;
+	}
+
+      if (b->type == bp_static_tracepoint && !marker_spec)
+	sals.sals[0] = update_static_tracepoint (b, sals.sals[0]);
+    }
+
+  *found = !not_found;
+
+  return sals;
+}
+
+/* Reevaluate a hardware or software breakpoint and recreate its locations.
+   This is necessary after symbols are read (e.g., an executable or DSO
+   was loaded, or the inferior just started).  */
+
+static void
+re_set_breakpoint (struct breakpoint *b)
+{
+  int found;
+  struct symtabs_and_lines sals;
+  struct symtabs_and_lines expanded = {0};
+  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
+
+  input_radix = b->input_radix;
+  save_current_space_and_thread ();
+  switch_to_program_space_and_thread (b->pspace);
+  set_language (b->language);
+
+  sals = addr_string_to_sals (b, b->addr_string, &found);
+  if (found)
+    {
+      make_cleanup (xfree, sals.sals);
+      expanded = expand_line_sal_maybe (sals.sals[0]);
+    }
+
+  update_breakpoint_locations (b, expanded);
+  do_cleanups (cleanups);
+}
+
 /* Reset a breakpoint given it's struct breakpoint * BINT.
    The value we return ends up being the return value from catch_errors.
    Unused in this case.  */
@@ -10493,14 +10603,6 @@ breakpoint_re_set_one (void *bint)
 {
   /* Get past catch_errs.  */
   struct breakpoint *b = (struct breakpoint *) bint;
-  int not_found = 0;
-  int *not_found_ptr = &not_found;
-  struct symtabs_and_lines sals = {0};
-  struct symtabs_and_lines expanded = {0};
-  char *s;
-  struct gdb_exception e;
-  struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
-  int marker_spec = 0;
 
   switch (b->type)
     {
@@ -10524,82 +10626,7 @@ breakpoint_re_set_one (void *bint)
 	  return 0;
 	}
 
-      input_radix = b->input_radix;
-      s = b->addr_string;
-
-      save_current_space_and_thread ();
-      switch_to_program_space_and_thread (b->pspace);
-
-      marker_spec = b->type == bp_static_tracepoint && is_marker_spec (s);
-
-      set_language (b->language);
-      TRY_CATCH (e, RETURN_MASK_ERROR)
-	{
-	  if (marker_spec)
-	    {
-	      sals = decode_static_tracepoint_spec (&s);
-	      if (sals.nelts > b->static_trace_marker_id_idx)
-		{
-		  sals.sals[0] = sals.sals[b->static_trace_marker_id_idx];
-		  sals.nelts = 1;
-		}
-	      else
-		error (_("marker %s not found"), b->static_trace_marker_id);
-	    }
-	  else
-	    sals = decode_line_1 (&s, 1, (struct symtab *) NULL, 0,
-				  NULL, not_found_ptr);
-	}
-
-      /* For pending breakpoints, it's expected that parsing will
-	 fail until the right shared library is loaded.  User has
-	 already told to create pending breakpoints and don't need
-	 extra messages.  If breakpoint is in bp_shlib_disabled
-	 state, then user already saw the message about that
-	 breakpoint being disabled, and don't want to see more
-	 errors.  */
-      if (e.reason < 0 && (!not_found || (!b->condition_not_parsed
-					   && !(b->loc && b->loc->shlib_disabled)
-					   && b->enable_state != bp_disabled)))
-	{
-	  /* We surely don't want to warn about the same breakpoint
-	     10 times.  One solution, implemented here, is disable
-	     the breakpoint on error.  Another solution would be to
-	     have separate 'warning emitted' flag.  Since this
-	     happens only when a binary has changed, I don't know
-	     which approach is better.  */
-	  b->enable_state = bp_disabled;
-	  throw_exception (e);
-	}
-
-      if (!not_found)
-	{
-	  gdb_assert (sals.nelts == 1);
-
-	  resolve_sal_pc (&sals.sals[0]);
-	  if (b->condition_not_parsed && s && s[0])
-	    {
-	      char *cond_string = 0;
-	      int thread = -1;
-	      int task = 0;
-
-	      find_condition_and_thread (s, sals.sals[0].pc,
-					 &cond_string, &thread, &task);
-	      if (cond_string)
-		b->cond_string = cond_string;
-	      b->thread = thread;
-	      b->task = task;
-	      b->condition_not_parsed = 0;
-	    }
-
-	  if (b->type == bp_static_tracepoint && !marker_spec)
-	    sals.sals[0] = update_static_tracepoint (b, sals.sals[0]);
-
-	  expanded = expand_line_sal_maybe (sals.sals[0]);
-	}
-
-      make_cleanup (xfree, sals.sals);
-      update_breakpoint_locations (b, expanded);
+      re_set_breakpoint (b);
       break;
 
     case bp_watchpoint:
@@ -10679,7 +10706,6 @@ breakpoint_re_set_one (void *bint)
       break;
     }
 
-  do_cleanups (cleanups);
   return 0;
 }