Simplify MI breakpoint setting

Simplify MI breakpoint setting

[Vladimir Prus]

[-- Attachment #1: Type: Text/Plain, Size: 331 bytes --]


I've noticed that mi_cmd_break_insert does some fairly
contrived logic which amounts to passing either 0 or 1
as the value of 'hardware' parameter to set_breakpoint.
Further, set_breakpoint appears to be trivial and useless
wrapper over break_command_really. This patch cleans that
up. Are breakpoint.{c,h} changes OK?

- Volodya

[-- Attachment #2: mi-break-simplify.diff --]
[-- Type: text/x-patch, Size: 4844 bytes --]

commit 17e617aa040e44f97d89afe6b93d11e9a78beab7
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Sat Aug 1 11:09:08 2009 +0400

    Simplify MI breakpoint setting.
    
    	* breakpoint.c (break_command_really): Make nonstatic.
    	(set_breakpoint): Remove.
    	* breakpoint.h: Adjust to above changes.
    	* mi/mi-cmd-break.c (mi_cmd_break_insert): Simplify.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4b9b44e..4bfbb57 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5848,7 +5848,7 @@ find_condition_and_thread (char *tok, CORE_ADDR pc,
    and thread specified by the COND_STRING and THREAD
    parameters.  */
 
-static void
+void
 break_command_really (struct gdbarch *gdbarch,
 		      char *arg, char *cond_string, int thread,
 		      int parse_condition_and_thread,
@@ -6056,22 +6056,6 @@ break_command_1 (char *arg, int flag, int from_tty)
 }
 
 
-void
-set_breakpoint (struct gdbarch *gdbarch,
-		char *address, char *condition,
-		int hardwareflag, int tempflag,
-		int thread, int ignore_count,
-		int pending, int enabled)
-{
-  break_command_really (gdbarch,
-			address, condition, thread,
-			0 /* condition and thread are valid.  */,
-			tempflag, hardwareflag, 0 /* traceflag */,
-			ignore_count,
-			pending 
-			? AUTO_BOOLEAN_TRUE : AUTO_BOOLEAN_FALSE,
-			NULL, 0, enabled);
-}
 
 /* Adjust SAL to the first instruction past the function prologue.
    The end of the prologue is determined using the line table from
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 54e2ea0..65e21a5 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -733,12 +733,14 @@ extern void awatch_command_wrapper (char *, int);
 extern void rwatch_command_wrapper (char *, int);
 extern void tbreak_command (char *, int);
 
-extern void set_breakpoint (struct gdbarch *gdbarch,
-			    char *address, char *condition,
-			    int hardwareflag, int tempflag,
-			    int thread, int ignore_count,
-			    int pending,
-			    int enabled);
+extern void break_command_really (struct gdbarch* gdbarch, char *arg, char *cond_string, int thread,
+				  int parse_condition_and_thread,
+				  int tempflag, int hardwareflag, int traceflag,
+				  int ignore_count,
+				  enum auto_boolean pending_break_support,
+				  struct breakpoint_ops *ops,
+				  int from_tty,
+				  int enabled);
 
 extern void insert_breakpoints (void);
 
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 9ab8f2d..54d7fe7 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -67,18 +67,20 @@ mi_cmd_break_insert (char *command, char **argv, int argc)
 {
   char *address = NULL;
   enum bp_type type = REG_BP;
+  int hardware = 0;
   int temp_p = 0;
   int thread = -1;
   int ignore_count = 0;
   char *condition = NULL;
   int pending = 0;
   int enabled = 1;
+  struct cleanup *back_to;
 
   struct gdb_exception e;
   struct gdb_events *old_hooks;
   enum opt
     {
-      HARDWARE_OPT, TEMP_OPT /*, REGEXP_OPT */ , CONDITION_OPT,
+      HARDWARE_OPT, TEMP_OPT, CONDITION_OPT,
       IGNORE_COUNT_OPT, THREAD_OPT, PENDING_OPT, DISABLE_OPT
     };
   static struct mi_opt opts[] =
@@ -108,13 +110,8 @@ mi_cmd_break_insert (char *command, char **argv, int argc)
 	  temp_p = 1;
 	  break;
 	case HARDWARE_OPT:
-	  type = HW_BP;
+	  hardware = 1;
 	  break;
-#if 0
-	case REGEXP_OPT:
-	  type = REGEXP_BP;
-	  break;
-#endif
 	case CONDITION_OPT:
 	  condition = optarg;
 	  break;
@@ -147,40 +144,16 @@ mi_cmd_break_insert (char *command, char **argv, int argc)
       mi_breakpoint_observers_installed = 1;
     }
 
+  back_to = make_cleanup_restore_integer (&mi_can_breakpoint_notify);
   mi_can_breakpoint_notify = 1;
-  /* Make sure we restore hooks even if exception is thrown.  */
-  TRY_CATCH (e, RETURN_MASK_ALL)
-    {
-      switch (type)
-	{
-	case REG_BP:
-	  set_breakpoint (get_current_arch (), address, condition,
-			  0 /*hardwareflag */ , temp_p,
-			  thread, ignore_count,
-			  pending, enabled);
-	  break;
-	case HW_BP:
-	  set_breakpoint (get_current_arch (), address, condition,
-			  1 /*hardwareflag */ , temp_p,
-			  thread, ignore_count,
-			  pending, enabled);
-	  break;
-#if 0
-	case REGEXP_BP:
-	  if (temp_p)
-	    error (_("mi_cmd_break_insert: Unsupported tempoary regexp breakpoint"));
-	  else
-	    rbreak_command_wrapper (address, FROM_TTY);
-	  break;
-#endif
-	default:
-	  internal_error (__FILE__, __LINE__,
-			  _("mi_cmd_break_insert: Bad switch."));
-	}
-    }
-  mi_can_breakpoint_notify = 0;
-  if (e.reason < 0)
-    throw_exception (e);
+  break_command_really (get_current_arch (), address, condition, thread,
+			0 /* condition and thread are valid.  */,
+			temp_p, hardware, 0 /* traceflag */,
+			ignore_count,
+			pending ? AUTO_BOOLEAN_TRUE : AUTO_BOOLEAN_FALSE,
+			NULL, 0, enabled);
+  do_cleanups (back_to);
+	  
 }
 
 enum wp_type

