Re: [RFA 5/6] Change Ada exceptions to use std::string

[Joel Brobecker <brobecker@adacore.com>]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

[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)


-- 
Joel

[-- Attachment #2: 0001-Fix-a-couple-of-regressions-introduced-by-the-previo.patch --]
[-- Type: text/x-diff, Size: 5373 bytes --]

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

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 5e9aebf..1caa3dc 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -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);
 }
diff --git a/gdb/mi/mi-cmd-catch.c b/gdb/mi/mi-cmd-catch.c
index f69cdaa..70939ee 100644
--- a/gdb/mi/mi-cmd-catch.c
+++ b/gdb/mi/mi-cmd-catch.c
@@ -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