Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* Re: MI/CLI breakpoint handling code duplication
@ 2008-01-11 22:05 Nick Roberts
  2008-01-15 20:43 ` Vladimir Prus
  0 siblings, 1 reply; 6+ messages in thread
From: Nick Roberts @ 2008-01-11 22:05 UTC (permalink / raw)
  To: Vladimir Prus; +Cc: gdb-patches

[2nd attempt]

(Fri, 14 Dec 2007 10:26:49 +0400)

> > 	* breakpoint.c (break_command_really): New, copied
> > 	from break_command_1. New parameters COND_STRING, THREAD
> > 	PARSE_CONDITITION_AND_THREAD and PENDING_BREAK_SUPPORT.
> > 	The previous FLAG parameter split into TEMPFLAG and
> > 	HARDWAREFLAG.
> > 	When PARSE_CONDITION_AND_THREAD is not set, duplicate
> > 	the passed condition string.
> > 	(struct captured_breakpoint_args): Remove
> > 	(do_captured_breakpoint): Remove.
> > 	(break_command_1): Relay to break_command_really.
> > 	(gdb_breakpoint): Relay to break_command_really.

> Ping? This patch is pure refactoring which is basis for
> the 'pending breakpoint in MI' patch. Given that the patch
> is not supposed to change any behaviour and causes no regression,
> I'd hope it should be easy to review :-)

Previously:

  (gdb) 
  -break-insert sqrt
  &"Function \"sqrt\" not defined.\n"
  ^error,msg="Function \"sqrt\" not defined."
  (gdb) 


After this change:

  (gdb) 
  -break-insert sqrt
  &"Function \"sqrt\" not defined.\n"
  ^error,msg="unknown error"
  (gdb) 


So this is *not* pure refactoring and happens because gdb_breakpoint used
catch_exceptions_with_msg previously while it now uses catch_exception
through break_command_really.  This means now that in mi_cmd_break_insert


      rc = gdb_breakpoint (address, condition,
			   0 /*hardwareflag */ , temp_p,
			   thread, ignore_count,
			   pending,
			   &mi_error_message);

mi_error_message points to NULL.


Furthermore with a sequence like:

(gdb) 
-break-insert main
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x080485bc",func="main",file="/home/nickrob/myprog.c",fullname="/home/nickrob/myprog.c",line="79",times="0"}
(gdb) 
-break-insert sqrt
&"Function \"sqrt\" not defined.\n"
^error,msg="unknown error"
(gdb) 
-exec-run
^running
(gdb) 
*stopped,bkpt={number="-1",type="longjmp resume",disp="keep",enabled="n",addr="0x00000000",at="",times="0"},bkpt={number="-2",type="longjmp resume",disp="keep",enabled="n",addr="0x00000000",at="",times="0"},bkpt={number="-4",type="longjmp resume",disp="keep",enabled="n",addr="0x00000000",at="",times="0"},bkpt={number="-5",type="longjmp resume",disp="keep",enabled="n",addr="0x00000000",at="",times="0"},reason="breakpoint-hit",bkptno="1",thread-id="0",frame={addr="0x080485bc",func="main",args=[{name="argc",value="-1074490904"},{name="argv",value="0xbff49214"}],file="/home/nickrob/myprog.c",fullname="/home/nickrob/myprog.c",line="79"}
(gdb) 


GDB prints extra output because previous content has not been flushed from the
stream.  I think this is because gdb_breakpoint tries to return int (really
enum REASON, -2 or -1) through break_command_really while it is type enum
gdb_rc (0, 1 or 2).

So that when gdb_breakpoint fails (and returns RETURN_ERROR = -1)
mi_cmd_break_insert thinks it has suceeded and returns MI_CMD_DONE
(GDB_RC_FAIL = 0):

  if (rc == GDB_RC_FAIL)
    return MI_CMD_ERROR;
  else
    return MI_CMD_DONE;

I still don't understand the difference between breakpoint handling in MI and
CLI but it must surely have been done differently for a reason.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


"Theories should be as simple as possible, but no simpler."

                                          Albert Einstein


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: MI/CLI breakpoint handling code duplication
@ 2008-01-11 22:03 Nick Roberts
  0 siblings, 0 replies; 6+ messages in thread
From: Nick Roberts @ 2008-01-11 22:03 UTC (permalink / raw)
  To: Vladimir Prus; +Cc: gdb-patches


(Fri, 14 Dec 2007 10:26:49 +0400)

> > 	* breakpoint.c (break_command_really): New, copied
> > 	from break_command_1. New parameters COND_STRING, THREAD
> > 	PARSE_CONDITITION_AND_THREAD and PENDING_BREAK_SUPPORT.
> > 	The previous FLAG parameter split into TEMPFLAG and
> > 	HARDWAREFLAG.
> > 	When PARSE_CONDITION_AND_THREAD is not set, duplicate
> > 	the passed condition string.
> > 	(struct captured_breakpoint_args): Remove
> > 	(do_captured_breakpoint): Remove.
> > 	(break_command_1): Relay to break_command_really.
> > 	(gdb_breakpoint): Relay to break_command_really.

> Ping? This patch is pure refactoring which is basis for
> the 'pending breakpoint in MI' patch. Given that the patch
> is not supposed to change any behaviour and causes no regression,
> I'd hope it should be easy to review :-)

Previously:

  (gdb) 
  -break-insert sqrt
  &"Function \"sqrt\" not defined.\n"
  ^error,msg="Function \"sqrt\" not defined."
  (gdb) 


After this change:

  (gdb) 
  -break-insert sqrt
  &"Function \"sqrt\" not defined.\n"
  ^error,msg="unknown error"
  (gdb) 


So this is *not* pure refactoring and happens because gdb_breakpoint used
catch_exceptions_with_msg previously while it now uses catch_exception
through break_command_really.  This means now that in mi_cmd_break_insert


      rc = gdb_breakpoint (address, condition,
			   0 /*hardwareflag */ , temp_p,
			   thread, ignore_count,
			   pending,
			   &mi_error_message);

mi_error_message points to NULL.


Furthermore with a sequence like:

(gdb) 
-break-insert main
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x080485bc",func="main",file="/home/nickrob/myprog.c",fullname="/home/nickrob/myprog.c",line="79",times="0"}
(gdb) 
-break-insert sqrt
&"Function \"sqrt\" not defined.\n"
^error,msg="unknown error"
(gdb) 
-exec-run
^running
(gdb) 
*stopped,bkpt={number="-1",type="longjmp resume",disp="keep",enabled="n",addr="0x00000000",at="",times="0"},bkpt={number="-2",type="longjmp resume",disp="keep",enabled="n",addr="0x00000000",at="",times="0"},bkpt={number="-4",type="longjmp resume",disp="keep",enabled="n",addr="0x00000000",at="",times="0"},bkpt={number="-5",type="longjmp resume",disp="keep",enabled="n",addr="0x00000000",at="",times="0"},reason="breakpoint-hit",bkptno="1",thread-id="0",frame={addr="0x080485bc",func="main",args=[{name="argc",value="-1074490904"},{name="argv",value="0xbff49214"}],file="/home/nickrob/myprog.c",fullname="/home/nickrob/myprog.c",line="79"}
(gdb) 


GDB prints extra output because previous content has not been flushed from the
stream.  I think this is because gdb_breakpoint tries to return int (really
enum REASON, -2 or -1) through break_command_really while it is type enum
gdb_rc (0, 1 or 2).

So that when gdb_breakpoint fails (and returns RETURN_ERROR = -1)
mi_cmd_break_insert thinks it has suceeded and returns MI_CMD_DONE
(GDB_RC_FAIL = 0):

  if (rc == GDB_RC_FAIL)
    return MI_CMD_ERROR;
  else
    return MI_CMD_DONE;

I still don't understand the difference between breakpoint handling in MI and
CLI but it must surely have been done differently for a reason.

-- 
Nick                                           http://www.inet.net.nz/~nickrob


"Theories should be as simple as possible, but no simpler."

                                          Albert Einstein


^ permalink raw reply	[flat|nested] 6+ messages in thread
* MI/CLI breakpoint handling code duplication
@ 2007-11-08 10:26 Vladimir Prus
  2007-12-14 14:08 ` Vladimir Prus
  0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Prus @ 2007-11-08 10:26 UTC (permalink / raw)
  To: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 814 bytes --]