Re: Simplify MI breakpoint setting

[Tom Tromey]

>>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:

Volodya> Further, set_breakpoint appears to be trivial and useless
Volodya> wrapper over break_command_really. This patch cleans that
Volodya> up. Are breakpoint.{c,h} changes OK?

I think the point of this is not to expose all the parameters of
break_command_really outside of breakpoint.c.

So, unless there is a problem with the current approach, I would prefer
not to change it.

In the alternative, if somebody does want to approve this, I would like
to recommend renaming break_command_really, as I don't think that is a
very good name for a public function.

Tom

Re: Simplify MI breakpoint setting

[Vladimir Prus]

On Friday 07 August 2009 Tom Tromey wrote:

> >>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:
> 
> Volodya> Further, set_breakpoint appears to be trivial and useless
> Volodya> wrapper over break_command_really. This patch cleans that
> Volodya> up. Are breakpoint.{c,h} changes OK?
> 
> I think the point of this is not to expose all the parameters of
> break_command_really outside of breakpoint.c.

And, progressing recursively, what is the point of not exposing all the
parameters of break_command_really? set_breakpoint is only ever used
in MI code, and MI code does not mind those 3 extra parameters.

- Volodya

Re: Simplify MI breakpoint setting

[Tom Tromey]

>>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:

Volodya> And, progressing recursively, what is the point of not exposing
Volodya> all the parameters of break_command_really?

I don't actually know.  But if I had to guess, I would say it is because
providing wrappers ensures you can't pass in some forms of nonsense.

If you really want to do it, and nobody objects, then I guess I don't
care all that much.  This whole API seems a bit nuts, any time you have
13 arguments you should just assume you've done something wrong already.

