[RFA] Refactor breakpoint_re_set_one

[RFA] Refactor breakpoint_re_set_one

[Thiago Jung Bauermann]

Hi,

For the PowerPC BookE ranged breakpoints patch I had to break
breakpoint_re_set_one into two functions. I thought it would be clearer
and easier to review if I put that refactoring in a separate patch. This
patch doesn't change any functionality, just moves code around.

It applies on top of:

http://sourceware.org/ml/gdb-patches/2011-03/msg00978.html

I could also make this patch not depend on the above patch (which is
just a cleanup after all, I don't strictly need that).

There are no regressions in i386-linux, ppc-linux and ppc64-linux. Ok?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


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.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 548b775..df8bb99 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10484,6 +10484,113 @@ update_breakpoint_locations (struct breakpoint *b,
   update_global_location_list (1);
 }
 
+/* Find the SaL locations corresponding to the given addr_string.  */
+
+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 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]);
+    }
+
+  *found = !not_found;
+
+  return sals;
+}
+
+/* Reset a hardware or software breakpoint.  */
+
+static void
+reset_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 +10600,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 +10623,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);
+      reset_breakpoint (b);
       break;
 
     case bp_watchpoint:
@@ -10679,7 +10703,6 @@ breakpoint_re_set_one (void *bint)
       break;
     }
 
-  do_cleanups (cleanups);
   return 0;
 }
 

Re: [RFA] Refactor breakpoint_re_set_one

[Joel Brobecker]

> 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?).

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...

> +  /* 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.  */

> +/* 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.

-- 
Joel

Re: [RFA] Refactor breakpoint_re_set_one

[Pedro Alves]

On Monday 28 March 2011 19:46:47, Joel Brobecker wrote:
> > +/* 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...

Wonder no more.  :-)

From <http://www.gnu.org/prep/standards/standards.html#Comments>,
5.2 Commenting Your Work:

 The comment on a function is much clearer if you use the argument names to
 speak about the argument values. The variable name itself should be lower case,
 but write it in upper case when you are speaking about the value rather
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 than the variable itself. Thus, "the inode number NODE_NUM" rather than "an inode".
 ^^^^^^^^^^^^^^^^^^^^^^^^

Thus, this is not restricted, or really specific of function arguments.

-- 
Pedro Alves

Re: [RFA] Refactor breakpoint_re_set_one

[Thiago Jung Bauermann]

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;
 }
 

Re: [RFA] Refactor breakpoint_re_set_one

[Joel Brobecker]

> Fixed.

BTW: I know the error was there before...

> 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.

This looks very good to me!

Thanks :)
-- 
Joel

Re: [RFA] Refactor breakpoint_re_set_one

[Ulrich Weigand]

Joel Brobecker wrote:
> > 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.
> 
> This looks very good to me!

FWIW this looks good to me as well ...

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

[commit] Fix uninitialized warning (Re: [RFA] Refactor breakpoint_re_set_one)

[Ulrich Weigand]

Thiago Jung Bauermann wrote:

> 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.


+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;

This caused a compiler warning for me, aborting the build:

/home/uweigand/fsf/gdb-head/gdb/breakpoint.c: In function 'addr_string_to_sals':
/home/uweigand/fsf/gdb-head/gdb/breakpoint.c:10587: warning: 'sals.nelts' may be used uninitialized in this function
/home/uweigand/fsf/gdb-head/gdb/breakpoint.c:10587: warning: 'sals.sals' may be used uninitialized in this function

At the place where you removed the variable definition, we used to have
a dummy initializer, presumably to avoid just such warnings:

@@ -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};

The patch below adds the initializer back, fixing the build for me.

Tested on amd64-linux, committed to mainline.

Bye,
Ulrich


ChangeLog:

	* breakpoint.c (addr_string_to_sals): Avoid uninitialized
	variable compiler warning.

Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.558
diff -u -p -r1.558 breakpoint.c
--- gdb/breakpoint.c	30 Mar 2011 20:59:08 -0000	1.558
+++ gdb/breakpoint.c	31 Mar 2011 10:28:52 -0000
@@ -10584,7 +10584,7 @@ addr_string_to_sals (struct breakpoint *
 {
   char *s;
   int marker_spec, not_found;
-  struct symtabs_and_lines sals;
+  struct symtabs_and_lines sals = {0};
   struct gdb_exception e;
 
   s = addr_string;


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Refactor breakpoint_re_set_one

[Thiago Jung Bauermann]

On Tue, 2011-03-29 at 14:21 +0200, Ulrich Weigand wrote:
> Joel Brobecker wrote:
> > > 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.
> > 
> > This looks very good to me!
> 
> FWIW this looks good to me as well ...

Great, thanks!

I committed the patch yesterday, but unexpectedly had to leave in a
hurry and wasn't able to mention the commit. Sorry about that.

This is the version I committed, which doesn't depend on the cleanup
patch I mentioned earlier. That patch remains valid, though.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2011-03-30  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.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c300df9..f07ee10 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -10576,6 +10576,123 @@ update_breakpoint_locations (struct breakpoint *b,
   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);
+    }
+  if (e.reason < 0)
+    {
+      int not_found_and_ok = 0;
+      /* 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 (not_found 
+	  && (b->condition_not_parsed 
+	      || (b->loc && b->loc->shlib_disabled)
+	      || b->enable_state == bp_disabled))
+	not_found_and_ok = 1;
+
+      if (!not_found_and_ok)
+	{
+	  /* 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.  */
@@ -10585,14 +10702,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)
     {
@@ -10617,89 +10726,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);
-	}
-      if (e.reason < 0)
-	{
-	  int not_found_and_ok = 0;
-	  /* 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 (not_found 
-	      && (b->condition_not_parsed 
-		  || (b->loc && b->loc->shlib_disabled)
-		  || b->enable_state == bp_disabled))
-	    not_found_and_ok = 1;
-
-	  if (!not_found_and_ok)
-	    {
-	      /* 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:
@@ -10780,7 +10807,6 @@ breakpoint_re_set_one (void *bint)
       break;
     }
 
-  do_cleanups (cleanups);
   return 0;
 }
 

Re: [commit] Fix uninitialized warning (Re: [RFA] Refactor breakpoint_re_set_one)

[Thiago Jung Bauermann]

On Thu, 2011-03-31 at 13:23 +0200, Ulrich Weigand wrote:
> Thiago Jung Bauermann wrote:
> 
> > 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.
> 
> 
> +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;
> 
> This caused a compiler warning for me, aborting the build:
> 
> /home/uweigand/fsf/gdb-head/gdb/breakpoint.c: In function 'addr_string_to_sals':
> /home/uweigand/fsf/gdb-head/gdb/breakpoint.c:10587: warning: 'sals.nelts' may be used uninitialized in this function
> /home/uweigand/fsf/gdb-head/gdb/breakpoint.c:10587: warning: 'sals.sals' may be used uninitialized in this function
> 
> At the place where you removed the variable definition, we used to have
> a dummy initializer, presumably to avoid just such warnings:
> 
> @@ -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};
> 
> The patch below adds the initializer back, fixing the build for me.
> 
> Tested on amd64-linux, committed to mainline.

I just compiled with -O3 and I was able to reproduce the problem here.
I've been compiling without any optimization. Sorry for the trouble, and
thanks for fixing it.

BTW, I'm also seeing a warning in macroexp.c when compiling with -O3.
I'll commit a patch to fix that shortly if someone doesn't beat me to
it.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center