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

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

[Hui Zhu]

Hi,

I found that behavior is not right is create pending breakpoint handle extra_string issue if not parse_condition_and_thread.
This issue cannot be reproduce with normal interpreter command "b" because it create breakpoint with parse_condition_and_thread.
But mi command "-break-insert" have this issue, for example:
gdb/testsuite$ gdb -i mi gdb.base/pending
(gdb)
-break-insert -f -c k>0 "pendfunc1 if k == 0"
&"Function \"pendfunc1\" not defined.\n"
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="<PENDING>",pending="pendfunc1 if k == 0",cond="k>0",times="0",original-location="pendfunc1 if k == 0"}
(gdb)
info b
&"info b\n"
~"Num     Type           Disp Enb Address    What\n"
~"1       breakpoint     keep y   <PENDING>  pendfunc1 if k == 0\n"
~"\tstop only if k>0\n"
^done
(gdb)
r
&"r\n"
~"Starting program: /home/teawater/gdb/bgdbno/gdb/testsuite/gdb.base/pending \n"
=thread-group-started,id="i1",pid="7861"
=thread-created,id="1",group-id="i1"
^running
*running,thread-id="all"
=library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-group="i1"
(gdb)
=library-loaded,id="/home/teawater/gdb/bgdbno/gdb/testsuite/gdb.base/pendshr.sl",target-name="/home/teawater/gdb/bgdbno/gdb/testsuite/gdb.base/pendshr.sl",host-name="/home/teawater/gdb/bgdbno/gdb/testsuite/gdb.base/pendshr.sl",symbols-loaded="0",thread-group="i1"
=library-loaded,id="/lib/x86_64-linux-gnu/libc.so.6",target-name="/lib/x86_64-linux-gnu/libc.so.6",host-name="/lib/x86_64-linux-gnu/libc.so.6",symbols-loaded="0",thread-group="i1"
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x00007ffff7bd8607",func="pendfunc1",file="../../../src/gdb/testsuite/gdb.base/pendshr.c",fullname="/home/teawater/gdb/src/gdb/testsuite/gdb.base/pendshr.c",line="22",thread-groups=["i1"],cond="k == 0",times="0",original-location="pendfunc1 if k == 0"}
=breakpoint-modified,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x00007ffff7bd8607",func="pendfunc1",file="../../../src/gdb/testsuite/gdb.base/pendshr.c",fullname="/home/teawater/gdb/src/gdb/testsuite/gdb.base/pendshr.c",line="22",thread-groups=["i1"],cond="k == 0",times="1",original-location="pendfunc1 if k == 0"}
*stopped,reason="breakpoint-hit",disp="keep",bkptno="1",frame={addr="0x00007ffff7bd8607",func="pendfunc1",args=[{name="x",value="3"}],file="../../../src/gdb/testsuite/gdb.base/pendshr.c",fullname="/home/teawater/gdb/src/gdb/testsuite/gdb.base/pendshr.c",line="22"},thread-id="1",stopped-threads="all",core="6"
info b
&"info b\n"
~"Num     Type           Disp Enb Address            What\n"
~"1       breakpoint     keep y   0x00007ffff7bd8607 in pendfunc1 at ../../../src/gdb/testsuite/gdb.base/pendshr.c:22\n"
~"\tstop only if k == 0\n"
~"\tbreakpoint already hit 1 time\n"
^done
(gdb)

With 2 "info b", you can found that condition "k>0" is overwrite by "k==0".

So I post a patch to fix this issue.
Please help me review it.

Best,
Hui

