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

login
register
mail settings
Submitter Joel Brobecker
Date Nov. 30, 2017, 10:59 p.m.
Message ID <20171130225916.csgxhw2gcauyby6x@adacore.com>
Download mbox | patch
Permalink /patch/24648/
State New
Headers show

Comments

Joel Brobecker - Nov. 30, 2017, 10:59 p.m.
[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)
Tom Tromey - Nov. 30, 2017, 11:53 p.m.
>>>>> "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 Brobecker - Dec. 1, 2017, 12:10 p.m.
> 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...

Patch

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