[RFA 2/3] Remove cleanup from ada_collect_symbol_completion_matches

[RFA 2/3] Remove cleanup from ada_collect_symbol_completion_matches

[Tom Tromey]

ada_collect_symbol_completion_matches installs a null_cleanup but not
any other cleanups.  This patch removes it.

gdb/ChangeLog
2018-05-18  Tom Tromey  <tom@tromey.com>

	* ada-lang.c (ada_collect_symbol_completion_matches): Remove
	cleanup.
---
 gdb/ChangeLog  | 5 +++++
 gdb/ada-lang.c | 3 ---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index da5f75030c..cbb0fe9d82 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -6445,7 +6445,6 @@ ada_collect_symbol_completion_matches (completion_tracker &tracker,
   struct objfile *objfile;
   const struct block *b, *surrounding_static_block = 0;
   struct block_iterator iter;
-  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
 
   gdb_assert (code == TYPE_CODE_UNDEF);
 
@@ -6550,8 +6549,6 @@ ada_collect_symbol_completion_matches (completion_tracker &tracker,
 				lookup_name, text, word);
     }
   }
-
-  do_cleanups (old_chain);
 }
 
                                 /* Field Access */
-- 
2.13.6

[RFA 3/3] Change ada_catchpoint::excep_string to be a std::string

[Tom Tromey]

This changes ada_catchpoint::excep_string to be a std::string and then
fixes up all t he users.

This found a memory leak in catch_ada_exception_command_split, where
"cond" was copied but never freed.

I changed the type of the "cond_string" argument to
catch_ada_exception_command_split to follow the rule that out
parameters should be pointers and not references.

This patch enables the removal of some cleanups and also the function
ada_get_next_arg.

gdb/ChangeLog
2018-05-18  Tom Tromey  <tom@tromey.com>

	* mi/mi-cmd-catch.c (mi_cmd_catch_assert)
	(mi_cmd_catch_exception, mi_cmd_catch_handlers): 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): Use std::string.  Change type
	of "excep_string", "cond_string".
	(catch_ada_exception_command): Update.
	(create_ada_exception_catchpoint): Change type of excep_string.
	(ada_exception_sal): Remove excep_string parameter.
---
 gdb/ChangeLog         |  17 +++++++
 gdb/ada-lang.c        | 122 +++++++++++++++-----------------------------------
 gdb/ada-lang.h        |   2 +-
 gdb/mi/mi-cmd-catch.c |  18 +++-----
 4 files changed, 59 insertions(+), 100 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index cbb0fe9d82..159c1a0da2 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12480,7 +12480,7 @@ struct ada_catchpoint : public breakpoint
   ~ada_catchpoint () override;
 
   /* The name of the specific exception the user specified.  */
-  char *excep_string;
+  std::string excep_string;
 };
 
 /* Parse the exception condition string in the context of each of the
@@ -12493,7 +12493,7 @@ create_excep_cond_exprs (struct ada_catchpoint *c,
   struct bp_location *bl;
 
   /* Nothing to do if there's no specific exception to catch.  */
-  if (c->excep_string == NULL)
+  if (c->excep_string.empty ())
     return;
 
   /* Same if there are no locations... */