I do care about not exporting a function named "break_command_really"
though.

Tom

Re: Simplify MI breakpoint setting

[Vladimir Prus]

On Tuesday 01 September 2009 Tom Tromey wrote:

> >>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:
> 
> Volodya> And, progressing recursively, what is the point of not exposing
> Volodya> all the parameters of break_command_really?
> 
> I don't actually know.  But if I had to guess, I would say it is because
> providing wrappers ensures you can't pass in some forms of nonsense.
> 
> If you really want to do it, and nobody objects, then I guess I don't
> care all that much.  This whole API seems a bit nuts, any time you have
> 13 arguments you should just assume you've done something wrong already.
> 
> I do care about not exporting a function named "break_command_really"
> though.

Ok. I imagine that break_command_really can be renamed to set_breakpoint :-)

- Volodya

Re: Simplify MI breakpoint setting

[Matt Rice]

On Mon, Aug 31, 2009 at 10:16 PM, Vladimir
Prus<vladimir@codesourcery.com> wrote:
> On Tuesday 01 September 2009 Tom Tromey wrote:
>
>> >>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:
>>
>> Volodya> And, progressing recursively, what is the point of not exposing
>> Volodya> all the parameters of break_command_really?
>>
>> I don't actually know.  But if I had to guess, I would say it is because
>> providing wrappers ensures you can't pass in some forms of nonsense.
>>
>> If you really want to do it, and nobody objects, then I guess I don't
>> care all that much.  This whole API seems a bit nuts, any time you have
>> 13 arguments you should just assume you've done something wrong already.
>>
>> I do care about not exporting a function named "break_command_really"
>> though.
>
> Ok. I imagine that break_command_really can be renamed to set_breakpoint :-)
>

here are my thoughts,

I don't like how break_command_really looks for the symbol first, then
if it fails to find one and
pending breakpoints are enabled it will set a pending breakpoint, I
think it might sometimes make sense
to set a pending breakpoint even though there may be a match.

though I do think that the break_command_really behaviour is good for
the  'break command'
if we were to introduce a 'pbreak', or pending breakpoint command, I
could imagine pbreak_command_really or something calling something
named set_breakpoint, but that is not just renaming
break_command_really.

similarly, there is currently no way to do 'set multiple-symbols
pending' or all+pending,
and making 'set multiple-symbols ask' respond like this:

[0] cancel
[1] all
[2] pending

(note that you could select  (1 2) get all, and a pending
or 2 and just set a pending breakpoint.
(now you'll have to ignore all the rest of the stuff you can think of
required to make this behaviour anything but annoying)

this stuff as-is seems difficult since all the multiple-symbols stuff
predates pending breakpoints.
and there is no real API for setting them outside of a non-match
afaict, please correct me if i'm wrong.

so i think that makes me pro exposing some subset of
break_command_really, and not opposed to also exposing the stuff I
wouldn't need exposed.

Re: Simplify MI breakpoint setting

[Vladimir Prus]

On Tuesday 08 September 2009 Matt Rice wrote:

> On Mon, Aug 31, 2009 at 10:16 PM, Vladimir
> Prus<vladimir@codesourcery.com> wrote:
> > On Tuesday 01 September 2009 Tom Tromey wrote:
> >
> >> >>>>> "Volodya" == Vladimir Prus <vladimir@codesourcery.com> writes:
> >>
> >> Volodya> And, progressing recursively, what is the point of not exposing
> >> Volodya> all the parameters of break_command_really?
> >>
> >> I don't actually know.  But if I had to guess, I would say it is because
> >> providing wrappers ensures you can't pass in some forms of nonsense.
> >>
> >> If you really want to do it, and nobody objects, then I guess I don't
> >> care all that much.  This whole API seems a bit nuts, any time you have
> >> 13 arguments you should just assume you've done something wrong already.
> >>
> >> I do care about not exporting a function named "break_command_really"
> >> though.
> >
> > Ok. I imagine that break_command_really can be renamed to set_breakpoint :-)
> >
> 
> here are my thoughts,
> 
> I don't like how break_command_really looks for the symbol first, then
> if it fails to find one and
> pending breakpoints are enabled it will set a pending breakpoint, I
> think it might sometimes make sense
> to set a pending breakpoint even though there may be a match.

