This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fix create pending breakpoint handle extra_string issue if not parse_condition_and_thread
- From: Pedro Alves <palves at redhat dot com>
- To: Keith Seitz <keiths at redhat dot com>
- Cc: Hui Zhu <hui_zhu at mentor dot com>, Tom Tromey <tromey at redhat dot com>, Hui Zhu <teawater at gmail dot com>, gdb-patches ml <gdb-patches at sourceware dot org>
- Date: Fri, 05 Apr 2013 18:50:37 +0100
- Subject: Re: [PATCH] Fix create pending breakpoint handle extra_string issue if not parse_condition_and_thread
- References: <514E8D6C dot 2010606 at mentor dot com> <514EEB43 dot 6040101 at redhat dot com> <CANFwon3rNhdJaaYr1UDy3XJTY2-s5N74QB_LmgVi2d-Qcp4FWQ at mail dot gmail dot com> <87620ftn9f dot fsf at fleche dot redhat dot com> <515092F2 dot 2000307 at redhat dot 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 {} {