This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] (Ada) New command to stop at start of exception handler.
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Xavier Roirand <roirand at adacore dot com>, Eli Zaretskii <eliz at gnu dot org>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 14 Dec 2017 18:29:57 +0400
- Subject: Re: [RFA] (Ada) New command to stop at start of exception handler.
- Authentication-results: sourceware.org; auth=none
- References: <1513094242-2313-1-git-send-email-roirand@adacore.com>
[Hi Eli - this patch adds a NEWS entry as well as some documentation
in the manual; so To'ing you]
On Tue, Dec 12, 2017 at 04:57:22PM +0100, Xavier Roirand wrote:
> When using gdb for debugging Ada source code, there are several catchpoint
> types you can define in order to stop upon certain conditions. Let's
> use this small example:
>
> procedure Foo is
> begin
> begin
> raise Constraint_Error;
> exception
> when Program_Error =>
> null;
> when Constraint_Error =>
> null;
> when others =>
> null;
> end;
> end Foo;
>
> One can stop when the exception is being raised by using the exception
> catchpoint like below:
>
> (gdb) catch exception
> Catchpoint 1: all Ada exceptions
> (gdb)
>
> In that case, when running Foo, gdb will stop at the line where the exception
> was raised:
>
> begin
> >>> raise Constraint_Error;
> exception
>
> This patch introduces new type of catchpoint, when the user wants to stop
> at the location of the exception handling.
> Imagine we want to stop on any exception handled by the program, we can do:
>
> (gdb) catch handle
> Catchpoint 1: all Ada exceptions handler
After consulting with a native English speaker, let's add an 's' to
'handler', so:
(gdb) catch handle
Catchpoint 1: all Ada exceptions handlers
Also, I just realized that we had said internally that we would
name the command "catch handler" (with an 'r' at the end), and
now that I'm thinking about it more, I think it would be even
more natural to name it "catch handlers" (with an additional 's'
at the end).
Can you make that change?
> (gdb) r
> Starting program: /tmp/foo
>
> By doing so, when running Foo, gdb will stop here:
>
> Catchpoint 1, exception at 0x000000000040255a in foo () at foo.adb:25
> 25 when Constraint_Error =>
> (gdb)
>
> It is also possible to stop when the Constraint_Error exception is being
> handled in this program. With this patch, we can use:
>
> (gdb) catch handle Constraint_Error
> Catchpoint 1: `Constraint_Error' Ada exception handler
Same as above, please. Let's name the command "catch handlers",
and then add an 's' at the end of 'handler' in the command's output.
> (gdb)
>
> Like for other catchpoint, you can set a condition when adding a catchpoint.
> Here the catchpoint checks Global_Var:
>
> (gdb) catch exception_handle if Global_Var /= 0
>
> gdb/ChangeLog:
>
> * ada-lang.h (ada_exception_catchpoint_kind): Add ada_catch_handle
> type.
> * ada-lang.c (struct exception_support_info): Add field.
Please specify the name of the field you are adding. You can do it
the following way:
* ada-lang.c (struct exception_support_info) <catch_handle_sym>:
Add field.
> (struct default_exception_support_info): Add field.
> (exception_support_info_fallback): Add field.
Same for these two.
> (ada_exception_name_addr_1): Add "catch handle" handling.
> (ada_exception_catchpoint_cond_string): Add exception type parameter.
Here you can say:
(ada_exception_catchpoint_cond_string): Add exception type
parameter. Update all callers.
This allows you to avoid having to repeat the same thing multiple
times below.
Ref: The GNU Coding Standards
https://www.gnu.org/prep/standards/standards.html#Simple-Changes
No need to do that for this patch, now that you've already typed
everything, but you can use that in the future.
One thing you can do to be more descriptive however, just put the name
of the parameter in the area reserved for indicating the part that
changed. Thus:
(ada_exception_catchpoint_cond_string) <ex>: Add exception type
parameter. Update all callers.
Many people will even simply say:
(ada_exception_catchpoint_cond_string) <ex>: New parameter.
All callers updated.
> (create_excep_cond_exprs): Add exception type parameter, call
> ada_exception_catchpoint_cond_string with one more parameter.
> (re_set_exception): call create_excep_cond_exprs with one more
> parameter.
> (print_it_exception): Add "catch handle" handling.
> (print_one_exception): Add "catch handle" handling.
> (print_mention_exception): Add "catch handle" handling.
> (print_recreate_exception): Add "catch handle" handling.
For the future, you can shorten the above into a single entry
affecting multiple functions, like so:
(print_it_exception, print_one_exception, print_mention_exception)
(print_recreate_exception): Add "catch handle" handling.
Please note how I close the '(' with a ')' on each line.
Again, no need to redo it here; it's fine but the above can help you
spend less time on those $#*&! ChangeLog entries.
> (allocate_location_catch_handle): New function.
> (re_set_catch_handle): New function.
> (check_status_catch_handle): New function.
> (print_it_catch_handle): New function.
> (print_one_catch_handle): New function.
> (print_mention_catch_handle): New function.
> (print_recreate_catch_handle): New function.
Same as above here; you can save a bit of time by aggregating
all those entries.
> (catch_ada_exception_command_split): Add handle parameter to handle
> "catch handle" case and add "catch handle" handling.
Again, I think it's clearer for reader, and often shorter for you,
if you use the notation I recommended above for specifing the name
of the parameter:
(catch_ada_exception_command_split) <handle>: New parameter.
> (ada_exception_sym_name): Add "catch handle" handling.
> (ada_exception_breakpoint_ops): Add "catch handle" handling.
> (ada_exception_catchpoint_cond_string): Add exception type parameter
> and add "catch handle" handling.
> (create_ada_exception_catchpoint): call create_excep_cond_exprs
> with one more parameter.
> (catch_ada_handle_command): New function.
> (initialize_ada_catchpoint_ops): Initialize "catch handler" operations
> structure.
> (_initialize_ada_language): Add "catch handle" command entry.
> * NEWS: Document "catch handle" feature.
You don't mention this in this patch, but we need to also worry
about GDB/MI (and having the corresponding testcase). This can be
done with a separate patch, but let's get that part started too.
>
> gdb/testsuite/ChangeLog:
> gdb.ada/excep_handle.exp: New test.
test -> testcase (a test is the individual "gdb_test" execution,
for instance).
> gdb.ada/excep_handle/foo.adb: New file.
> gdb.ada/excep_handle/pck.ads: New file.
>
> gdb/doc/ChangeLog:
>
> * gdb.textinfo (Set Catchpoints): Add documentation for
> new "catch handle" action.
While this is not an absolute rule, I think we would typically
put the documentation changes ahead of the testsuite ones.
Since there are some additional comments below, can you swap
those two, please?
> ---
> gdb/NEWS | 3 +
> gdb/ada-lang.c | 216 ++++++++++++++++++++++++++---
> gdb/ada-lang.h | 3 +-
> gdb/doc/gdb.texinfo | 19 +++
> gdb/testsuite/gdb.ada/excep_handle.exp | 159 +++++++++++++++++++++
> gdb/testsuite/gdb.ada/excep_handle/foo.adb | 103 ++++++++++++++
> gdb/testsuite/gdb.ada/excep_handle/pck.ads | 19 +++
> 7 files changed, 499 insertions(+), 23 deletions(-)
> create mode 100644 gdb/testsuite/gdb.ada/excep_handle.exp
> create mode 100644 gdb/testsuite/gdb.ada/excep_handle/foo.adb
> create mode 100644 gdb/testsuite/gdb.ada/excep_handle/pck.ads
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index c6fe297..ef5e348 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -656,6 +656,9 @@ show max-value-size
> * Support for reading/writing memory and extracting values on architectures
> whose memory is addressable in units of any integral multiple of 8 bits.
>
> +catch handle
> + Allows to break when an Ada exception is handled.
> +
> * New remote packets
>
> exec stop reason
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 9e637eb..373a9a3 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -11833,6 +11833,10 @@ struct exception_support_info
> a catchpoint on failed assertions. */
> const char *catch_assert_sym;
>
> + /* The name of the symbol to break on in order to insert
> + a catchpoint on exception handling. */
> + const char *catch_handle_sym;
> +
> /* Assuming that the inferior just triggered an unhandled exception
> catchpoint, this function is responsible for returning the address
> in inferior memory where the name of that exception is stored.
> @@ -11852,6 +11856,7 @@ static const struct exception_support_info default_exception_support_info =
> "__gnat_debug_raise_exception", /* catch_exception_sym */
> "__gnat_unhandled_exception", /* catch_exception_unhandled_sym */
> "__gnat_debug_raise_assert_failure", /* catch_assert_sym */
> + "__gnat_begin_handler", /* catch_handle_sym */
> ada_unhandled_exception_name_addr
> };
>
> @@ -11864,6 +11869,7 @@ static const struct exception_support_info exception_support_info_fallback =
> "__gnat_raise_nodefer_with_msg", /* catch_exception_sym */
> "__gnat_unhandled_exception", /* catch_exception_unhandled_sym */
> "system__assertions__raise_assert_failure", /* catch_assert_sym */
> + "__gnat_begin_handler", /* catch_handle_sym */
> ada_unhandled_exception_name_addr_from_raise
> };
>
> @@ -12132,7 +12138,12 @@ ada_exception_name_addr_1 (enum ada_exception_catchpoint_kind ex,
> case ada_catch_exception_unhandled:
> return data->exception_info->unhandled_exception_name_addr ();
> break;
> -
> +
> + case ada_catch_handle:
> + return 0; /* The runtimes does not provide access to the exception
> + name. */
> + break;
> +
> case ada_catch_assert:
> return 0; /* Exception name is not relevant in this case. */
> break;
> @@ -12238,7 +12249,8 @@ ada_exception_name_addr (enum ada_exception_catchpoint_kind ex,
> return result;
> }
>
> -static char *ada_exception_catchpoint_cond_string (const char *excep_string);
> +static char *ada_exception_catchpoint_cond_string (const char *excep_string,
> + enum ada_exception_catchpoint_kind ex);
This line is too long; you can do the following instead:
static char *ada_exception_catchpoint_cond_string
(const char *excep_string,
enum ada_exception_catchpoint_kind ex);
>
> /* Ada catchpoints.
>
> @@ -12301,7 +12313,8 @@ struct ada_catchpoint : public breakpoint
> catchpoint's locations, and store them for later evaluation. */
>
> static void
> -create_excep_cond_exprs (struct ada_catchpoint *c)
> +create_excep_cond_exprs (struct ada_catchpoint *c,
> + enum ada_exception_catchpoint_kind ex)
> {
> struct cleanup *old_chain;
> struct bp_location *bl;
> @@ -12317,7 +12330,7 @@ create_excep_cond_exprs (struct ada_catchpoint *c)
>
> /* Compute the condition expression in text form, from the specific
> expection we want to catch. */
> - cond_string = ada_exception_catchpoint_cond_string (c->excep_string);
> + cond_string = ada_exception_catchpoint_cond_string (c->excep_string, ex);
> old_chain = make_cleanup (xfree, cond_string);
>
> /* Iterate over all the catchpoint's locations, and parse an
> @@ -12385,7 +12398,7 @@ re_set_exception (enum ada_exception_catchpoint_kind ex, struct breakpoint *b)
>
> /* Reparse the exception conditional expressions. One for each
> location. */
> - create_excep_cond_exprs (c);
> + create_excep_cond_exprs (c, ex);
> }
>
> /* Returns true if we should stop for this breakpoint hit. If the
> @@ -12474,6 +12487,7 @@ print_it_exception (enum ada_exception_catchpoint_kind ex, bpstat bs)
> {
> case ada_catch_exception:
> case ada_catch_exception_unhandled:
> + case ada_catch_handle:
> {
> const CORE_ADDR addr = ada_exception_name_addr (ex, b);
> char exception_name[256];
> @@ -12557,22 +12571,36 @@ print_one_exception (enum ada_exception_catchpoint_kind ex,
> case ada_catch_exception:
> if (c->excep_string != NULL)
> {
> - char *msg = xstrprintf (_("`%s' Ada exception"), c->excep_string);
> + char *info = xstrprintf (_("`%s' Ada exception"), c->excep_string);
> + struct cleanup *old_chain = make_cleanup (xfree, info);
>
> - uiout->field_string ("what", msg);
> - xfree (msg);
> + uiout->text (info);
> + do_cleanups (old_chain);
Can you please explain this change (which is actually two changes)?
I don't understand why:
- Although not technically incorrect, I'm wondering why you chose
to replace the free by a make_cleanup/do_cleanup sequence.
Considering that cleanups are progressively being phased out,
this seems like an unnecessary regression in that regard.
- You chose to use uiout->text instead of uiout->field_string
While it may look the same when using the GDB/CLI (default)
command-line interface, it changes the behavior in GDB/MI,
where the information about which exception we are breaking on
is no longer displayed correctly. You now get...
-catch-exception
^done,bkptno="2",bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000408eeb",thread-groups=["i1"],times="0",original-location="__gnat_debug_raise_exception"}
... instead of (look for the field "what", which is missing
from the output above):
-catch-exception
^done,bkptno="2",bkpt={number="2",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000408eeb",what="all Ada exceptions",thread-groups=["i1"],times="0",original-location="__gnat_debug_raise_exception"}
As a result, GUIs running GDB in GDB/MI mode, which is the prefered
mode for them, as it provides easily parsable output, will no longer
have part of the information we'd like to show users.
This is caught by a couple of testcases that we have in gdb.ada.
Did you remember to run the testsuite before you submitted
this patch?
> }
> else
> - uiout->field_string ("what", "all Ada exceptions");
> + uiout->text (_("all Ada exceptions"));
Same as above.
>
> break;
>
> case ada_catch_exception_unhandled:
> - uiout->field_string ("what", "unhandled Ada exceptions");
> + uiout->text (_("unhandled Ada exceptions"));
Same as above.
> break;
> -
> +
> + case ada_catch_handle:
> + if (c->excep_string != NULL)
> + {
> + char *info = xstrprintf (_("`%s' Ada exception hanlder"), c->excep_string);
This line is too long.
> + struct cleanup *old_chain = make_cleanup (xfree, info);
Let's be consistent with the above, and just do an xfree directly
right after the call to uiout->field_string.
> +
> + uiout->text (info);
Same as above.
> + do_cleanups (old_chain);
> + }
> + else
> + uiout->text (_("all Ada exceptions handler"));
Same as above.
> + break;
> +
> case ada_catch_assert:
> - uiout->field_string ("what", "failed Ada assertions");
> + uiout->text (_("failed Ada assertions"));
Same as above.
> break;
>
> default:
> @@ -12614,7 +12642,21 @@ print_mention_exception (enum ada_exception_catchpoint_kind ex,
> case ada_catch_exception_unhandled:
> uiout->text (_("unhandled Ada exceptions"));
> break;
> -
> +
> + case ada_catch_handle:
> + if (c->excep_string != NULL)
> + {
> + char *info = xstrprintf (_("`%s' Ada exception handler"),
> + c->excep_string);
> + struct cleanup *old_chain = make_cleanup (xfree, info);
> +
> + uiout->text (info);
> + do_cleanups (old_chain);
> + }
> + else
> + uiout->text (_("all Ada exceptions handler"));
> + break;
> +
> case ada_catch_assert:
> uiout->text (_("failed Ada assertions"));
> break;
> @@ -12646,6 +12688,10 @@ print_recreate_exception (enum ada_exception_catchpoint_kind ex,
> fprintf_filtered (fp, "catch exception unhandled");
> break;
>
> + case ada_catch_handle:
> + fprintf_filtered (fp, "catch handle");
> + break;
> +
> case ada_catch_assert:
> fprintf_filtered (fp, "catch assert");
> break;
> @@ -12796,6 +12842,54 @@ print_recreate_catch_assert (struct breakpoint *b, struct ui_file *fp)
>
> static struct breakpoint_ops catch_assert_breakpoint_ops;
>
> +/* Virtual table for "catch handle" breakpoints. */
> +
> +static struct bp_location *
> +allocate_location_catch_handle (struct breakpoint *self)
> +{
> + return allocate_location_exception (ada_catch_handle, self);
> +}
> +
> +static void
> +re_set_catch_handle (struct breakpoint *b)
> +{
> + re_set_exception (ada_catch_handle, b);
> +}
> +
> +static void
> +check_status_catch_handle (bpstat bs)
> +{
> + check_status_exception (ada_catch_handle, bs);
> +}
> +
> +static enum print_stop_action
> +print_it_catch_handle (bpstat bs)
> +{
> + return print_it_exception (ada_catch_handle, bs);
> +}
> +
> +static void
> +print_one_catch_handle (struct breakpoint *b,
> + struct bp_location **last_loc)
> +{
> + print_one_exception (ada_catch_handle, b, last_loc);
> +}
> +
> +static void
> +print_mention_catch_handle (struct breakpoint *b)
> +{
> + print_mention_exception (ada_catch_handle, b);
> +}
> +
> +static void
> +print_recreate_catch_handle (struct breakpoint *b,
> + struct ui_file *fp)
> +{
> + print_recreate_exception (ada_catch_handle, b, fp);
> +}
> +
> +static struct breakpoint_ops catch_handle_breakpoint_ops;
> +
> /* Return a newly allocated copy of the first space-separated token
> in ARGSP, and then adjust ARGSP to point immediately after that
> token.
> @@ -12834,6 +12928,8 @@ ada_get_next_arg (const char **argsp)
> Set EX to the appropriate catchpoint type.
> Set EXCEP_STRING to the name of the specific exception if
> specified by the user.
> + HANDLE: Nonzero if the arguments are for a "catch handle" command.
> + Zero otherwise.
> If a condition is found at the end of the arguments, the condition
> expression is stored in COND_STRING (memory must be deallocated
> after use). Otherwise COND_STRING is set to NULL. */
> @@ -12842,7 +12938,7 @@ static void
> catch_ada_exception_command_split (const char *args,
> enum ada_exception_catchpoint_kind *ex,
> char **excep_string,
> - char **cond_string)
> + char **cond_string, int handle)
Can you make the new parameter a "bool".
Also, to keep that parameter combined with the other "in" parameters,
let's move it either first, or second?
And finally, can you use a more descriptive name? Your name is not
bad, but I think something like either "is_catch_handle_cmd" or
"catch_handle_cmd_p" both show that this is about the catch handle
command, and that this is a predicate/condition.
> {
> struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
> char *exception_name;
> @@ -12885,7 +12981,13 @@ catch_ada_exception_command_split (const char *args,
>
> discard_cleanups (old_chain);
>
> - if (exception_name == NULL)
> + if (handle)
> + {
> + /* Catch handling of exceptions. */
> + *ex = ada_catch_handle;
> + *excep_string = exception_name;
> + }
> + else if (exception_name == NULL)
> {
> /* Catch all exceptions. */
> *ex = ada_catch_exception;
> @@ -12927,6 +13029,9 @@ ada_exception_sym_name (enum ada_exception_catchpoint_kind ex)
> case ada_catch_assert:
> return (data->exception_info->catch_assert_sym);
> break;
> + case ada_catch_handle:
> + return (data->exception_info->catch_handle_sym);
> + break;
> default:
> internal_error (__FILE__, __LINE__,
> _("unexpected catchpoint kind (%d)"), ex);
> @@ -12950,6 +13055,9 @@ ada_exception_breakpoint_ops (enum ada_exception_catchpoint_kind ex)
> case ada_catch_assert:
> return (&catch_assert_breakpoint_ops);
> break;
> + case ada_catch_handle:
> + return (&catch_handle_breakpoint_ops);
> + break;
> default:
> internal_error (__FILE__, __LINE__,
> _("unexpected catchpoint kind (%d)"), ex);
> @@ -12960,14 +13068,18 @@ ada_exception_breakpoint_ops (enum ada_exception_catchpoint_kind ex)
> being raised with the exception that the user wants to catch. This
> assumes that this condition is used when the inferior just triggered
> an exception catchpoint.
> + EX: the type of catchpoints used for catching Ada exceptions.
>
> The string returned is a newly allocated string that needs to be
> deallocated later. */
>
> static char *
> -ada_exception_catchpoint_cond_string (const char *excep_string)
> +ada_exception_catchpoint_cond_string (const char *excep_string,
> + enum ada_exception_catchpoint_kind ex)
> {
> - int i;
> + int i, is_standard_exc;
Looks like "is_standard_exc" may be used unitialized.
> + const char *actual_exc_expr;
> + char *ref_exc_expr;
>
> /* The standard exceptions are a special case. They are defined in
> runtime units that have been compiled without debugging info; if
> @@ -12988,15 +13100,32 @@ ada_exception_catchpoint_cond_string (const char *excep_string)
> breakpoint condition is to use its fully-qualified named:
> e.g. my_package.constraint_error. */
>
> + if (ex == ada_catch_handle)
> + /* For exception handler catchpoints, the condition string does
> + not use the same parameter as for the other exceptions. */
For the purpose of deciding whether to use curly braces on not ("{"
and "}", we consider comments like these the same as statements.
Hence, you now have a situation where you have more than 1 statement
in the "if" block, so you need to use them. Eg:
| if (i)
| {
| /* Return success. */
| return 0;
| }
(the above is a copy/paste of the Coding Standard page in GDB's wiki)
> + actual_exc_expr
> + = "long_integer (\
> +GNAT_GCC_exception_Access(gcc_exception).all.occurrence.id)";
A better way to split this, IMO, so as to avoid breaking the visual
indentation (which also "tricks" the "-p" switch of "diff"), is to
format as follow:
actual_exc_expr = ("long_integer (GNAT_GCC_exception_Access"
"(gcc_exception).all.occurrence.id)");
The parentheses are not strictly necessary, but help the code formatters
(including most editors who have (sometimes) simple-minded auto-format
features.
If you are wondering what's different between this case, and the case
where you provide the documentation for a new command (further down
in your patch): In the second case (the doc for the new command),
using the same string with '\' at the end and then going to the start
of the next line allows you to write the text in a way that makes it
visually easy to see how long each line is, so you can easily format
it for an 80 characters terminal.
> + else
> + actual_exc_expr = "long_integer (e)";
You placed the block of code computing ACTUAL_EXC_EXPR in between
the comment explaining why standard exceptions are special, and
the code that actually determines whether the exception we're
trying to catch is standard or not. Please move that code either
before or after :).
> +
> for (i = 0; i < sizeof (standard_exc) / sizeof (char *); i++)
> {
> if (strcmp (standard_exc [i], excep_string) == 0)
> {
> - return xstrprintf ("long_integer (e) = long_integer (&standard.%s)",
> - excep_string);
> + is_standard_exc = 1;
> + break;
> }
> }
> - return xstrprintf ("long_integer (e) = long_integer (&%s)", excep_string);
> +
> + if (is_standard_exc)
> + ref_exc_expr = xstrprintf ("long_integer (&standard.%s)", excep_string);
> + else
> + ref_exc_expr = xstrprintf ("long_integer (&%s)", excep_string);
> +
> + //xfree (ref_exc_expr);
Ahem... You need to free that memory. In that case, it looks like using
a temporary would allow you to fix that leak. Eg:
char *result = xstrprintf ("%s = %s", actual_exc_expr, ref_exc_expr);
xfree (ref_exc_expr);
return result;
Note that commented-out code should never be pushed without an
explanation.
> +
> + return xstrprintf ("%s = %s", actual_exc_expr, ref_exc_expr);
> }
>
> /* Return the symtab_and_line that should be used to insert an exception
> @@ -13079,7 +13208,7 @@ create_ada_exception_catchpoint (struct gdbarch *gdbarch,
> init_ada_exception_breakpoint (c.get (), gdbarch, sal, addr_string,
> ops, tempflag, disabled, from_tty);
> c->excep_string = excep_string;
> - create_excep_cond_exprs (c.get ());
> + create_excep_cond_exprs (c.get (), ex_kind);
> if (cond_string != NULL)
> set_breakpoint_condition (c.get (), cond_string, from_tty);
> install_breakpoint (0, std::move (c), 1);
> @@ -13103,7 +13232,32 @@ catch_ada_exception_command (const char *arg_entry, int from_tty,
> if (!arg)
> arg = "";
> catch_ada_exception_command_split (arg, &ex_kind, &excep_string,
> - &cond_string);
> + &cond_string, 0);
> + create_ada_exception_catchpoint (gdbarch, ex_kind,
> + excep_string, cond_string,
> + tempflag, 1 /* enabled */,
> + from_tty);
> +}
> +
> +/* Implement the "catch handle" command. */
> +
> +static void
> +catch_ada_handle_command (const char *arg_entry, int from_tty,
> + struct cmd_list_element *command)
> +{
> + const char *arg = arg_entry;
> + struct gdbarch *gdbarch = get_current_arch ();
> + int tempflag;
> + enum ada_exception_catchpoint_kind ex_kind;
> + char *excep_string = NULL;
> + char *cond_string = NULL;
> +
> + tempflag = get_cmd_context (command) == CATCH_TEMPORARY;
> +
> + if (!arg)
> + arg = "";
> + catch_ada_exception_command_split (arg, &ex_kind, &excep_string,
> + &cond_string, 1);
> create_ada_exception_catchpoint (gdbarch, ex_kind,
> excep_string, cond_string,
> tempflag, 1 /* enabled */,
> @@ -14217,6 +14371,16 @@ initialize_ada_catchpoint_ops (void)
> ops->print_one = print_one_catch_assert;
> ops->print_mention = print_mention_catch_assert;
> ops->print_recreate = print_recreate_catch_assert;
> +
> + ops = &catch_handle_breakpoint_ops;
> + *ops = bkpt_breakpoint_ops;
> + ops->allocate_location = allocate_location_catch_handle;
> + ops->re_set = re_set_catch_handle;
> + ops->check_status = check_status_catch_handle;
> + ops->print_it = print_it_catch_handle;
> + ops->print_one = print_one_catch_handle;
> + ops->print_mention = print_mention_catch_handle;
> + ops->print_recreate = print_recreate_catch_handle;
> }
>
> /* This module's 'new_objfile' observer. */
> @@ -14277,6 +14441,14 @@ With an argument, catch only exceptions with the given name."),
> NULL,
> CATCH_PERMANENT,
> CATCH_TEMPORARY);
> +
> + add_catch_command ("handle", _("\
> +Catch Ada exceptions, when handled.\n\
> +With an argument, catch only exceptions with the given name."),
> + catch_ada_handle_command,
> + NULL,
> + CATCH_PERMANENT,
> + CATCH_TEMPORARY);
> add_catch_command ("assert", _("\
> Catch failed Ada assertions, when raised.\n\
> With an argument, catch only exceptions with the given name."),
> diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
> index 0530e9a..4e9850e 100644
> --- a/gdb/ada-lang.h
> +++ b/gdb/ada-lang.h
> @@ -108,7 +108,8 @@ enum ada_exception_catchpoint_kind
> {
> ada_catch_exception,
> ada_catch_exception_unhandled,
> - ada_catch_assert
> + ada_catch_assert,
> + ada_catch_handle
> };
>
> /* Ada task structures. */
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 062f01d..85e71e5 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -4453,6 +4453,25 @@ called @code{Constraint_Error} is defined in package @code{Pck}, then
> the command to use to catch such exceptions is @kbd{catch exception
> Pck.Constraint_Error}.
>
> +@item handle
> +@kindex catch handle
> +@cindex Ada exception handle catching
> +@cindex catch Ada exceptions when handled
> +An Ada exception being handled. If an exception name is
> +specified at the end of the command (eg @code{catch exception Program_Error}),
> +the debugger will stop only when this specific exception is handled.
> +Otherwise, the debugger stops execution when any Ada exception is handled.
> +
> +When inserting an handle catchpoint on a user-defined
> +exception whose name is identical to one of the exceptions
> +defined by the language, the fully qualified name must be used
> +as the exception name. Otherwise, @value{GDBN} will assume that it
> +should stop on the pre-defined exception rather than the
> +user-defined one. For instance, assuming an exception called
> + @code{Constraint_Error} is defined in package @code{Pck}, then the
> +command to use to catch such exceptions handling is
> +@kbd{catch handle Pck.Constraint_Error}.
> +
> @item exception unhandled
> @kindex catch exception unhandled
> An exception that was raised but is not handled by the program.
> diff --git a/gdb/testsuite/gdb.ada/excep_handle.exp b/gdb/testsuite/gdb.ada/excep_handle.exp
> new file mode 100644
> index 0000000..ec63d98
> --- /dev/null
> +++ b/gdb/testsuite/gdb.ada/excep_handle.exp
> @@ -0,0 +1,159 @@
> +# Copyright 2017 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +load_lib "ada.exp"
> +
> +standard_ada_testfile foo
> +
> +if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug additional_flags=-gnata ]] != "" } {
> + return -1
> +}
> +
> +clean_restart ${testfile}
> +
> +# Some global variables used to simplify the maintenance of some of
> +# the regular expressions below.
> +set eol "\[\r\n\]+"
> +set sp "\[ \t\]*"
> +
> +set when "when"
> +set catchpoint_constraint_error_msg \
> + "Catchpoint $decimal, exception at $hex in foo \\\(\\\).*at .*foo.adb:$decimal$eol$decimal$sp$when Constraint_Error =>"
> +
> +set catchpoint_program_error_msg \
> + "Catchpoint $decimal, exception at $hex in foo \\\(\\\).*at .*foo.adb:$decimal$eol$decimal$sp$when Program_Error =>"
> +
> +set catchpoint_storage_error_msg \
> + "Catchpoint $decimal, exception at $hex in foo \\\(\\\).*at .*foo.adb:$decimal$eol$decimal$sp$when Storage_Error =>"
> +
> +############################################
> +# Check that runtime supports catchpoint. #
> +############################################
> +
> +if ![runto_main] then {
> + fail "Cannot run to main, testcase aborted"
> + return 0
> +}
> +
> +set msg "insert catchpoint on all Ada exceptions handle"
> +gdb_test_multiple "catch handle" $msg {
> +-re "Catchpoint $decimal: all Ada exceptions handler$eol$gdb_prompt $" {
> + pass $msg
> + }
> + -re "Your Ada runtime appears to be missing some debugging information.*$eol$gdb_prompt $" {
> + # If the runtime was not built with enough debug information,
> + # or if it was stripped, we can not test exception handler
> + # catchpoints.
> + unsupported $msg
> + return -1
> + }
> +}
> +
> +############################################
> +# 1. Try catching all exceptions handler. #
> +############################################
> +
> +# Continue. The program should stop at first exception handling.
> +
> +gdb_test "continue" \
> + "Continuing\.$eol$catchpoint_constraint_error_msg$eol.*" \
> + "continuing to Constraint_Error exception handle"
> +
> +# Check that when no exception is raised, the program does not
> +# stop in the exception handling.
I must have understood this comment at some point, but I don't
anymore. In particular, you say "does not stop in the exception
handling", but then we test that we stop at a handler. So I am
confused, now.
> +
> +gdb_test "continue" \
> + "Continuing\.$eol$catchpoint_storage_error_msg$eol.*" \
> + "continuing without stopping to Storage_Error exception handle"
> +
> +gdb_test_no_output "delete 2" "disable catchpoint 1"
You have to admit that "delete *2*" next to "catchpoint *1*"
can be a little confusing too ;-). Howzabout you say "catchpoint
on all Ada exceptions handlers".
Note that one thing you can do to avoid having to keep track of
breakpoint/catchpoints numbers is to use "server delete" to delete
all breakpoints/catchpoints (the "server" command prefix just
tells GDB to not ask for confirmation - among other things).
> +
> +##################################################
> +# 2. Try catching some named exception handler. #
> +##################################################
> +
> +# Insert Program_Error Ada exception handler
This is confusing. It sounds like you are inserting a handler,
which is not the case. Try:
# Insert a catchpoint on Program_Error Ada exception handlers.
(Remember that sentences end with a period)
> +
> +gdb_test "catch handle Program_Error" \
> + "Catchpoint $decimal: `Program_Error' Ada exception handler" \
> + "insert catchpoint on Program_Error Ada exception handler"
> +
> +# Continue, we should not stop at ABORT_SIGNAL but as Program_Error one
as -> at
> +
> +gdb_test "continue" \
> + "Continuing\.$eol$catchpoint_program_error_msg$eol.*" \
> + "continuing without stopping to Program_Error exception handle"
> +
> +gdb_test_no_output "delete 3" "disable catchpoint 2"
Same as above. Use a description of the catchpoint instead of its
number.
> +# Insert Storage_Error Ada exception handler
Same as above.
> +gdb_test "catch handle Storage_Error" \
> + "Catchpoint $decimal: `Storage_Error' Ada exception handler" \
> + "insert catchpoint on Storage_Error Ada exception handler"
> +
> +# Continue, we should stop at Storage_Error handler
> +
> +gdb_test "continue" \
> + "Continuing\.$eol$catchpoint_storage_error_msg$eol.*" \
> + "continuing without stopping to Storage_Error exception handle"
> +
> +gdb_test_no_output "delete 4" "disable catchpoint 3"
Same as above.
> +########################################################################
> +# 3. Try catching with condition and without named exception handler. #
> +########################################################################
> +
> +# Insert Ada all exceptions handler with condition
# Insert catchpoint on all Ada exception handlers with a condition.
> +gdb_test "catch handle if Global_Var = 2" \
> + "Catchpoint $decimal: all Ada exceptions handler" \
> + "insert catchpoint on all Ada exception handler with condition"
handler -> handlers
> +# Check that condition is stored and properly displayed
> +
> +gdb_test "info breakpoint" ".*stop only if Global_Var = 2" \
I believe the ".*" at the start of your expected output is unnecessary.
> + "Check catch handle with condition"
> +
> +# Continue, we should not stop at ABORT_SIGNAL but as Program_Error one
as -> at
> +
> +gdb_test "continue" \
> + "Continuing\.$eol$catchpoint_constraint_error_msg$eol.*" \
> + "continuing to Constraint_Error exception handle"
You're using the same test message for multiple test. Please review
the test names so that the same name isn't used more than once.
The best way to do that, IMO, is to write a one-liner in of shell
commands that extract the test names from gdb.sum, sorts them, and
then prints how many of each there are. Here's one way, which, for
good measure, even sorts by highest number of occurrences first,
and also eliminates test names that are only used once:
# Just run your testcase so your gdb.sum only has the results for
# your testcase. Otherwise, just grep the gdb.sum file with
# your testcase's name.
$ make check RUNTESTFLAGS='excep_handle.exp'
And then use the following command:
$ grep '^[A-Z]\+:' gdb.sum | cut -d: -f3- | sort | uniq -c | sort -n -r | grep -v '^\s\+1\s'
2 continuing without stopping to Storage_Error exception handle
2 continuing to Constraint_Error exception handle
Please fix so there is no longer any output.
> +gdb_test_no_output "delete 5" "disable catchpoint 4"
Same as above.
> +################################################################
> +# 4. Try catching with condition and named exception handler. #
> +################################################################
> +
> +# Insert Program_Error Ada exception handler with condition
# Insert a catchpoint on Program_Error Ada exception handlers with condition
> +gdb_test "catch handle Program_Error if Global_Var = 4" \
> + "Catchpoint $decimal: `Program_Error' Ada exception handler" \
> + "insert catchpoint on Program_Error Ada exception handler with condition"
> +
> +# Continue, we should not stop at first Program_Error handler but at
> +# the second one.
> +
> +gdb_test "continue" \
> + "Continuing\.$eol$catchpoint_program_error_msg$eol.*" \
> + "continuing to Program_Error exception handle"
> +
> +# Continue, the program should exit properly.
> +
> +gdb_test "continue" \
> + "Continuing\..*$inferior_exited_re.*" \
> + "continuing to program completion"
> diff --git a/gdb/testsuite/gdb.ada/excep_handle/foo.adb b/gdb/testsuite/gdb.ada/excep_handle/foo.adb
> new file mode 100644
> index 0000000..887e339
> --- /dev/null
> +++ b/gdb/testsuite/gdb.ada/excep_handle/foo.adb
> @@ -0,0 +1,103 @@
> +-- Copyright 2017 Free Software Foundation, Inc.
> +--
> +-- This program is free software; you can redistribute it and/or modify
> +-- it under the terms of the GNU General Public License as published by
> +-- the Free Software Foundation; either version 3 of the License, or
> +-- (at your option) any later version.
> +--
> +-- This program is distributed in the hope that it will be useful,
> +-- but WITHOUT ANY WARRANTY; without even the implied warranty of
> +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +-- GNU General Public License for more details.
> +--
> +-- You should have received a copy of the GNU General Public License
> +-- along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +with Pck; use Pck;
> +
> +procedure Foo is
> +begin
> +
> + -- Part 1 of the test
test -> testcase
(please fix the other parts as well).
No further comments beyond this point.
> +
> + begin
> + raise Constraint_Error;
> + exception
> + when Constraint_Error =>
> + null;
> + end;
> +
> + begin
> + null;
> + exception
> + when others =>
> + null;
> + end;
> +
> + begin
> + raise Storage_Error;
> + exception
> + when Storage_Error =>
> + null;
> + end;
> +
> + -- Part 2 of the test
> +
> + begin
> + raise ABORT_SIGNAL;
> + exception
> + when others =>
> + null;
> + end;
> +
> + begin
> + raise Program_Error;
> + exception
> + when Program_Error =>
> + null;
> + end;
> +
> + begin
> + raise Storage_Error;
> + exception
> + when Storage_Error =>
> + null;
> + end;
> +
> + -- Part 3 of the test
> +
> + begin
> + Global_Var := Global_Var + 1;
> + raise ABORT_SIGNAL;
> + exception
> + when others =>
> + null;
> + end;
> +
> + begin
> + Global_Var := Global_Var + 1;
> + raise Constraint_Error;
> + exception
> + when Constraint_Error =>
> + null;
> + end;
> +
> + -- Part 4 of the test
> +
> + begin
> + Global_Var := Global_Var + 1;
> + raise Program_Error;
> + exception
> + when others =>
> + null;
> + end;
> +
> + begin
> + Global_Var := Global_Var + 1;
> + raise Program_Error;
> + exception
> + when Program_Error =>
> + null;
> + end;
> +
> +end Foo;
> diff --git a/gdb/testsuite/gdb.ada/excep_handle/pck.ads b/gdb/testsuite/gdb.ada/excep_handle/pck.ads
> new file mode 100644
> index 0000000..1b438fc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.ada/excep_handle/pck.ads
> @@ -0,0 +1,19 @@
> +-- Copyright 2017 Free Software Foundation, Inc.
> +--
> +-- This program is free software; you can redistribute it and/or modify
> +-- it under the terms of the GNU General Public License as published by
> +-- the Free Software Foundation; either version 3 of the License, or
> +-- (at your option) any later version.
> +--
> +-- This program is distributed in the hope that it will be useful,
> +-- but WITHOUT ANY WARRANTY; without even the implied warranty of
> +-- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +-- GNU General Public License for more details.
> +--
> +-- You should have received a copy of the GNU General Public License
> +-- along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +package Pck is
> + Global_Var : Integer := 0;
> + ABORT_SIGNAL : exception;
> +end Pck;
> --
> 2.7.4
--
Joel