Re: [PATCH] Fix create pending breakpoint handle extra_string issue if not parse_condition_and_thread

[Pedro Alves <palves@redhat.com>]

Hello,

On 03/25/2013 06:09 PM, Keith Seitz wrote:
>>
> 
> Actually I /was/ talking about create_breakpoint. As you stated, the only way to demonstrate the problem is via MI, so that's what I used to demonstrate how I think the situation should be handled.
> 
> Here's a patch which does exactly what I consider the "right" way to react to having both cond_string and a condition inside arg:

I think you're on the right track here.

> Index: breakpoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.747
> diff -u -p -r1.747 breakpoint.c
> --- breakpoint.c    20 Mar 2013 22:17:18 -0000    1.747
> +++ breakpoint.c    25 Mar 2013 17:59:36 -0000
> @@ -9659,6 +9659,11 @@ create_breakpoint (struct gdbarch *gdbar
>                  extra_string = xstrdup (extra_string);
>                  make_cleanup (xfree, extra_string);
>            }
> +        else if (*arg != '\000')
> +          {
> +        extra_string = xstrdup (arg);
> +        make_cleanup (xfree, extra_string);
> +          }

This however, leaves the erroring out to ops->create_breakpoints_sal,
and ultimately to init_breakpoint_sal:

      /* Dynamic printf requires and uses additional arguments on the
	 command line, otherwise it's an error.  */
      if (type == bp_dprintf)
	{
	  if (b->extra_string)
	    update_dprintf_command_list (b);
	  else
	    error (_("Format string required"));
	}
      else if (b->extra_string)
	error (_("Garbage '%s' at end of command"), b->extra_string);


That'd mean that e.g., for a future MI dprintf command, we'd accept
the remainder of ARG as extra string.  A MI command / python API for dprintf
should want to pass the format string separately from the location.  Given that
create_breakpoint has an explicit "extra_string" parameter, I think
the below is more appropriate.  That is, if !PARSE_CONDITION_AND_THREAD,
then ARG  is just the location, nothing else.  The fact that the describing
comment of create_breakpoint doesn't mention this just looks like
an oversight of when extra_string was added.  "parse_condition_and_thread" has
been a misnomer ever since extra_string was added.  Better rename it avoid
more confusion.  I made it "parse_arg", as that'll remain stable even
when more explicit parameters are added, and,

break_command_1:

  create_breakpoint (get_current_arch (),
		     arg,
		     NULL, 0, NULL, 1 /* parse arg */,

we even have comments calling it that already too.


>          }
> 
>        ops->create_breakpoints_sal (gdbarch, &canonical, lsal,

WDYT?

-------
gdb/
2013-04-05  Pedro Alves  <palves@redhat.com>
	    Keith Seitz  <keiths@redhat.com>

	* breakpoint.c (create_breakpoint): Rename
	"parse_condition_and_thread" parameter to "parse_arg".  Update
	describing comment.  If !PARSE_ARG, then error out if ARG is not
	the empty string after extracting the location.
	* breakpoint.h (create_breakpoint): Rename
	"parse_condition_and_thread" parameter to "parse_arg".

gdb/testsuite/
2013-04-05  Pedro Alves  <palves@redhat.com>

	* gdb.mi/mi-break.exp (test_error): Add tests with garbage after
	the location.
---

 gdb/breakpoint.c                  |   25 ++++++++++++++-----------
 gdb/breakpoint.h                  |    2 +-
 gdb/testsuite/gdb.mi/mi-break.exp |   12 ++++++++++++
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5ba1f2f..89f1a53 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9510,20 +9510,20 @@ decode_static_tracepoint_spec (char **arg_p)
 
 /* 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.  If INTERNAL is non-zero,
-   the breakpoint number will be allocated from the internal
-   breakpoint count.  Returns true if any breakpoint was created;
-   false otherwise.  */
+   modes of operations, selected by the PARSE_ARG parameter.  If
+   non-zero, the function will parse ARG, extracting location,
+   address, thread and extra string.  Otherwise, ARG is just the
+   breakpoint's location, with condition, thread, and extra string
+   specified by the COND_STRING, THREAD and EXTRA_STRING parameters.
+   If INTERNAL is non-zero, the breakpoint number will be allocated
+   from the internal breakpoint count.  Returns true if any breakpoint
+   was created; false otherwise.  */
 
 int
 create_breakpoint (struct gdbarch *gdbarch,
 		   char *arg, char *cond_string,
 		   int thread, char *extra_string,
-		   int parse_condition_and_thread,
+		   int parse_arg,
 		   int tempflag, enum bptype type_wanted,
 		   int ignore_count,
 		   enum auto_boolean pending_break_support,
@@ -9641,7 +9641,7 @@ create_breakpoint (struct gdbarch *gdbarch,
 
       lsal = VEC_index (linespec_sals, canonical.sals, 0);
 
-      if (parse_condition_and_thread)
+      if (parse_arg)
         {
 	    char *rest;
             /* Here we only parse 'arg' to separate condition
@@ -9660,6 +9660,9 @@ create_breakpoint (struct gdbarch *gdbarch,
         }
       else
         {
+	    if (*arg != '\0')
+	      error (_("Garbage '%s' at end of location"), arg);
+
             /* Create a private copy of condition string.  */
             if (cond_string)
             {
@@ -9699,7 +9702,7 @@ create_breakpoint (struct gdbarch *gdbarch,
       init_raw_breakpoint_without_location (b, gdbarch, type_wanted, ops);
 
       b->addr_string = copy_arg;
-      if (parse_condition_and_thread)
+      if (parse_arg)
 	b->cond_string = NULL;
       else
 	{
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 68f3ed9..d740625 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1269,7 +1269,7 @@ enum breakpoint_create_flags
 extern int create_breakpoint (struct gdbarch *gdbarch, char *arg,
 			      char *cond_string, int thread,
 			      char *extra_string,
-			      int parse_condition_and_thread,
+			      int parse_arg,
 			      int tempflag, enum bptype wanted_type,
 			      int ignore_count,
 			      enum auto_boolean pending_break_support,
diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
index 9cf0126..d5d58c7 100644
--- a/gdb/testsuite/gdb.mi/mi-break.exp
+++ b/gdb/testsuite/gdb.mi/mi-break.exp
@@ -196,6 +196,18 @@ proc test_error {} {
     mi_gdb_test "-var-update *" \
         "\\^done,changelist=\\\[\\\]" \
         "update varobj for function call"    
+
+    # Try setting breakpoints with garbage after the location.
+
+    # "if" only works in the CLI.  It's not supposed to be accepted by
+    # MI.  The way to specify a condition is with -c.
+    mi_gdb_test "-break-insert \"callme if i < 4\"" \
+        ".*\\^error,msg=\"Garbage 'if i < 4' at end of location\"" \
+        "breakpoint with garbage after location"
+
+    mi_gdb_test "-break-insert -c i==4 \"callme if i < 4\"" \
+        ".*\\^error,msg=\"Garbage 'if i < 4' at end of location\"" \
+        "conditional breakpoint with garbage after location"
 }
 
 proc test_disabled_creation {} {