@@ -12503,7 +12503,7 @@ create_excep_cond_exprs (struct ada_catchpoint *c,
   /* Compute the condition expression in text form, from the specific
      expection we want to catch.  */
   std::string cond_string
-    = ada_exception_catchpoint_cond_string (c->excep_string, ex);
+    = ada_exception_catchpoint_cond_string (c->excep_string.c_str (), ex);
 
   /* Iterate over all the catchpoint's locations, and parse an
      expression for each.  */
@@ -12541,7 +12541,6 @@ create_excep_cond_exprs (struct ada_catchpoint *c,
 
 ada_catchpoint::~ada_catchpoint ()
 {
-  xfree (this->excep_string);
 }
 
 /* Implement the ALLOCATE_LOCATION method in the breakpoint_ops
@@ -12584,7 +12583,7 @@ should_stop_exception (const struct bp_location *bl)
   int stop;
 
   /* With no specific exception, should always stop.  */
-  if (c->excep_string == NULL)
+  if (c->excep_string.empty ())
     return 1;
 
   if (ada_loc->excep_cond_expr == NULL)
@@ -12734,12 +12733,12 @@ print_one_exception (enum ada_exception_catchpoint_kind ex,
   switch (ex)
     {
       case ada_catch_exception:
-        if (c->excep_string != NULL)
+        if (!c->excep_string.empty ())
           {
-            char *msg = xstrprintf (_("`%s' Ada exception"), c->excep_string);
+	    std::string msg = string_printf (_("`%s' Ada exception"),
+					     c->excep_string.c_str ());
 
             uiout->field_string ("what", msg);
-            xfree (msg);
           }
         else
           uiout->field_string ("what", "all Ada exceptions");
@@ -12751,11 +12750,11 @@ print_one_exception (enum ada_exception_catchpoint_kind ex,
         break;
       
       case ada_catch_handlers:
-        if (c->excep_string != NULL)
+        if (!c->excep_string.empty ())
           {
 	    uiout->field_fmt ("what",
 			      _("`%s' Ada exception handlers"),
-			      c->excep_string);
+			      c->excep_string.c_str ());
           }
         else
 	  uiout->field_string ("what", "all Ada exceptions handlers");
@@ -12789,10 +12788,10 @@ print_mention_exception (enum ada_exception_catchpoint_kind ex,
   switch (ex)
     {
       case ada_catch_exception:
-        if (c->excep_string != NULL)
+        if (!c->excep_string.empty ())
 	  {
 	    std::string info = string_printf (_("`%s' Ada exception"),
-					      c->excep_string);
+					      c->excep_string.c_str ());
 	    uiout->text (info.c_str ());
 	  }
         else
@@ -12804,11 +12803,11 @@ print_mention_exception (enum ada_exception_catchpoint_kind ex,
         break;
 
       case ada_catch_handlers:
-        if (c->excep_string != NULL)
+        if (!c->excep_string.empty ())
 	  {
 	    std::string info
 	      = string_printf (_("`%s' Ada exception handlers"),
-			       c->excep_string);
+			       c->excep_string.c_str ());
 	    uiout->text (info.c_str ());
 	  }
         else
@@ -12838,8 +12837,8 @@ print_recreate_exception (enum ada_exception_catchpoint_kind ex,
     {
       case ada_catch_exception:
 	fprintf_filtered (fp, "catch exception");
-	if (c->excep_string != NULL)
-	  fprintf_filtered (fp, " %s", c->excep_string);
+	if (!c->excep_string.empty ())
+	  fprintf_filtered (fp, " %s", c->excep_string.c_str ());
 	break;
 
       case ada_catch_exception_unhandled:
@@ -13048,40 +13047,6 @@ print_recreate_catch_handlers (struct breakpoint *b,
 
 static struct breakpoint_ops catch_handlers_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.
-
-   Return NULL if ARGPS does not contain any more tokens.  */
-
-static char *
-ada_get_next_arg (const char **argsp)
-{
-  const char *args = *argsp;
-  const char *end;
-  char *result;
-
-  args = skip_spaces (args);
-  if (args[0] == '\0')
-    return NULL; /* No more arguments.  */
-  
-  /* Find the end of the current argument.  */
-
-  end = skip_to_space (args);
-
-  /* Adjust ARGSP to point to the start of the next argument.  */
-
-  *argsp = end;
-
-  /* Make a copy of the current argument and return it.  */
-
-  result = (char *) xmalloc (end - args + 1);
-  strncpy (result, args, end - args);
-  result[end - args] = '\0';
-  
-  return result;
-}
-
 /* Split the arguments specified in a "catch exception" command.  
    Set EX to the appropriate catchpoint type.
    Set EXCEP_STRING to the name of the specific exception if
@@ -13096,24 +13061,20 @@ static void
 catch_ada_exception_command_split (const char *args,
 				   bool is_catch_handlers_cmd,
                                    enum ada_exception_catchpoint_kind *ex,
-				   char **excep_string,
-				   std::string &cond_string)
+				   std::string *excep_string,
+				   std::string *cond_string)
 {
-  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
-  char *exception_name;
-  char *cond = NULL;
+  std::string exception_name;
 
-  exception_name = ada_get_next_arg (&args);
-  if (exception_name != NULL && strcmp (exception_name, "if") == 0)
+  exception_name = extract_arg (&args);
+  if (exception_name == "if")
     {
       /* This is not an exception name; this is the start of a condition
 	 expression for a catchpoint on all exceptions.  So, "un-get"
 	 this token, and set exception_name to NULL.  */
-      xfree (exception_name);
-      exception_name = NULL;
+      exception_name.clear ();
       args -= 2;
     }
-  make_cleanup (xfree, exception_name);
 
   /* Check to see if we have a condition.  */
 
@@ -13126,8 +13087,7 @@ catch_ada_exception_command_split (const char *args,
 
       if (args[0] == '\0')
         error (_("Condition missing after `if' keyword"));
-      cond = xstrdup (args);
-      make_cleanup (xfree, cond);
+      *cond_string = args;
 
       args += strlen (args);
     }
@@ -13138,25 +13098,23 @@ catch_ada_exception_command_split (const char *args,
   if (args[0] != '\0')
     error (_("Junk at end of expression"));
 
-  discard_cleanups (old_chain);
-
   if (is_catch_handlers_cmd)
     {
       /* Catch handling of exceptions.  */
       *ex = ada_catch_handlers;
       *excep_string = exception_name;
     }
-  else if (exception_name == NULL)
+  else if (exception_name.empty ())
     {
       /* Catch all exceptions.  */
       *ex = ada_catch_exception;
-      *excep_string = NULL;
+      excep_string->clear ();
     }
-  else if (strcmp (exception_name, "unhandled") == 0)
+  else if (exception_name == "unhandled")
     {
       /* Catch unhandled exceptions.  */
       *ex = ada_catch_exception_unhandled;
-      *excep_string = NULL;
+      excep_string->clear ();
     }
   else
     {
@@ -13164,8 +13122,6 @@ catch_ada_exception_command_split (const char *args,
       *ex = ada_catch_exception;
       *excep_string = exception_name;
     }
-  if (cond != NULL)
-    cond_string.assign (cond);
 }
 
 /* Return the name of the symbol on which we should break in order to
@@ -13289,15 +13245,12 @@ ada_exception_catchpoint_cond_string (const char *excep_string,
 /* Return the symtab_and_line that should be used to insert an exception
    catchpoint of the TYPE kind.
 
-   EXCEP_STRING should contain the name of a specific exception that
-   the catchpoint should catch, or NULL otherwise.
-
    ADDR_STRING returns the name of the function where the real
    breakpoint that implements the catchpoints is set, depending on the
    type of catchpoint we need to create.  */
 
 static struct symtab_and_line
-ada_exception_sal (enum ada_exception_catchpoint_kind ex, char *excep_string,
+ada_exception_sal (enum ada_exception_catchpoint_kind ex,
 		   const char **addr_string, const struct breakpoint_ops **ops)
 {
   const char *sym_name;
@@ -13333,13 +13286,11 @@ ada_exception_sal (enum ada_exception_catchpoint_kind ex, char *excep_string,
 
    EX_KIND is the kind of exception catchpoint to be created.
 
-   If EXCEPT_STRING is NULL, this catchpoint is expected to trigger
+   If EXCEPT_STRING is empty, this catchpoint is expected to trigger
    for all exceptions.  Otherwise, EXCEPT_STRING indicates the name
-   of the exception to which this catchpoint applies.  When not NULL,
-   the string must be allocated on the heap, and its deallocation
-   is no longer the responsibility of the caller.
+   of the exception to which this catchpoint applies.
 
-   COND_STRING, if not NULL, is the catchpoint condition.  This string
+   COND_STRING, if not empty, is the catchpoint condition.  This string
    must be allocated on the heap, and its deallocation is no longer
    the responsibility of the caller.
 
@@ -13351,7 +13302,7 @@ ada_exception_sal (enum ada_exception_catchpoint_kind ex, char *excep_string,
 void
 create_ada_exception_catchpoint (struct gdbarch *gdbarch,
 				 enum ada_exception_catchpoint_kind ex_kind,
-				 char *excep_string,
+				 const std::string &excep_string,
 				 const std::string &cond_string,
 				 int tempflag,
 				 int disabled,
@@ -13359,8 +13310,7 @@ create_ada_exception_catchpoint (struct gdbarch *gdbarch,
 {
   const char *addr_string = NULL;
   const struct breakpoint_ops *ops = NULL;
-  struct symtab_and_line sal
-    = ada_exception_sal (ex_kind, excep_string, &addr_string, &ops);
+  struct symtab_and_line sal = ada_exception_sal (ex_kind, &addr_string, &ops);
 
   std::unique_ptr<ada_catchpoint> c (new ada_catchpoint ());
   init_ada_exception_breakpoint (c.get (), gdbarch, sal, addr_string,
@@ -13382,7 +13332,7 @@ catch_ada_exception_command (const char *arg_entry, int from_tty,
   struct gdbarch *gdbarch = get_current_arch ();
   int tempflag;
   enum ada_exception_catchpoint_kind ex_kind;
-  char *excep_string = NULL;
+  std::string excep_string;
   std::string cond_string;
 
   tempflag = get_cmd_context (command) == CATCH_TEMPORARY;
@@ -13390,7 +13340,7 @@ catch_ada_exception_command (const char *arg_entry, int from_tty,
   if (!arg)
     arg = "";
   catch_ada_exception_command_split (arg, false, &ex_kind, &excep_string,
-				     cond_string);
+				     &cond_string);
   create_ada_exception_catchpoint (gdbarch, ex_kind,
 				   excep_string, cond_string,
 				   tempflag, 1 /* enabled */,
@@ -13407,7 +13357,7 @@ catch_ada_handlers_command (const char *arg_entry, int from_tty,
   struct gdbarch *gdbarch = get_current_arch ();
   int tempflag;
   enum ada_exception_catchpoint_kind ex_kind;
-  char *excep_string = NULL;
+  std::string excep_string;
   std::string cond_string;
 
   tempflag = get_cmd_context (command) == CATCH_TEMPORARY;
@@ -13415,7 +13365,7 @@ catch_ada_handlers_command (const char *arg_entry, int from_tty,
   if (!arg)
     arg = "";
   catch_ada_exception_command_split (arg, true, &ex_kind, &excep_string,
-				     cond_string);
+				     &cond_string);
   create_ada_exception_catchpoint (gdbarch, ex_kind,
 				   excep_string, cond_string,
 				   tempflag, 1 /* enabled */,
diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h
index 09e7b403f2..a4192fc8a5 100644
--- a/gdb/ada-lang.h
+++ b/gdb/ada-lang.h
@@ -377,7 +377,7 @@ extern char *ada_main_name (void);
 
 extern void create_ada_exception_catchpoint
   (struct gdbarch *gdbarch, enum ada_exception_catchpoint_kind ex_kind,
-   char *excep_string, const std::string &cond_string, int tempflag,
+   const std::string &excep_string, const std::string &cond_string, int tempflag,
    int disabled, int from_tty);
 
 /* Some information about a given Ada exception.  */
diff --git a/gdb/mi/mi-cmd-catch.c b/gdb/mi/mi-cmd-catch.c
index 078e73ab6a..77c9f952f5 100644
--- a/gdb/mi/mi-cmd-catch.c
+++ b/gdb/mi/mi-cmd-catch.c
@@ -79,8 +79,8 @@ mi_cmd_catch_assert (const char *cmd, char *argv[], int argc)
     error (_("Invalid argument: %s"), argv[oind]);
 
   scoped_restore restore_breakpoint_reporting = setup_breakpoint_reporting ();
-  create_ada_exception_catchpoint (gdbarch, ada_catch_assert,
-				   NULL, condition, temp, enabled, 0);
+  create_ada_exception_catchpoint (gdbarch, ada_catch_assert, std::string (),
+				   condition, temp, enabled, 0);
 }
 
 /* Handler for the -catch-exception command.  */
@@ -91,7 +91,7 @@ mi_cmd_catch_exception (const char *cmd, char *argv[], int argc)
   struct gdbarch *gdbarch = get_current_arch();
   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;
 
@@ -148,14 +148,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 ();
-  /* create_ada_exception_catchpoint needs EXCEPTION_NAME to be
-     xstrdup'ed, and will assume control of its lifetime.  */
-  if (exception_name != NULL)
-    exception_name = xstrdup (exception_name);
   create_ada_exception_catchpoint (gdbarch, ex_kind,
 				   exception_name,
 				   condition, temp, enabled, 0);
@@ -169,7 +165,7 @@ mi_cmd_catch_handlers (const char *cmd, char *argv[], int argc)
   struct gdbarch *gdbarch = get_current_arch ();
   std::string condition;
   int enabled = 1;
-  char *exception_name = NULL;
+  std::string exception_name;
   int temp = 0;
 
   int oind = 0;
@@ -220,10 +216,6 @@ mi_cmd_catch_handlers (const char *cmd, char *argv[], int argc)
 
   scoped_restore restore_breakpoint_reporting
     = setup_breakpoint_reporting ();
-  /* create_ada_exception_catchpoint needs EXCEPTION_NAME to be
-     xstrdup'ed, and will assume control of its lifetime.  */
-  if (exception_name != NULL)
-    exception_name = xstrdup (exception_name);
   create_ada_exception_catchpoint (gdbarch, ada_catch_handlers,
 				   exception_name,
 				   condition, temp, enabled, 0);
-- 
2.13.6

[RFA 0/3] More Ada cleanup removal

[Tom Tromey]

This resurrects some of the patches from my old Ada cleanup removal
thread.

https://sourceware.org/ml/gdb-patches/2017-11/msg00812.html

Now, I believe patch #3 addresses the comments in

https://sourceware.org/ml/gdb-patches/2017-11/msg00853.html

Tested by the buildbot.

Tom

[RFA 1/3] Remove cleanup from ada-lang.c

[Tom Tromey]

This removes a cleanup from ada-lang.c by having
ada_exception_message_1 return a unique_xmalloc_ptr.

gdb/ChangeLog
2018-05-18  Tom Tromey  <tom@tromey.com>

	* ada-lang.c (ada_exception_message_1, ada_exception_message):
	Return unique_xmalloc_ptr.
	(print_it_exception): Update.
---
 gdb/ChangeLog  |  6 ++++++
 gdb/ada-lang.c | 29 +++++++++--------------------
 2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 7cbf1ec08d..da5f75030c 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -12342,8 +12342,6 @@ ada_exception_name_addr_1 (enum ada_exception_catchpoint_kind ex,
    return the message which was associated to the exception, if
    available.  Return NULL if the message could not be retrieved.
 
-   The caller must xfree the string after use.
-
    Note: The exception message can be associated to an exception
    either through the use of the Raise_Exception function, or
    more simply (Ada 2005 and later), via:
@@ -12352,13 +12350,11 @@ ada_exception_name_addr_1 (enum ada_exception_catchpoint_kind ex,
 
    */
 
-static char *
+static gdb::unique_xmalloc_ptr<char>
 ada_exception_message_1 (void)
 {
   struct value *e_msg_val;
-  char *e_msg = NULL;
   int e_msg_len;
-  struct cleanup *cleanups;
 
   /* For runtimes that support this feature, the exception message
      is passed as an unbounded string argument called "message".  */
@@ -12375,22 +12371,20 @@ ada_exception_message_1 (void)
   if (e_msg_len <= 0)
     return NULL;
 
-  e_msg = (char *) xmalloc (e_msg_len + 1);
-  cleanups = make_cleanup (xfree, e_msg);
-  read_memory_string (value_address (e_msg_val), e_msg, e_msg_len + 1);
-  e_msg[e_msg_len] = '\0';
+  gdb::unique_xmalloc_ptr<char> e_msg ((char *) xmalloc (e_msg_len + 1));
+  read_memory_string (value_address (e_msg_val), e_msg.get (), e_msg_len + 1);
+  e_msg.get ()[e_msg_len] = '\0';
 
-  discard_cleanups (cleanups);
   return e_msg;
 }
 
 /* Same as ada_exception_message_1, except that all exceptions are
    contained here (returning NULL instead).  */
 
-static char *
+static gdb::unique_xmalloc_ptr<char>
 ada_exception_message (void)
 {
-  char *e_msg = NULL;  /* Avoid a spurious uninitialized warning.  */
+  gdb::unique_xmalloc_ptr<char> e_msg;
 
   TRY
     {
@@ -12398,7 +12392,7 @@ ada_exception_message (void)
     }
   CATCH (e, RETURN_MASK_ERROR)
     {
-      e_msg = NULL;
+      e_msg.reset (nullptr);
     }
   END_CATCH
 
@@ -12639,7 +12633,6 @@ print_it_exception (enum ada_exception_catchpoint_kind ex, bpstat bs)
 {
   struct ui_out *uiout = current_uiout;
   struct breakpoint *b = bs->breakpoint_at;
-  char *exception_message;
 
   annotate_catchpoint (b->number);
 
@@ -12707,16 +12700,12 @@ print_it_exception (enum ada_exception_catchpoint_kind ex, bpstat bs)
 	break;
     }
 
-  exception_message = ada_exception_message ();
+  gdb::unique_xmalloc_ptr<char> exception_message = ada_exception_message ();
   if (exception_message != NULL)
     {
-      struct cleanup *cleanups = make_cleanup (xfree, exception_message);
-
       uiout->text (" (");
-      uiout->field_string ("exception-message", exception_message);
+      uiout->field_string ("exception-message", exception_message.get ());
       uiout->text (")");
-
-      do_cleanups (cleanups);
     }
 
   uiout->text (" at ");
-- 
2.13.6

Re: [RFA 1/3] Remove cleanup from ada-lang.c

[Simon Marchi]

On 2018-05-19 12:06 PM, Tom Tromey wrote:
> This removes a cleanup from ada-lang.c by having
> ada_exception_message_1 return a unique_xmalloc_ptr.

LGTM.

Simon

Re: [RFA 2/3] Remove cleanup from ada_collect_symbol_completion_matches

[Simon Marchi]

On 2018-05-19 12:06 PM, Tom Tromey wrote:
> ada_collect_symbol_completion_matches installs a null_cleanup but not
> any other cleanups.  This patch removes it.

It doesn't seem like any of the called function leaves a dangling cleanup, so LGTM.

Simon

Re: [RFA 3/3] Change ada_catchpoint::excep_string to be a std::string

[Simon Marchi]

On 2018-05-19 12:06 PM, Tom Tromey wrote:
> This changes ada_catchpoint::excep_string to be a std::string and then
> fixes up all t he users.
> 
> This found a memory leak in catch_ada_exception_command_split, where
> "cond" was copied but never freed.
> 
> I changed the type of the "cond_string" argument to
> catch_ada_exception_command_split to follow the rule that out
> parameters should be pointers and not references.

It wasn't a rule yet (AFAIK), but it is now.

https://sourceware.org/ml/gdb-patches/2018-05/msg00450.html

The patch LGTM, with some nits.

> 
> This patch enables the removal of some cleanups and also the function
> ada_get_next_arg.

You can remove the ada_catchpoint destructor I think.

> @@ -13333,13 +13286,11 @@ ada_exception_sal (enum ada_exception_catchpoint_kind ex, char *excep_string,
>  
>     EX_KIND is the kind of exception catchpoint to be created.
>  
> -   If EXCEPT_STRING is NULL, this catchpoint is expected to trigger
> +   If EXCEPT_STRING is empty, this catchpoint is expected to trigger
>     for all exceptions.  Otherwise, EXCEPT_STRING indicates the name
> -   of the exception to which this catchpoint applies.  When not NULL,
> -   the string must be allocated on the heap, and its deallocation
> -   is no longer the responsibility of the caller.
> +   of the exception to which this catchpoint applies.
>  
> -   COND_STRING, if not NULL, is the catchpoint condition.  This string
> +   COND_STRING, if not empty, is the catchpoint condition.  This string
>     must be allocated on the heap, and its deallocation is no longer
>     the responsibility of the caller.

The last part of this paragraph is probably no longer valid.

Simon

[pushed+RFC] C++ exception during command triggers stale cleanup internal-warning

[Joel Brobecker]

Hello,

There was a small bug in Tom's patch that caused the execution
of the "catch assert" command to throw a C++ exception. I have
pushed the following patch to master as an obvious fix to the
immediate issue:

  [] fix "stale cleanup" internal-warning when using "catch assert"

But at the same time, this leads me to believe we may have a weakness
top.c::execute_command, which installs a cleanup, and "forgets" to
discard it when C++ exceptions are raised.

But maybe that's just the way it is, and we just want to make sure
we never let C+ exceptions get away from us? Thoughts on that?

Anyways, for now, the immediate issue is fixed.

-- 
Joel

[pushed/Ada] fix "stale cleanup" internal-warning when using "catch assert" command

[Joel Brobecker]

Trying to insert a catchpoint on all Ada assertions now triggers
the following internal warning regardless of the situation. For
instance, not even debugging any program:

    (gdb) catch assert
    /[...]/gdb/common/cleanups.c:264: internal-warning:
    restore_my_cleanups has found a stale cleanup

This is due to a small bug in the following C++-ification commit:

    commit bc18fbb575437dd10089ef4619e46c0b9a93097d
    Date:   Fri May 18 15:58:50 2018 -0600
    Subject: Change ada_catchpoint::excep_string to be a std::string

The stale cleanup in question is the following one in top.c:execute_command:

    cleanup_if_error = make_bpstat_clear_actions_cleanup ();

This cleanup is expected to be discarded if there are no exception.
There were no GDB exception; however, a C++ exception was triggered,
because we passed NULL as the excep_string argument when calling
create_ada_exception_catchpoint, which is a reference to a const
string. So we get a C++ exception during the std::string constructor,
which propagates up, causing the cleanup to unexpectedly remain
in the cleanup chain.

This patch fixes the immediate issue of the incorrect call to
create_ada_exception_catchpoint.

gdb/ChangeLog:

        * ada-lang.c (catch_assert_command): Pass empty string instead
        of NULL for excep_string argument.

Tested on x86_64-linux, fixes the following failures:

  * catch_assert_if.exp: insert catchpoint on failed assertions with condition
  * catch_ex.exp: insert catchpoint on failed assertions

This also fixes about a dozen UNRESOLVED tests that are a consequence
of the two tests above failing and crashing GDB.

Pushed to master.

---
 gdb/ChangeLog  | 5 +++++
 gdb/ada-lang.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8d425a7..653771a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-05-22  Joel Brobecker  <brobecker@adacore.com>
+
+	* ada-lang.c (catch_assert_command): Pass empty string instead
+	of NULL for excep_string argument.
+
 2018-05-22  Maciej W. Rozycki  <macro@mips.com>
 
 	* mips-linux-nat.c (mips64_linux_register_addr): Return -1 if
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index eaf3058..64bddc2 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -13408,7 +13408,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,
+				   "", cond_string,
 				   tempflag, 1 /* enabled */,
 				   from_tty);
 }
-- 
2.1.4

Re: [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> There was a small bug in Tom's patch that caused the execution
Joel> of the "catch assert" command to throw a C++ exception.

Sorry about that.

Joel> But at the same time, this leads me to believe we may have a weakness
Joel> top.c::execute_command, which installs a cleanup, and "forgets" to
Joel> discard it when C++ exceptions are raised.

My view was always that dangling cleanups are nearly always bugs; with
the exceptions being functions that either mention "cleanup" in the name
or return a cleanup.

I'm still hoping that we can just remove all cleanups and not have to
deal with the issue any more.

Tom

Re: [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning

[Joel Brobecker]

> Joel> But at the same time, this leads me to believe we may have a weakness
> Joel> top.c::execute_command, which installs a cleanup, and "forgets" to
> Joel> discard it when C++ exceptions are raised.
> 
> My view was always that dangling cleanups are nearly always bugs; with
> the exceptions being functions that either mention "cleanup" in the name
> or return a cleanup.

Completely agreed. My question is: Should the function catch C++
exceptions in order to take care of the cleanup before re-throwing?
Thinking things further, I am realizing that this function is most
likely not alone in that situation. So there is no clear reason
to be treating that function specifically.

> I'm still hoping that we can just remove all cleanups and not have to
> deal with the issue any more.

Agree on the goal. To say that I am not a fan of cleanups would be
an understatement (I used to like them, until I realized how tricky
they are to maintain).

-- 
Joel