2013-03-24  Hui Zhu  <hui@codesourcery.com>

	* breakpoint.c (create_breakpoint): Handle extra_string if
	parse_condition_and_thread.
	(addr_string_to_sals): Add check for pending breakpoints.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9687,7 +9687,11 @@ create_breakpoint (struct gdbarch *gdbar
  
        b->addr_string = copy_arg;
        if (parse_condition_and_thread)
-	b->cond_string = NULL;
+        {
+	  b->cond_string = NULL;
+	  b->extra_string = NULL;
+	  b->condition_not_parsed = 1;
+	}
        else
  	{
  	  /* Create a private copy of condition string.  */
@@ -9697,11 +9701,17 @@ create_breakpoint (struct gdbarch *gdbar
  	      make_cleanup (xfree, cond_string);
  	    }
  	  b->cond_string = cond_string;
+	  /* Create a private copy of extra string.  */
+	  if (extra_string)
+	    {
+	      extra_string = xstrdup (extra_string);
+	      make_cleanup (xfree, extra_string);
+	    }
+	  b->extra_string = extra_string;
+	  b->condition_not_parsed = 0;
  	}
-      b->extra_string = NULL;
        b->ignore_count = ignore_count;
        b->disposition = tempflag ? disp_del : disp_donttouch;
-      b->condition_not_parsed = 1;
        b->enable_state = enabled ? bp_enabled : bp_disabled;
        if ((type_wanted != bp_breakpoint
             && type_wanted != bp_hardware_breakpoint) || thread != -1)
@@ -14166,7 +14176,8 @@ addr_string_to_sals (struct breakpoint *
  	 breakpoint being disabled, and don't want to see more
  	 errors.  */
        if (e.error == NOT_FOUND_ERROR
-	  && (b->condition_not_parsed
+	  && (b->condition_not_parsed
+	      || (b->loc == NULL || b->loc->shlib_disabled)
  	      || (b->loc && b->loc->shlib_disabled)
  	      || (b->loc && b->loc->pspace->executing_startup)
  	      || b->enable_state == bp_disabled))

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

[Keith Seitz]

On 03/23/2013 10:21 PM, Hui Zhu wrote:

> -break-insert -f -c k>0 "pendfunc1 if k == 0"
> &"Function \"pendfunc1\" not defined.\n"
> ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="<PENDING>",pending="pendfunc1
> if k == 0",cond="k>0",times="0",original-location="pendfunc1 if k == 0"}

IMO, this should not be permitted at all. Two reasons:

1) We could argue about which condition is correct to keep.
2) The MI specification does not say anything about permitting this type 
of usage. [I would therefore argue it is illegal.]

Likewise for "thread" (and "task", but there is no task parameter for 
create_breakpoint for some reason).

Ideally an error should be issued when the breakpoint is set:

-break-insert -c "argc > 1" "main if argc > 2"
^error,msg="Garbage 'if argc > 2' at end of command"

Please remember to submit a test case (or cases) with patches whenever 
possible.

Keith

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

[Hui Zhu]

Hi Keith,

Thanks for your review.

On Sun, Mar 24, 2013 at 8:02 PM, Keith Seitz <keiths@redhat.com> wrote:
> On 03/23/2013 10:21 PM, Hui Zhu wrote:
>
>> -break-insert -f -c k>0 "pendfunc1 if k == 0"
>> &"Function \"pendfunc1\" not defined.\n"
>>
>> ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="<PENDING>",pending="pendfunc1
>> if k == 0",cond="k>0",times="0",original-location="pendfunc1 if k == 0"}
>
>
> IMO, this should not be permitted at all. Two reasons:
>
> 1) We could argue about which condition is correct to keep.
> 2) The MI specification does not say anything about permitting this type of
> usage. [I would therefore argue it is illegal.]
>
> Likewise for "thread" (and "task", but there is no task parameter for
> create_breakpoint for some reason).
>
> Ideally an error should be issued when the breakpoint is set:
>
> -break-insert -c "argc > 1" "main if argc > 2"
> ^error,msg="Garbage 'if argc > 2' at end of command"

I am sorry that what you care about is the issue that affect the mi.
But my patch is for the issue inside the function create_breakpoint.
I post the mi commands to show the issue is because it call
create_breakpoint with parse_condition_and_thread is 0 and easy to
show how it can affect the behavior of a pending breakpoint.
I think this issue also affect other functions that call
create_breakpoint with parse_condition_and_thread is 0.

So if you don't mind, please let us focus on create_breakpoint first.

And we also can do some work in:
-break-insert -c "argc > 1" "main if argc > 2"
> ^error,msg="Garbage 'if argc > 2' at end of command"

>
> Please remember to submit a test case (or cases) with patches whenever
> possible.

Thanks for you mind.  I will do it later.

Best,
Hui

>
> Keith

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

[Yao Qi]

On 03/25/2013 11:45 AM, Hui Zhu wrote:
> But my patch is for the issue inside the function create_breakpoint.
> I post the mi commands to show the issue is because it call
> create_breakpoint with parse_condition_and_thread is 0 and easy to
> show how it can affect the behavior of a pending breakpoint.
> I think this issue also affect other functions that call
> create_breakpoint with parse_condition_and_thread is 0.

Hui,
The comment of create_breakpoint says something,

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

in other words, it is invalid to put condition into ARG and set 
parse_condition_and_thread 0.

-- 
Yao (齐尧)

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

[Hui Zhu]

On Mon, Mar 25, 2013 at 9:31 PM, Yao Qi <yao@codesourcery.com> wrote:
> On 03/25/2013 11:45 AM, Hui Zhu wrote:
>>
>> But my patch is for the issue inside the function create_breakpoint.
>> I post the mi commands to show the issue is because it call
>> create_breakpoint with parse_condition_and_thread is 0 and easy to
>> show how it can affect the behavior of a pending breakpoint.
>> I think this issue also affect other functions that call
>> create_breakpoint with parse_condition_and_thread is 0.
>
>
> Hui,
> The comment of create_breakpoint says something,
>
> /* 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.  */
>
> in other words, it is invalid to put condition into ARG and set
> parse_condition_and_thread 0.