The code paths handling break insertion in MI and CLI
feature undue code duplication. This patch makes extract
the body of break_command_1 into separate function, makes
it slightly more flexibile, and as result, both MI and CLI
code paths merely forward to the new function.
No regressions on x86, OK?

- Volodya

	* breakpoint.c (break_command_really): New, copied
	from break_command_1. New parameters COND_STRING, THREAD
	PARSE_CONDITITION_AND_THREAD and PENDING_BREAK_SUPPORT.
	The previous FLAG parameter split into TEMPFLAG and
	HARDWAREFLAG.
	When PARSE_CONDITION_AND_THREAD is not set, duplicate
	the passed condition string.
	(struct captured_breakpoint_args): Remove
	(do_captured_breakpoint): Remove.
	(break_command_1): Relay to break_command_really.
	(gdb_breakpoint): Relay to break_command_really.

[-- Attachment #2: pending_mi_2_code_duplication.diff --]
[-- Type: text/x-diff, Size: 8422 bytes --]

--- gdb/breakpoint.c	(/patches/pending_mi_1_cleanup)	(revision 43)
+++ gdb/breakpoint.c	(/patches/pending_mi_2_code_duplication)	(revision 43)
@@ -5435,18 +5435,26 @@ find_condition_and_thread (char *tok, CO
     }
 }
 
