[RFA,5/6] Change Ada exceptions to use std::string
Commit Message
[really attached, this time!]
> > This changes the Ada exception code to use std::string, allowing the
> > removal of some more cleanups.
> >
> > ChangeLog
> > 2017-11-29 Tom Tromey <tom@tromey.com>
> >
> > * mi/mi-cmd-catch.c (mi_cmd_catch_assert)
> > (mi_cmd_catch_exception): Update.
> > * ada-lang.h (create_ada_exception_catchpoint): Update.
> > * ada-lang.c (struct ada_catchpoint) <excep_string>: Now a
> > std::string.
> > (create_excep_cond_exprs, ~ada_catchpoint)
> > (should_stop_exception, print_one_exception)
> > (print_mention_exception, print_recreate_exception): Update.
> > (ada_get_next_arg): Remove.
> > (catch_ada_exception_command_split): Change two arguments to
> > "std::string *". Remove cleanups.
> > (ada_exception_catchpoint_cond_string): Change "string" to
> > std::string.
> > (ada_exception_sal): Remove excep_string parameter.
> > (create_ada_exception_catchpoint): Change two arguments to
> > "std::string &&".
> > (catch_ada_exception_command): Update.
>
> As hinted in my answer to the cover email, I found a few issues.
> Attached is a commit that explains and fixes them. If the fixes
> look good to you, a new commit combining the two is preapproved.
>
> (I tested my patch on x86_64-linux, limiting myself to gdb.ada)
Comments
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
>> (I tested my patch on x86_64-linux, limiting myself to gdb.ada)
Joel> Pb #1: Crash while trying to insert an assert catchpoint:
I wonder why I didn't see any problems from the buildbot.
Joel> Pb #3: condition handling in exception catchpoints is broken
Joel> Tom removed a call to "skip_spaces" before checking for the next
Joel> argument, and I am not sure I understand why.
Here I was confused and thought extract_arg skipped spaces again after
extracting the arg. But, it doesn't.
Still - why not test failures? This is disturbing.
Tom
> Joel> Pb #1: Crash while trying to insert an assert catchpoint:
>
> I wonder why I didn't see any problems from the buildbot.
>
> Joel> Pb #3: condition handling in exception catchpoints is broken
>
> Joel> Tom removed a call to "skip_spaces" before checking for the next
> Joel> argument, and I am not sure I understand why.
>
> Here I was confused and thought extract_arg skipped spaces again after
> extracting the arg. But, it doesn't.
>
> Still - why not test failures? This is disturbing.
That's indeed strange. Do we have access to the reference report?
Maybe the Ada testing is not run (eg: missing Ada compiler on
the buildbot), or maybe the Ada compiler there is such that the tests
were failing in the first place, preventing it from detecting
regressions?
In any case, I'm quite happy to be the safety net in this case...
From c2ad16ea37be009bf58afb7467f2bdd67b31710a Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 30 Nov 2017 16:15:09 -0500
Subject: [PATCH] Fix a couple of regressions introduced by the previous patch
Pb #1: Crash while trying to insert an assert catchpoint:
With any program built with "gnatmake -g -gnata ...", trying
to insert a catchpoint on failed assertions would cause an internal
error:
(gdb) catch assert
/[...]/common/cleanups.c:264: internal-warning: restore_my_cleanups has found a stale cleanup
I tracked this down to an issue in the call to
create_ada_exception_catchpoint, where we pass NULL for at least
one of the parameters declared as a "std::string &&". I don't know
C++ well enough, but instruction-by-instruction leads me to believe
that the constructor doesn't like NULL as a char *, so we get an
exception, and that exception eventually somehow leads to the
cleanup error above (not sure how, unfortunately). The fix was
to pass an std::string() instead.
And by the time I understood this for the cond string parameter
(on which I had target fixation), I had C++-ifed
catch_ada_assert_command_split.
Pb #2: Same kind of crash insertin an exception in GDB/MI mode.
In this case, it was just a case of getting confused between
a couple of temporary "char *" variables, and the corresponding
std::string parameters that Tom introduced.
I first fixed the confusion, but then I wondered why using
intermediate "char *" variables at all. So I changed this function
to only use std::string variables.
Pb #3: condition handling in exception catchpoints is broken
(gdb) catch exception constraint_error if True
Junk at end of expression
Tom removed a call to "skip_spaces" before checking for the next
argument, and I am not sure I understand why. But debugging
the problem showed by arg was equal to...
"constraint_error if True"
... and that once extract_arg is called, args is now equal to...
" if True"
... so the condition identifying an "if" block no longer works.
I restored the call to skip_spaces.
---
gdb/ada-lang.c | 9 +++++----
gdb/mi/mi-cmd-catch.c | 12 +++---------
2 files changed, 8 insertions(+), 13 deletions(-)
@@ -12744,6 +12744,7 @@ catch_ada_exception_command_split (const char *args,
/* Check to see if we have a condition. */
+ args = skip_spaces (args);
if (startswith (args, "if")
&& (isspace (args[2]) || args[2] == '\0'))
{
@@ -12991,7 +12992,7 @@ catch_ada_exception_command (const char *arg_entry, int from_tty,
(the memory needs to be deallocated after use). */
static void
-catch_ada_assert_command_split (const char *args, char **cond_string)
+catch_ada_assert_command_split (const char *args, std::string *cond_string)
{
args = skip_spaces (args);
@@ -13003,7 +13004,7 @@ catch_ada_assert_command_split (const char *args, char **cond_string)
args = skip_spaces (args);
if (args[0] == '\0')
error (_("condition missing after `if' keyword"));
- *cond_string = xstrdup (args);
+ *cond_string = args;
}
/* Otherwise, there should be no other argument at the end of
@@ -13021,7 +13022,7 @@ catch_assert_command (const char *arg_entry, int from_tty,
const char *arg = arg_entry;
struct gdbarch *gdbarch = get_current_arch ();
int tempflag;
- char *cond_string = NULL;
+ std::string cond_string;
tempflag = get_cmd_context (command) == CATCH_TEMPORARY;
@@ -13029,7 +13030,7 @@ catch_assert_command (const char *arg_entry, int from_tty,
arg = "";
catch_ada_assert_command_split (arg, &cond_string);
create_ada_exception_catchpoint (gdbarch, ada_catch_assert,
- NULL, cond_string,
+ std::string (), std::move (cond_string),
tempflag, 1 /* enabled */,
from_tty);
}
@@ -93,9 +93,9 @@ void
mi_cmd_catch_exception (const char *cmd, char *argv[], int argc)
{
struct gdbarch *gdbarch = get_current_arch();
- char *condition = NULL;
+ std::string condition;
int enabled = 1;
- char *exception_name = NULL;
+ std::string exception_name;
int temp = 0;
enum ada_exception_catchpoint_kind ex_kind = ada_catch_exception;
@@ -152,16 +152,10 @@ mi_cmd_catch_exception (const char *cmd, char *argv[], int argc)
/* Specifying an exception name does not make sense when requesting
an unhandled exception breakpoint. */
- if (ex_kind == ada_catch_exception_unhandled && exception_name != NULL)
+ if (ex_kind == ada_catch_exception_unhandled && ! exception_name.empty())
error (_("\"-e\" and \"-u\" are mutually exclusive"));
scoped_restore restore_breakpoint_reporting = setup_breakpoint_reporting ();
- std::string except_str;
- if (exception_name != NULL)
- except_str = exception_name;
- std::string cond_str;
- if (condition != NULL)
- cond_str = condition;
create_ada_exception_catchpoint (gdbarch, ex_kind,
std::move (exception_name),
std::move (condition),
--
2.1.4