Yao, thanks for your remind.  And my patch just to make
create_breakpoint handle pending breakpoint follow this comments,
right?  :)


*If non-zero, the function will parse arg, extracting breakpoint
location, address and thread. *
So if it is zero, the function should get arg and anything from
argument of this function, right?

But "-break-insert -f -c k>0 "pendfunc1 if k == 0" " is a example that
it is zero, and GDB get condition from arg, it is a right behavior?

And extra_string just be dropped even if parse_condition_and_thread is
non-zero, is that right?

Thanks,
Hui

>
> --
> Yao (齐尧)

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

[Tom Tromey]

>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:

Hui> I am sorry that what you care about is the issue that affect the mi.
Hui> But my patch is for the issue inside the function create_breakpoint.

In this case, it seems to me that the API must be a bad one.
Can't we just tell callers, "don't do that"?
To me it seems like a pathological case.

Tom

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

[Keith Seitz]

On 03/25/2013 10:14 AM, Tom Tromey wrote:
>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>
> Hui> I am sorry that what you care about is the issue that affect the mi.
> Hui> But my patch is for the issue inside the function create_breakpoint.

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:

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

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

> In this case, it seems to me that the API must be a bad one.

Yes, that API extension was a horribly implemented (quick and dirty), 
but create_breakpoint is a bit of a mess, since it not only has to deal 
with setting breakpoints (of various varieties), it also has to deal 
with parsing user input. I'm not a fan of this (too common) paradigm.

> Can't we just tell callers, "don't do that"?
> To me it seems like a pathological case.

We can certainly enforce this, as my patchlet above demonstrates:

-break-insert -c "argc > 1" "main if argc > 2"
^error,msg="Garbage 'if argc > 2' at end of command"

Keith

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

[Hui Zhu]

On Tue, Mar 26, 2013 at 2:09 AM, Keith Seitz <keiths@redhat.com> wrote:
> On 03/25/2013 10:14 AM, Tom Tromey wrote:
>>>>>>>
>>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>>
>>
>> Hui> I am sorry that what you care about is the issue that affect the mi.
>> Hui> But my patch is for the issue inside the function create_breakpoint.
>
>
> 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:

This function have parse_condition_and_thread.  It already choice
which part it will get the cond_string from, why we still need this
check?
Also it didn't handle this pending breakpoints.

>
> 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);
> +             }
>          }
>
>        ops->create_breakpoints_sal (gdbarch, &canonical, lsal,
>
>
>> In this case, it seems to me that the API must be a bad one.
>
>
> Yes, that API extension was a horribly implemented (quick and dirty), but
> create_breakpoint is a bit of a mess, since it not only has to deal with
> setting breakpoints (of various varieties), it also has to deal with parsing
> user input. I'm not a fan of this (too common) paradigm.
>

And if we want add comments on this function to let people don't do
that.  We need also tell they, the pending breakpoints's extra_string
will be dropped.

>
>> Can't we just tell callers, "don't do that"?
>> To me it seems like a pathological case.
>
>
> We can certainly enforce this, as my patchlet above demonstrates:
>
>
> -break-insert -c "argc > 1" "main if argc > 2"
> ^error,msg="Garbage 'if argc > 2' at end of command"
>
> Keith

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

[Pedro Alves]

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

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

[Keith Seitz]

On 04/05/2013 10:50 AM, Pedro Alves wrote:
> I think you're on the right track here.

That's good to know. :-) Honestly, though, that was just meant as an 
example -- not a definitive solution to the problem.

> That'd mean that e.g., for a future MI dprintf command, we'd accept
> the remainder of ARG as extra string.

Gah. I totally spaced on MI.

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

A very welcome change.

> WDYT?

It looks good to me.

Thank you again for your review of my review. Hopefully next time I'll 
be a bit better at it!

Keith

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

[Pedro Alves]

On 04/06/2013 10:49 PM, Keith Seitz wrote:

> It looks good to me.

> Thank you again for your review of my review. Hopefully next time I'll be a bit better at it!

You're too hard on yourself.  :-)

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

I noticed that "address" in "location, address and thread" must
be a typo for "condition".  I've fixed that too now, and checked it all in.

----------
Subject: create_breapoint / explicit mode: Error out if there's garbage after the breakpoint location.

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.
This makes it "parse_arg", as that'll remain stable even if/when more
explicit parameters are added.

gdb/
2013-04-08  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-08  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..667bedd 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,
+   condition, 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 {} {