-/* Set a breakpoint according to ARG (function, linenum or *address)
-   flag: first bit  : 0 non-temporary, 1 temporary.
-   second bit : 0 normal breakpoint, 1 hardware breakpoint.  */
+/* Set a breakpoint.  This function is shared between
+   CLI and MI functions for setting a breakpoint.
+   This function has two major modes of operations,
+   selected by the PARSE_CONDITION_AND_THREAD parameter.
+   If non-zero, the function will parse arg, extracting
+   breakpoint location, address and thread. Otherwise,
+   ARG is just the location of breakpoint, with condition
+   and thread specified by the COND_STRING and THREAD
+   parameters.  */
 
 static int
-break_command_1 (char *arg, int flag, int from_tty)
+break_command_really (char *arg, char *cond_string, int thread,
+                      int parse_condition_and_thread,
+                      int tempflag, int hardwareflag, 
+                      enum auto_boolean pending_break_support,
+                      int from_tty)
 {
   struct gdb_exception e;
-  int tempflag, hardwareflag;
   struct symtabs_and_lines sals;
   struct symtab_and_line pending_sal;
-  char *cond_string = NULL;
   char *copy_arg;
   char *err_msg;
   char *addr_start = arg;
@@ -5456,13 +5464,9 @@ break_command_1 (char *arg, int flag, in
   struct captured_parse_breakpoint_args parse_args;
   int i;
   int pending = 0;
-  int thread = -1;
   int ignore_count = 0;
   int not_found = 0;
 
-  hardwareflag = flag & BP_HARDWAREFLAG;
-  tempflag = flag & BP_TEMPFLAG;
-
   sals.sals = NULL;
   sals.nelts = 0;
   addr_string = NULL;
@@ -5557,13 +5561,27 @@ break_command_1 (char *arg, int flag, in
      breakpoint. */
   if (!pending)
     {
-      /* Here we only parse 'arg' to separate condition
-	 from thread number, so parsing in context of first
-	 sal is OK.  When setting the breakpoint we'll 
-	 re-parse it in context of each sal.  */
-      find_condition_and_thread (arg, sals.sals[0].pc, &cond_string, &thread);
-      if (cond_string)
-	make_cleanup (xfree, cond_string);
+        if (parse_condition_and_thread)
+        {
+            /* Here we only parse 'arg' to separate condition
+               from thread number, so parsing in context of first
+               sal is OK.  When setting the breakpoint we'll 
+               re-parse it in context of each sal.  */
+            cond_string = NULL;
+            thread = -1;
+            find_condition_and_thread (arg, sals.sals[0].pc, &cond_string, &thread);
+            if (cond_string)
+                make_cleanup (xfree, cond_string);
+        }
+        else
+        {
+            /* Create a private copy of condition string.  */
+            if (cond_string)
+            {
+                cond_string = xstrdup (cond_string);
+                make_cleanup (xfree, cond_string);
+            }
+        }
       create_breakpoints (sals, addr_string, cond_string,
 			  hardwareflag ? bp_hardware_breakpoint 
 			  : bp_breakpoint,
@@ -5582,13 +5600,14 @@ break_command_1 (char *arg, int flag, in
 					       : bp_breakpoint);
       set_breakpoint_count (breakpoint_count + 1);
       b->number = breakpoint_count;
-      b->thread = thread;
+      b->thread = -1;
       b->addr_string = addr_string[0];
-      b->cond_string = cond_string;
+      b->cond_string = NULL;
       b->ignore_count = ignore_count;
       b->disposition = tempflag ? disp_del : disp_donttouch;
       b->from_tty = from_tty;
-      b->flag = flag;
+      b->flag = (hardwareflag ? BP_HARDWAREFLAG : 0) 
+          | (tempflag ? BP_TEMPFLAG : 0);
       b->condition_not_parsed = 1;
       mention (b);
     }
@@ -5605,119 +5624,37 @@ break_command_1 (char *arg, int flag, in
   return GDB_RC_OK;
 }
 
-/* Set a breakpoint of TYPE/DISPOSITION according to ARG (function,
-   linenum or *address) with COND and IGNORE_COUNT. */
-
-struct captured_breakpoint_args
-  {
-    char *address;
-    char *condition;
-    int hardwareflag;
-    int tempflag;
-    int thread;
-    int ignore_count;
-  };
-
+/* Set a breakpoint. 
+   ARG is a string describing breakpoint address,
+   condition, and thread.
+   FLAG specifies if a breakpoint is hardware on,
+   and if breakpoint is temporary, using BP_HARDWARE_FLAG
+   and BP_TEMPFLAG.  */
+   
 static int
-do_captured_breakpoint (struct ui_out *uiout, void *data)
+break_command_1 (char *arg, int flag, int from_tty)
 {
-  struct captured_breakpoint_args *args = data;
-  struct symtabs_and_lines sals;
-  struct expression **cond;
-  struct cleanup *old_chain;
-  struct cleanup *breakpoint_chain = NULL;
-  int i;
-  char **addr_string;
-  char *cond_string = 0;
-
-  char *address_end;
-
-  /* Parse the source and lines spec.  Delay check that the expression
-     didn't contain trailing garbage until after cleanups are in
-     place. */
-  sals.sals = NULL;
-  sals.nelts = 0;
-  address_end = args->address;
-  addr_string = NULL;
-  parse_breakpoint_sals (&address_end, &sals, &addr_string, 0);
-
-  if (!sals.nelts)
-    return GDB_RC_NONE;
-
-  /* Create a chain of things at always need to be cleaned up. */
-  old_chain = make_cleanup (null_cleanup, 0);
-
-  /* Always have a addr_string array, even if it is empty. */
-  make_cleanup (xfree, addr_string);
-
-  /* Make sure that all storage allocated to SALS gets freed.  */
-  make_cleanup (xfree, sals.sals);
-
-  /* Allocate space for all the cond expressions. */
-  cond = xcalloc (sals.nelts, sizeof (struct expression *));
-  make_cleanup (xfree, cond);
-
-  /* ----------------------------- SNIP -----------------------------
-     Anything added to the cleanup chain beyond this point is assumed
-     to be part of a breakpoint.  If the breakpoint create goes
-     through then that memory is not cleaned up. */
-  breakpoint_chain = make_cleanup (null_cleanup, 0);
-
-  /* Mark the contents of the addr_string for cleanup.  These go on
-     the breakpoint_chain and only occure if the breakpoint create
-     fails. */
-  for (i = 0; i < sals.nelts; i++)
-    {
-      if (addr_string[i] != NULL)
-	make_cleanup (xfree, addr_string[i]);
-    }
-
-  /* Wait until now before checking for garbage at the end of the
-     address. That way cleanups can take care of freeing any
-     memory. */
-  if (*address_end != '\0')
-    error (_("Garbage %s following breakpoint address"), address_end);
-
-  /* Resolve all line numbers to PC's.  */
-  breakpoint_sals_to_pc (&sals, args->address);
+  int hardwareflag = flag & BP_HARDWAREFLAG;
+  int tempflag = flag & BP_TEMPFLAG;
 
-  if (args->condition != NULL)
-    {
-      cond_string = xstrdup (args->condition);
-      make_cleanup (xfree, cond_string);
-    }
-
-  create_breakpoints (sals, addr_string, args->condition,
-		      args->hardwareflag ? bp_hardware_breakpoint : bp_breakpoint,
-		      args->tempflag ? disp_del : disp_donttouch,
-		      args->thread, args->ignore_count, 0/*from-tty*/);
-
-  /* That's it. Discard the cleanups for data inserted into the
-     breakpoint. */
-  discard_cleanups (breakpoint_chain);
-  /* But cleanup everything else. */
-  do_cleanups (old_chain);
-  return GDB_RC_OK;
+  return break_command_really (arg, 
+                               NULL, 0, 1 /* parse arg */,
+                               tempflag, hardwareflag,
+                               pending_break_support, from_tty);
 }
 
+
 enum gdb_rc
 gdb_breakpoint (char *address, char *condition,
 		int hardwareflag, int tempflag,
 		int thread, int ignore_count,
 		char **error_message)
 {
-  struct captured_breakpoint_args args;
-  args.address = address;
-  args.condition = condition;
-  args.hardwareflag = hardwareflag;
-  args.tempflag = tempflag;
-  args.thread = thread;
-  args.ignore_count = ignore_count;
-  if (catch_exceptions_with_msg (uiout, do_captured_breakpoint, &args,
-				 error_message, RETURN_MASK_ALL) < 0)
-    return GDB_RC_FAIL;
-  else
-    return GDB_RC_OK;
+    return break_command_really (address, condition, thread,
+                                 0 /* condition and thread are valid.  */,
+                                 tempflag, hardwareflag,
+                                 AUTO_BOOLEAN_FALSE /* no pending. */,
+                                 0);
 }
 
 

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-01-15 20:43 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-01-11 22:05 MI/CLI breakpoint handling code duplication Nick Roberts
2008-01-15 20:43 ` Vladimir Prus
  -- strict thread matches above, loose matches on Subject: below --
2008-01-11 22:03 Nick Roberts
2007-11-08 10:26 Vladimir Prus
2007-12-14 14:08 ` Vladimir Prus
2007-12-14 17:06   ` Daniel Jacobowitz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox