This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch/MI] Implementation for break-catch command


On Thursday 29 May 2008 18:48:36 Aleksandar Ristovski wrote:
> Index: gdb/breakpoint.c

First of all, I would like a some testcases to be added with this patch.

I have some questions about behaviour, too:

1. -break-catch throw
   ^done,bkpt={number="3",type="breakpoint",disp="keep",enabled="y",
               addr="0xb7ef6e05",what="exception throw",times="0",
               original-location="__cxa_throw"}
   (gdb)

The way this breakpoint is printed seems not sufficiently precise. Suppose
such a breakpoint was added via CLI. GUI would like to detect this, and
add a breakpoint to the breakpoint list as "catch" breakpoint. How can we
detect it is a catch breakpoint? The "what" field appears to be of a fairly
ad-hoc use in MI. Should the type of breakpoint be actually 'exception-throw'?

2. -break-catch -c "1==1" throw
   ^error,msg="Junk at end of arguments."

How exactly can one set a condition for a catch breakpoint? This is one reason
I'd like to see a testcase -- it makes it absolutely clear how things are
supposed to work.

> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.322
> diff -u -p -r1.322 breakpoint.c
> --- gdb/breakpoint.c????28 May 2008 14:04:21 -0000??????1.322
> +++ gdb/breakpoint.c????29 May 2008 14:44:59 -0000
> @@ -67,8 +67,6 @@
> ?static void until_break_command_continuation (struct continuation_arg *arg, 
> ???????????????????????????????????????? ? ? ?int error);
> ?
> -static void catch_command_1 (char *, int, int);
> -
> ?static void enable_delete_command (char *, int);
> ?
> ?static void enable_delete_breakpoint (struct breakpoint *);
> @@ -6549,13 +6547,16 @@ print_mention_exception_catchpoint (stru
> ? ?int bp_temp;
> ? ?int bp_throw;
> ?
> - ?bp_temp = b->loc->owner->disposition == disp_del;
> - ?bp_throw = strstr (b->addr_string, "throw") != NULL;
> - ?ui_out_text (uiout, bp_temp ? _("Temporary catchpoint ")
> -??????????????????????? ? ? ?: _("Catchpoint "));
> - ?ui_out_field_int (uiout, "bkptno", b->number);
> - ?ui_out_text (uiout, bp_throw ? _(" (throw)")
> -??????????????????????? ? ? ? : _(" (catch)"));
> + ?if (!ui_out_is_mi_like_p (uiout))
> + ? ?{
> + ? ? ?bp_temp = b->loc->owner->disposition == disp_del;
> + ? ? ?bp_throw = strstr (b->addr_string, "throw") != NULL;
> + ? ? ?ui_out_text (uiout, bp_temp ? _("Temporary catchpoint ")
> +??????????????????????????????? ?: _("Catchpoint "));
> + ? ? ?ui_out_field_int (uiout, "bkptno", b->number);
> + ? ? ?ui_out_text (uiout, bp_throw ? _(" (throw)")
> +??????????????????????????????? ? : _(" (catch)"));
> + ? ?}

Uh, seems like there's some messiness in MI here. For ordinary breakpoints,
MI installs the 'breakpoint_notify' hook that prints all the information, and 'mention'
specifically avoids printing anything for MI. For watchpoints, however, no hooks
are installed, and 'mention' does print something -- basically just id and expression.

For -break-catch, you choose to install hooks, and disable printing anything in
'mention' -- seems fine to me.

> +@subsubheading Synopsis
> +
> +@smallexample
> + -break-catch [ -t ] [ @var{event} ]
> +@end smallexample
> +
> +@noindent
> +where @var{event} can be one of:
> +
> +@itemize @bullet
> +@item catch
> +@item throw
> +@end itemize
> +
> +The possible optional parameters of this command are:
> +
> +@table @samp
> +@item -t
> +Insert a temporary catchpoint.
> +@end table
> +
> +@subsubheading Result
> +
> +The result is in the form:
> +
> +@smallexample
> +^done,bkpt=@{number="@var{number}",type="@var{type}",
> +disp="del"|"keep",enabled="y"|"n",addr="@var{hex}",
> +what="exception catch"|"exception throw",times="@var{times}",
> +original-location="@var{funcname}"@}
> +@end smallexample
> +
> +@noindent
> +where @var{number} is the @value{GDBN} number for this catchpoint,
> +@var{funcname} is the name of the function where the catchpoint
> +was inserted (target specific) and @var{times} the number of times
> +that the catchpoint has been hit (always 0 for @code{-break-catch}
> +but may be greater for @code{-break-info} or @code{-break-list} 
> +which use the same output).

Is @var{type} documented here?

> Index: gdb/mi/mi-cmd-break.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mi/mi-cmd-break.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 mi-cmd-break.c
> --- gdb/mi/mi-cmd-break.c???????1 Feb 2008 16:24:46 -0000???????1.19
> +++ gdb/mi/mi-cmd-break.c???????29 May 2008 14:45:07 -0000
> @@ -241,3 +241,98 @@ mi_cmd_break_watch (char *command, char 
> ? ? ?}
> ? ?return MI_CMD_DONE;
> ?}
> +
> +/* Implements the -break-catch command.
> + ? See the MI manual for the list of possible options. ?*/
> +
> +enum mi_cmd_result
> +mi_cmd_break_catch (char *command, char **argv, int argc)
> +{
> + ?char *event_name = NULL;
> + ?enum bp_type type = REG_BP;
> + ?int temp_p = 0;
> + ?int thread = -1;
> + ?int ignore_count = 0;
> + ?char *condition = NULL;
> + ?struct gdb_exception e;
> + ?struct gdb_events *old_hooks;
> + ?char argument[250];
> + ?enum opt
> + ? ?{
> + ? ? ?HARDWARE_OPT, TEMP_OPT, CONDITION_OPT,
> + ? ? ?IGNORE_COUNT_OPT, THREAD_OPT
> + ? ?};
> + ?static struct mi_opt opts[] =
> + ?{
> + ? ?{"h", HARDWARE_OPT, 0},
> + ? ?{"t", TEMP_OPT, 0},
> + ? ?{"c", CONDITION_OPT, 1},
> + ? ?{"i", IGNORE_COUNT_OPT, 1},
> + ? ?{"p", THREAD_OPT, 1},
> + ? ?{ 0, 0, 0 }
> + ?};
> +
> + ?/* Parse arguments. It could be -r or -h or -t, <location> or ``--''
> + ? ? to denote the end of the option list. */
> + ?int optind = 0;
> + ?char *optarg;
> + ?while (1)
> + ? ?{
> + ? ? ?int opt = mi_getopt ("mi_cmd_break_catch", argc, argv, opts, &optind, &optarg);
> + ? ? ?if (opt < 0)
> +???????break;
> + ? ? ?switch ((enum opt) opt)
> +???????{
> +???????case TEMP_OPT:
> +??????? ?temp_p = 1;
> +??????? ?break;
> +???????case HARDWARE_OPT:
> +??????? ?warning (_("Hardware flag ignored for catchpoints"));
> +??????? ?break;
> +???????case CONDITION_OPT:
> +??????? ?condition = optarg;
> +??????? ?break;
> +???????case IGNORE_COUNT_OPT:
> +??????? ?ignore_count = atol (optarg);
> +??????? ?warning (_("Ignore count not yet implemented for catchpoints"));
> +??????? ?break;
> +???????case THREAD_OPT:
> +??????? ?thread = atol (optarg);
> +??????? ?warning (_("Thread option not yet implemented for catchpoints"));
> +??????? ?break;
> +???????}

I honestly don't know if parsing undocumented options and reporting that they
are not supported is a good idea. Are you planning to support those other options
soon? If not, the code might be cleaner without support for this unsupported options.

> + ? ?}
> +
> + ?if (optind >= argc)
> + ? ?error (_("mi_cmd_break_catch: Missing <event name>"));
> + ?if (optind < argc - 1)
> + ? ?error (_("mi_cmd_break_catch: Garbage following <event name>"));

Please remove "mi_cmd_break_catch:" here. MI error messages should not mention
the names of internal functions.

> + ?event_name = argv[optind];
> +
> + ?if (condition != NULL)
> + ? ?snprintf (argument, sizeof (argument), "%s %s", event_name, condition);

Fixed size buffer seems wrong here. Please use 'concat' or something else from
libiberty. Also, you need "if" between event name and condition for condition
to work.

> + ?else
> + ? ?strcpy (argument, event_name);
> + ?/* Now we have what we need, let's insert the breakpoint! */
> + ?old_hooks = deprecated_set_gdb_event_hooks (&breakpoint_hooks);
> + ?/* Make sure we restore hooks even if exception is thrown. ?*/
> + ?TRY_CATCH (e, RETURN_MASK_ALL)
> + ? ?{
> + ? ? ?switch (type)
> +???????{
> +???????case REG_BP:
> +??????? ?catch_command_1 (argument, temp_p, 0);
> +??????? ?break;
> + ? ? ?default:
> +??????? ?internal_error (__FILE__, __LINE__,
> +??????????????????????? ?_("mi_cmd_break_catch: Bad switch."));
> +???????}

Just kill this switch -- if we even implement hardware catch then
a 'hardware' flag will be sufficient.

- Volodya


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]