I am missing something. Pending breakpoint, by definition, is a breakpoint
that has zero locations. And breakpoint can have zero locations if and only
if we have failed to find any location that corresponds to the string
that user has specified. Therefore, pending breakpoint for a found symbol
or line seems just impossible.

> 
> though I do think that the break_command_really behaviour is good for
> the  'break command'
> if we were to introduce a 'pbreak', or pending breakpoint command, I
> could imagine pbreak_command_really or something calling something
> named set_breakpoint, but that is not just renaming
> break_command_really.
> 
> similarly, there is currently no way to do 'set multiple-symbols
> pending' or all+pending,
> and making 'set multiple-symbols ask' respond like this:
> 
> [0] cancel
> [1] all
> [2] pending
> 
> (note that you could select  (1 2) get all, and a pending
> or 2 and just set a pending breakpoint.
> (now you'll have to ignore all the rest of the stuff you can think of
> required to make this behaviour anything but annoying)

It is true that interaction of the 'set multiple breakpoints for
each overloaded function' functionality with the multiple-location
functionality is not much designed.

- Volodya

Re: Simplify MI breakpoint setting

[Matt Rice]

On Mon, Sep 7, 2009 at 10:20 PM, Vladimir Prus<vladimir@codesourcery.com> wrote:
> On Tuesday 08 September 2009 Matt Rice wrote:

<snip>

>>
>> I don't like how break_command_really looks for the symbol first, then
>> if it fails to find one and
>> pending breakpoints are enabled it will set a pending breakpoint, I
>> think it might sometimes make sense
>> to set a pending breakpoint even though there may be a match.
>
> I am missing something. Pending breakpoint, by definition, is a breakpoint
> that has zero locations. And breakpoint can have zero locations if and only
> if we have failed to find any location that corresponds to the string
> that user has specified. Therefore, pending breakpoint for a found symbol
> or line seems just impossible.
>

I was only speaking about the behaviour of pending breakpoints where
they accumulate a location
when a location matching the user specified string is loaded in the
future, which seems useful to me regardless of it
having a location when using languages where it is possible for the
user specified string to match multiple locations.

But yes it appears that would not be a pending breakpoint by the
definition given by you and manual.

Re: Simplify MI breakpoint setting

[Vladimir Prus]

On Tuesday 08 September 2009 you wrote:

> On Mon, Sep 7, 2009 at 10:20 PM, Vladimir Prus<vladimir@codesourcery.com> wrote:
> > On Tuesday 08 September 2009 Matt Rice wrote:
> 
> <snip>
> 
> >>
> >> I don't like how break_command_really looks for the symbol first, then
> >> if it fails to find one and
> >> pending breakpoints are enabled it will set a pending breakpoint, I
> >> think it might sometimes make sense
> >> to set a pending breakpoint even though there may be a match.
> >
> > I am missing something. Pending breakpoint, by definition, is a breakpoint
> > that has zero locations. And breakpoint can have zero locations if and only
> > if we have failed to find any location that corresponds to the string
> > that user has specified. Therefore, pending breakpoint for a found symbol
> > or line seems just impossible.
> >
> 
> I was only speaking about the behaviour of pending breakpoints where
> they accumulate a location
> when a location matching the user specified string is loaded in the
> future, which seems useful to me regardless of it
> having a location when using languages where it is possible for the
> user specified string to match multiple locations.
> 
> But yes it appears that would not be a pending breakpoint by the
> definition given by you and manual.

In fact, *all* breakpoint will pick new locations at present. We don't
have a mechanism to "lock" a breakpoint to the locations it presently
has. (And nobody has yet requested this)

- Volodya