RFC: introduce scoped cleanups

RFC: introduce scoped cleanups

[Tom Tromey]

This adds scoped cleanups to gdb.  A scoped cleanup is a more
efficient, and checkable, alternative to the common idiom:

    cleanup = make_cleanup (null_cleanup, NULL);
    ...
    do_cleanups (cleanup);

... where the cleanup is always run when exiting the scope.

The efficiency comes from stack allocating the scoped cleanup.  This
is probably minor.  However, I've noticed myself sometimes avoiding
null cleanups on this basis, and it is nice to erase that bias.

The checking comes from using a GCC extension when available.  This
check ensures that the scoped cleanup was either run or discarded when
the scope exits.

I'm curious to know what people think of this.

The patch includes an example use of scoped cleanups -- see the
linespec.c change.

This applies on top of my cleanup-checker series.  However it probably
applies just as well to trunk if you drop the cleanup_check.py change.

Tom
---
 gdb/cleanups.c               | 70 +++++++++++++++++++++++++++++++++++---------
 gdb/cleanups.h               | 55 ++++++++++++++++++++++++++++++++++
 gdb/contrib/cleanup_check.py |  2 +-
 gdb/linespec.c               |  4 +--
 4 files changed, 114 insertions(+), 17 deletions(-)

diff --git a/gdb/cleanups.c b/gdb/cleanups.c
index 02db9f5..76d28c4 100644
--- a/gdb/cleanups.c
+++ b/gdb/cleanups.c
@@ -35,9 +35,10 @@
    free that memory.  This function will be called both when the cleanup
    is executed and when it's discarded.  */
 
-struct cleanup
+struct real_cleanup
 {
-  struct cleanup *next;
+  struct cleanup base;
+
   void (*function) (void *);
   void (*free_arg) (void *);
   void *arg;
@@ -53,7 +54,7 @@ struct cleanup
    This is const for a bit of extra robustness.
    It is initialized to coax gcc into putting it into .rodata.
    All fields are initialized to survive -Wextra.  */
-static const struct cleanup sentinel_cleanup = { 0, 0, 0, 0 };
+static const struct real_cleanup sentinel_cleanup = { { 0, 0, 0 }, 0, 0, 0 };
 
 /* Handy macro to use when referring to sentinel_cleanup.  */
 #define SENTINEL_CLEANUP ((struct cleanup *) &sentinel_cleanup)
@@ -79,15 +80,15 @@ static struct cleanup *
 make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function,
 		  void *arg,  void (*free_arg) (void *))
 {
-  struct cleanup *new
-    = (struct cleanup *) xmalloc (sizeof (struct cleanup));
+  struct real_cleanup *new = XNEW (struct real_cleanup);
   struct cleanup *old_chain = *pmy_chain;
 
-  new->next = *pmy_chain;
+  new->base.next = *pmy_chain;
+  new->base.scoped = 0;
   new->function = function;
   new->free_arg = free_arg;
   new->arg = arg;
-  *pmy_chain = new;
+  *pmy_chain = &new->base;
 
   gdb_assert (old_chain != NULL);
   return old_chain;
@@ -152,10 +153,17 @@ do_my_cleanups (struct cleanup **pmy_chain,
   while ((ptr = *pmy_chain) != old_chain)
     {
       *pmy_chain = ptr->next;	/* Do this first in case of recursion.  */
-      (*ptr->function) (ptr->arg);
-      if (ptr->free_arg)
-	(*ptr->free_arg) (ptr->arg);
-      xfree (ptr);
+      if (ptr->scoped)
+	ptr->cleaned_up = 1;
+      else
+	{
+	  struct real_cleanup *rc = (struct real_cleanup *) ptr;
+
+	  (*rc->function) (rc->arg);
+	  if (rc->free_arg)
+	    (*rc->free_arg) (rc->arg);
+	  xfree (ptr);
+	}
     }
 }
 
@@ -200,9 +208,16 @@ discard_my_cleanups (struct cleanup **pmy_chain,
   while ((ptr = *pmy_chain) != old_chain)
     {
       *pmy_chain = ptr->next;
-      if (ptr->free_arg)
-	(*ptr->free_arg) (ptr->arg);
-      xfree (ptr);
+      if (ptr->scoped)
+	ptr->cleaned_up = 1;
+      else
+	{
+	  struct real_cleanup *rc = (struct real_cleanup *) ptr;
+
+	  if (rc->free_arg)
+	    (*rc->free_arg) (rc->arg);
+	  xfree (ptr);
+	}
     }
 }
 
@@ -292,3 +307,30 @@ void
 null_cleanup (void *arg)
 {
 }
+
+/* Initialize a scoped cleanup.  */
+
+struct cleanup *
+init_scoped_cleanup (struct scoped_cleanup *cl)
+{
+  struct cleanup *old_chain = cleanup_chain;
+
+  cl->base.next = cleanup_chain;
+  cleanup_chain = &cl->base;
+
+  cl->base.scoped = 1;
+  cl->base.cleaned_up = 0;
+
+  return old_chain;
+}
+
+/* Verify that a scoped cleanup was in fact handled.  */
+
+void
+cleanup_close_scope (struct scoped_cleanup *cl)
+{
+#ifdef SCOPED_CLEANUP_CHECKING
+  if (!cl->base.cleaned_up)
+    internal_warning (__FILE__, __LINE__, "scoped cleanup leaked");
+#endif /* SCOPED_CLEANUP_CHECKING */
+}
diff --git a/gdb/cleanups.h b/gdb/cleanups.h
index 463b923..9f8cbd4 100644
--- a/gdb/cleanups.h
+++ b/gdb/cleanups.h
@@ -66,4 +66,59 @@ extern void restore_final_cleanups (struct cleanup *);
    to pass to do_cleanups.  */
 extern void null_cleanup (void *);
 
+
+/* You should continue to treat this as opaque.  It is defined here
+   so that scoped cleanups can be stack-allocated and specially
+   treated.  */
+
+struct cleanup
+{
+  struct cleanup *next;
+
+  /* True if this is a scoped cleanup.  */
+  unsigned scoped : 1;
+
+  /* True if this is scoped cleanup has been cleaned up or discarded.
+     Not used for ordinary cleanups.  */
+
+  unsigned cleaned_up : 1;
+};
+
+/* This is used for scoped cleanups.  It should be treated as
+   opaque.  */
+
+struct scoped_cleanup
+{
+  struct cleanup base;
+};
+
+extern struct cleanup *init_scoped_cleanup (struct scoped_cleanup *);
+
+extern void cleanup_close_scope (struct scoped_cleanup *);
+
+#if defined (__GNUC__) && __GNUC__ >= 4
+
+/* Define this to consolidate #if checking with the
+   implementation.  */
+
+#define SCOPED_CLEANUP_CHECKING
+
+#define SCOPED_CLEANUP_ATTRIBUTE \
+  __attribute__ ((cleanup (cleanup_close_scope)))
+
+#else
+
+#define SCOPED_CLEANUP_ATTRIBUTE
+
+#endif
+
+/* Use this to declare a scoped cleanup.  A scoped cleanup must be
+   cleaned up or discarded whenever the scope is exited.  When
+   possible, this is checked at runtime.  */
+
+#define SCOPED_CLEANUP(name)						\
+  struct scoped_cleanup name ## __LINE__				\
+    SCOPED_CLEANUP_ATTRIBUTE;						\
+  struct cleanup *name = init_scoped_cleanup (& (name ## __LINE__))
+
 #endif /* CLEANUPS_H */
diff --git a/gdb/contrib/cleanup_check.py b/gdb/contrib/cleanup_check.py
index dca0ec8..a8f4fa1 100644
--- a/gdb/contrib/cleanup_check.py
+++ b/gdb/contrib/cleanup_check.py
@@ -53,7 +53,7 @@ special_names = set(['do_final_cleanups', 'discard_final_cleanups',
                      'restore_cleanups', 'restore_final_cleanups',
                      'exceptions_state_mc_init',
                      'make_my_cleanup2', 'make_final_cleanup', 'all_cleanups',
-                     'save_my_cleanups', 'quit_target'])
+                     'save_my_cleanups', 'quit_target', 'init_scoped_cleanup'])
 
 def needs_special_treatment(decl):
     return decl.name in special_names
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 4bbd6e4..c6603d0 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2821,7 +2821,7 @@ find_superclass_methods (VEC (typep) *superclasses,
 {
   int old_len = VEC_length (const_char_ptr, *result_names);
   VEC (typep) *iter_classes;
-  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
+  SCOPED_CLEANUP (cleanup);
 
   iter_classes = superclasses;
   while (1)
@@ -2855,7 +2855,7 @@ find_method (struct linespec_state *self, VEC (symtab_p) *file_symtabs,
 	     VEC (minsym_and_objfile_d) **minsyms)
 {
   struct symbol *sym;
-  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
+  SCOPED_CLEANUP (cleanup);
   int ix;
   int last_result_len;
   VEC (typep) *superclass_vec;
-- 
1.8.1.4

Re: RFC: introduce scoped cleanups

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> This adds scoped cleanups to gdb.

Pedro said he was interested in general stack-based allocation of
cleanups, so I implemented that, following his plan.

This patch adds two new features to gdb.

First, it adds optional stack-allocation of cleanup structures.  There
is a convenience macro for this, which uses alloca, so stack-allocation
isn't generally suitable in loops.  However, most cleanups can be
converted to stack allocation.

Second, like the previous iteration of the patch, this adds a "scoped"
cleanup that can be used to do runtime error-checking of cleanup use.
This checking only works when gdb is compiled with gcc.

I converted linespec.c entirely to the new setup, just to show what it
looks like.


With the introduction of make_stack_cleanup, it is now better to write a
cleanup function and to use the generic make_cleanup than it is to write
a make_cleanup_something_special function.

There is an example of this in the patch as well.


Let me know what you think.  In the absence of comments I imagine I will
eventually put this in and slowly work on converting other parts of gdb.
I think it is worth doing both on efficiency and reliability grounds.

Tom

diff --git a/gdb/cleanups.c b/gdb/cleanups.c
index 898e526..398e0fc 100644
--- a/gdb/cleanups.c
+++ b/gdb/cleanups.c
@@ -20,29 +20,6 @@
 #include "defs.h"
 #include "gdb_assert.h"
 
-/* The cleanup list records things that have to be undone
-   if an error happens (descriptors to be closed, memory to be freed, etc.)
-   Each link in the chain records a function to call and an
-   argument to give it.
-
-   Use make_cleanup to add an element to the cleanup chain.
-   Use do_cleanups to do all cleanup actions back to a given
-   point in the chain.  Use discard_cleanups to remove cleanups
-   from the chain back to a given point, not doing them.
-
-   If the argument is pointer to allocated memory, then you need
-   to additionally set the 'free_arg' member to a function that will
-   free that memory.  This function will be called both when the cleanup
-   is executed and when it's discarded.  */
-
-struct cleanup
-{
-  struct cleanup *next;
-  void (*function) (void *);
-  void (*free_arg) (void *);
-  void *arg;
-};
-
 /* Used to mark the end of a cleanup chain.
    The value is chosen so that it:
    - is non-NULL so that make_cleanup never returns NULL,
@@ -53,7 +30,7 @@ struct cleanup
    This is const for a bit of extra robustness.
    It is initialized to coax gcc into putting it into .rodata.
    All fields are initialized to survive -Wextra.  */
-static const struct cleanup sentinel_cleanup = { 0, 0, 0, 0 };
+static const struct cleanup sentinel_cleanup = { 0, 0, 0, 0, 0, 0 };
 
 /* Handy macro to use when referring to sentinel_cleanup.  */
 #define SENTINEL_CLEANUP ((struct cleanup *) &sentinel_cleanup)
@@ -84,6 +61,7 @@ make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function,
   struct cleanup *old_chain = *pmy_chain;
 
   new->next = *pmy_chain;
+  new->stack = 0;
   new->function = function;
   new->free_arg = free_arg;
   new->arg = arg;
@@ -155,7 +133,10 @@ do_my_cleanups (struct cleanup **pmy_chain,
       (*ptr->function) (ptr->arg);
       if (ptr->free_arg)
 	(*ptr->free_arg) (ptr->arg);
-      xfree (ptr);
+      if (ptr->stack)
+	ptr->cleaned_up = 1;
+      else
+	xfree (ptr);
     }
 }
 
@@ -202,7 +183,10 @@ discard_my_cleanups (struct cleanup **pmy_chain,
       *pmy_chain = ptr->next;
       if (ptr->free_arg)
 	(*ptr->free_arg) (ptr->arg);
-      xfree (ptr);
+      if (ptr->stack)
+	ptr->cleaned_up = 1;
+      else
+	xfree (ptr);
     }
 }
 
@@ -295,3 +279,37 @@ void
 null_cleanup (void *arg)
 {
 }
+
+/* Initialize a scoped cleanup.  */
+
+struct cleanup *
+init_stack_cleanup (struct cleanup *cl, make_cleanup_ftype *func,
+		    void *datum, make_cleanup_dtor_ftype *dtor)
+{
+  struct cleanup *old_chain = cleanup_chain;
+
+  cl->next = cleanup_chain;
+  cleanup_chain = cl;
+
+  cl->function = func;
+  cl->free_arg = dtor;
+  cl->arg = datum;
+  cl->stack = 1;
+  cl->cleaned_up = 0;
+
+  return old_chain;
+}
+
+#ifdef SCOPED_CLEANUP_CHECKING
+
+/* Verify that a scoped cleanup was in fact handled.  */
+
+void
+cleanup_close_scope (struct cleanup *cl)
+{
+  gdb_assert (cl->stack);
+  if (!cl->cleaned_up)
+    internal_warning (__FILE__, __LINE__, "scoped cleanup leaked");
+}
+
+#endif /* SCOPED_CLEANUP_CHECKING */
diff --git a/gdb/cleanups.h b/gdb/cleanups.h
index 463b923..3f49e80 100644
--- a/gdb/cleanups.h
+++ b/gdb/cleanups.h
@@ -45,6 +45,15 @@ extern struct cleanup *make_cleanup_dtor (make_cleanup_ftype *, void *,
 
 extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
 
+/* Allocate a cleanup on the stack, with alloca, and initialize
+   it.  Use like make_cleanup.  */
+
+#define make_stack_cleanup(FUNC, ARG) \
+  (init_stack_cleanup (alloca (sizeof (struct cleanup)), (FUNC), (ARG), NULL))
+
+#define make_stack_cleanup_dtor(FUNC, ARG, DTOR) \
+  (init_stack_cleanup (alloca (sizeof (struct cleanup)), (FUNC), (ARG), (DTOR)))
+
 /* A special value to pass to do_cleanups and do_final_cleanups
    to tell them to do all cleanups.  */
 extern struct cleanup *all_cleanups (void);
@@ -66,4 +75,76 @@ extern void restore_final_cleanups (struct cleanup *);
    to pass to do_cleanups.  */
 extern void null_cleanup (void *);
 
+
+/* You should continue to treat this as opaque.  It is defined here so
+   that scoped cleanups can be stack-allocated and specially treated.
+
+   The cleanup list records things that have to be undone
+   if an error happens (descriptors to be closed, memory to be freed, etc.)
+   Each link in the chain records a function to call and an
+   argument to give it.
+
+   Use make_cleanup to add an element to the cleanup chain.
+   Use do_cleanups to do all cleanup actions back to a given
+   point in the chain.  Use discard_cleanups to remove cleanups
+   from the chain back to a given point, not doing them.
+
+   If the argument is pointer to allocated memory, then you need
+   to additionally set the 'free_arg' member to a function that will
+   free that memory.  This function will be called both when the cleanup
+   is executed and when it's discarded.  */
+
+struct cleanup
+{
+  void (*function) (void *);
+  void (*free_arg) (void *);
+  void *arg;
+  struct cleanup *next;
+
+  /* True if this is a stack-allocated cleanup.  */
+  unsigned stack : 1;
+
+  /* True if this is scoped cleanup has been cleaned up or discarded.
+     Not used for ordinary cleanups.  */
+
+  unsigned cleaned_up : 1;
+};
+
+extern struct cleanup *init_stack_cleanup (struct cleanup *,
+					   make_cleanup_ftype *,
+					   void *,
+					   make_cleanup_dtor_ftype *);
+
+#if defined (__GNUC__) && __GNUC__ >= 4
+
+/* Define this to consolidate #if checking with the
+   implementation.  */
+
+#define SCOPED_CLEANUP_CHECKING
+
+#define SCOPED_CLEANUP_ATTRIBUTE \
+  __attribute__ ((cleanup (cleanup_close_scope)))
+
+extern void cleanup_close_scope (struct cleanup *);
+
+#else
+
+#define SCOPED_CLEANUP_ATTRIBUTE
+
+#endif
+
+/* Use this to declare a scoped cleanup.  A scoped cleanup must be
+   cleaned up or discarded whenever the scope is exited.  When
+   possible, this is checked at runtime.  */
+
+#define SCOPED_CLEANUP_DTOR(NAME, FUNC, DATA, DTOR)			\
+  struct cleanup NAME ## __LINE__ SCOPED_CLEANUP_ATTRIBUTE;		\
+  struct cleanup *NAME = init_stack_cleanup (& (NAME ## __LINE__),	\
+					     (FUNC), (DATA), (DTOR))
+
+#define SCOPED_CLEANUP(NAME, FUNC, DATA)				\
+  SCOPED_CLEANUP_DTOR(NAME, (FUNC), (DATA), NULL)
+
+#define SCOPED_NULL_CLEANUP(NAME) SCOPED_CLEANUP (NAME, null_cleanup, NULL)
+
 #endif /* CLEANUPS_H */
diff --git a/gdb/contrib/cleanup_check.py b/gdb/contrib/cleanup_check.py
index 0dd149f..a93af4c 100644
--- a/gdb/contrib/cleanup_check.py
+++ b/gdb/contrib/cleanup_check.py
@@ -54,7 +54,7 @@ special_names = set(['do_final_cleanups', 'discard_final_cleanups',
                      'restore_cleanups', 'restore_final_cleanups',
                      'exceptions_state_mc_init',
                      'make_my_cleanup2', 'make_final_cleanup', 'all_cleanups',
-                     'save_my_cleanups', 'quit_target'])
+                     'save_my_cleanups', 'quit_target', 'init_stack_cleanup'])
 
 def needs_special_treatment(decl):
     return decl.name in special_names
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 61e5377..2b58618 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -1267,11 +1267,11 @@ filter_results (struct linespec_state *self,
 	{
 	  const struct linespec_canonical_name *canonical;
 	  char *fullform;
-	  struct cleanup *cleanup;
+	  SCOPED_NULL_CLEANUP (cleanup);
 
 	  canonical = &self->canonical_names[j];
 	  fullform = canonical_to_fullform (canonical);
-	  cleanup = make_cleanup (xfree, fullform);
+	  make_cleanup (xfree, fullform);
 
 	  if (strcmp (name, fullform) == 0)
 	    add_sal_to_sals_basic (&lsal.sals, &result->sals[j]);
@@ -1352,8 +1352,8 @@ decode_line_2 (struct linespec_state *self,
 {
   char *args, *prompt;
   int i;
-  struct cleanup *old_chain;
   VEC (const_char_ptr) *filters = NULL;
+  SCOPED_CLEANUP (old_chain, VEC_cleanup (const_char_ptr), &filters);
   struct get_number_or_range_state state;
   struct decode_line_2_item *items;
   int items_count;
@@ -1362,12 +1362,10 @@ decode_line_2 (struct linespec_state *self,
   gdb_assert (self->canonical != NULL);
   gdb_assert (result->nelts >= 1);
 
-  old_chain = make_cleanup (VEC_cleanup (const_char_ptr), &filters);
-
   /* Prepare ITEMS array.  */
   items_count = result->nelts;
   items = xmalloc (sizeof (*items) * items_count);
-  make_cleanup (xfree, items);
+  make_stack_cleanup (xfree, items);
   for (i = 0; i < items_count; ++i)
     {
       const struct linespec_canonical_name *canonical;
@@ -1545,10 +1543,9 @@ unexpected_linespec_error (linespec_parser *parser)
       || token.type == LSTOKEN_KEYWORD)
     {
       char *string;
-      struct cleanup *cleanup;
 
       string = copy_token_string (token);
-      cleanup = make_cleanup (xfree, string);
+      make_stack_cleanup (xfree, string);
       throw_error (GENERIC_ERROR,
 		   _("malformed linespec error: unexpected %s, \"%s\""),
 		   token_type_strings[token.type], string);
@@ -1604,7 +1601,7 @@ linespec_parse_basic (linespec_parser *parser)
     {
       /* Record the line offset and get the next token.  */
       name = copy_token_string (token);
-      cleanup = make_cleanup (xfree, name);
+      cleanup = make_stack_cleanup (xfree, name);
       PARSER_RESULT (parser)->line_offset = linespec_parse_line_offset (name);
       do_cleanups (cleanup);
 
@@ -1631,7 +1628,7 @@ linespec_parse_basic (linespec_parser *parser)
   /* The current token will contain the name of a function, method,
      or label.  */
   name  = copy_token_string (token);
-  cleanup = make_cleanup (xfree, name);
+  cleanup = make_stack_cleanup (xfree, name);
 
   /* Try looking it up as a function/method.  */
   find_linespec_symbols (PARSER_STATE (parser),
@@ -1685,7 +1682,7 @@ linespec_parse_basic (linespec_parser *parser)
 	  /* User specified an offset.  Record the line offset and
 	     get the next token.  */
 	  name = copy_token_string (token);
-	  cleanup = make_cleanup (xfree, name);
+	  cleanup = make_stack_cleanup (xfree, name);
 	  PARSER_RESULT (parser)->line_offset
 	    = linespec_parse_line_offset (name);
 	  do_cleanups (cleanup);
@@ -1697,7 +1694,7 @@ linespec_parse_basic (linespec_parser *parser)
 	{
 	  /* Grab a copy of the label's name and look it up.  */
 	  name = copy_token_string (token);
-	  cleanup = make_cleanup (xfree, name);
+	  cleanup = make_stack_cleanup (xfree, name);
 	  labels = find_label_symbols (PARSER_STATE (parser),
 				       PARSER_RESULT (parser)->function_symbols,
 				       &symbols, name);
@@ -1731,7 +1728,7 @@ linespec_parse_basic (linespec_parser *parser)
 
 	      /* Record the lione offset and get the next token.  */
 	      name = copy_token_string (token);
-	      cleanup = make_cleanup (xfree, name);
+	      cleanup = make_stack_cleanup (xfree, name);
 
 	      PARSER_RESULT (parser)->line_offset
 		= linespec_parse_line_offset (name);
@@ -1901,7 +1898,7 @@ create_sals_line_offset (struct linespec_state *self,
 	decode_digits_ordinary (self, ls, best_entry->line,
 				&intermediate_results, &best_entry);
 
-      cleanup = make_cleanup (xfree, intermediate_results.sals);
+      cleanup = make_stack_cleanup (xfree, intermediate_results.sals);
 
       /* For optimized code, the compiler can scatter one source line
 	 across disjoint ranges of PC values, even when no duplicate
@@ -1914,9 +1911,9 @@ create_sals_line_offset (struct linespec_state *self,
 	 block.  If yes, the other PCs are filtered out.  */
 
       filter = XNEWVEC (int, intermediate_results.nelts);
-      make_cleanup (xfree, filter);
+      make_stack_cleanup (xfree, filter);
       blocks = XNEWVEC (struct block *, intermediate_results.nelts);
-      make_cleanup (xfree, blocks);
+      make_stack_cleanup (xfree, blocks);
 
       for (i = 0; i < intermediate_results.nelts; ++i)
 	{
@@ -2190,7 +2187,7 @@ parse_linespec (linespec_parser *parser, char **argptr)
 
       /* User specified an expression, *EXPR.  */
       copy = expr = copy_token_string (token);
-      cleanup = make_cleanup (xfree, expr);
+      cleanup = make_stack_cleanup (xfree, expr);
       PARSER_RESULT (parser)->expr_pc = linespec_expression_to_pc (&copy);
       discard_cleanups (cleanup);
       PARSER_RESULT (parser)->expression = expr;
@@ -2218,7 +2215,7 @@ parse_linespec (linespec_parser *parser, char **argptr)
 
       /* User specified a convenience variable or history value.  */
       var = copy_token_string (token);
-      cleanup = make_cleanup (xfree, var);
+      cleanup = make_stack_cleanup (xfree, var);
       PARSER_RESULT (parser)->line_offset
 	= linespec_parse_variable (PARSER_STATE (parser), var);
       do_cleanups (cleanup);
@@ -2412,7 +2409,7 @@ decode_line_full (char **argptr, int flags,
 		  const char *filter)
 {
   struct symtabs_and_lines result;
-  struct cleanup *cleanups;
+  SCOPED_NULL_CLEANUP (cleanups);
   VEC (const_char_ptr) *filters = NULL;
   linespec_parser parser;
   struct linespec_state *state;
@@ -2428,7 +2425,7 @@ decode_line_full (char **argptr, int flags,
 
   linespec_parser_new (&parser, flags, current_language, default_symtab,
 		       default_line, canonical);
-  cleanups = make_cleanup (linespec_parser_delete, &parser);
+  make_stack_cleanup (linespec_parser_delete, &parser);
   save_current_program_space ();
 
   result = parse_linespec (&parser, argptr);
@@ -2443,7 +2440,7 @@ decode_line_full (char **argptr, int flags,
     {
       int i;
 
-      make_cleanup (xfree, state->canonical_names);
+      make_stack_cleanup (xfree, state->canonical_names);
       for (i = 0; i < result.nelts; ++i)
 	{
 	  gdb_assert (state->canonical_names[i].suffix != NULL);
@@ -2463,7 +2460,7 @@ decode_line_full (char **argptr, int flags,
     {
       if (filter != NULL)
 	{
-	  make_cleanup (VEC_cleanup (const_char_ptr), &filters);
+	  make_stack_cleanup (VEC_cleanup (const_char_ptr), &filters);
 	  VEC_safe_push (const_char_ptr, filters, filter);
 	  filter_results (state, &result, filters);
 	}
@@ -2485,11 +2482,11 @@ decode_line_1 (char **argptr, int flags,
 {
   struct symtabs_and_lines result;
   linespec_parser parser;
-  struct cleanup *cleanups;
+  SCOPED_NULL_CLEANUP (cleanups);
 
   linespec_parser_new (&parser, flags, current_language, default_symtab,
 		       default_line, NULL);
-  cleanups = make_cleanup (linespec_parser_delete, &parser);
+  make_stack_cleanup (linespec_parser_delete, &parser);
   save_current_program_space ();
 
   result = parse_linespec (&parser, argptr);
@@ -2598,13 +2595,12 @@ decode_objc (struct linespec_state *self, linespec_p ls, char **argptr)
   VEC (const_char_ptr) *symbol_names = NULL;
   struct symtabs_and_lines values;
   char *new_argptr;
-  struct cleanup *cleanup = make_cleanup (VEC_cleanup (const_char_ptr),
-					  &symbol_names);
+  SCOPED_CLEANUP (cleanup, VEC_cleanup (const_char_ptr), &symbol_names);
 
   info.state = self;
   info.file_symtabs = NULL;
   VEC_safe_push (symtab_p, info.file_symtabs, NULL);
-  make_cleanup (VEC_cleanup (symtab_p), &info.file_symtabs);
+  make_stack_cleanup (VEC_cleanup (symtab_p), &info.file_symtabs);
   info.result.symbols = NULL;
   info.result.minimal_symbols = NULL;
   values.nelts = 0;
@@ -2703,16 +2699,14 @@ lookup_prefix_sym (struct linespec_state *state, VEC (symtab_p) *file_symtabs,
   int ix;
   struct symtab *elt;
   struct decode_compound_collector collector;
-  struct cleanup *outer;
+  SCOPED_CLEANUP (outer, VEC_cleanup (symbolp), &collector.symbols);
   struct cleanup *cleanup;
 
   collector.symbols = NULL;
-  outer = make_cleanup (VEC_cleanup (symbolp), &collector.symbols);
-
   collector.unique_syms = htab_create_alloc (1, htab_hash_pointer,
 					     htab_eq_pointer, NULL,
 					     xcalloc, xfree);
-  cleanup = make_cleanup_htab_delete (collector.unique_syms);
+  cleanup = make_stack_cleanup (htab_delete_cleanup, collector.unique_syms);
 
   for (ix = 0; VEC_iterate (symtab_p, file_symtabs, ix, elt); ++ix)
     {
@@ -2825,7 +2819,7 @@ find_superclass_methods (VEC (typep) *superclasses,
 {
   int old_len = VEC_length (const_char_ptr, *result_names);
   VEC (typep) *iter_classes;
-  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
+  SCOPED_NULL_CLEANUP (cleanup);
 
   iter_classes = superclasses;
   while (1)
@@ -2859,7 +2853,7 @@ find_method (struct linespec_state *self, VEC (symtab_p) *file_symtabs,
 	     VEC (minsym_and_objfile_d) **minsyms)
 {
   struct symbol *sym;
-  struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
+  SCOPED_NULL_CLEANUP (cleanup);
   int ix;
   int last_result_len;
   VEC (typep) *superclass_vec;
@@ -2888,9 +2882,9 @@ find_method (struct linespec_state *self, VEC (symtab_p) *file_symtabs,
      because we collect data across the program space before deciding
      what to do.  */
   superclass_vec = NULL;
-  make_cleanup (VEC_cleanup (typep), &superclass_vec);
+  make_stack_cleanup (VEC_cleanup (typep), &superclass_vec);
   result_names = NULL;
-  make_cleanup (VEC_cleanup (const_char_ptr), &result_names);
+  make_stack_cleanup (VEC_cleanup (const_char_ptr), &result_names);
   last_result_len = 0;
   for (ix = 0; VEC_iterate (symbolp, sym_classes, ix, sym); ++ix)
     {
@@ -2979,13 +2973,13 @@ static VEC (symtab_p) *
 collect_symtabs_from_filename (const char *file)
 {
   struct symtab_collector collector;
-  struct cleanup *cleanups;
+  SCOPED_NULL_CLEANUP (cleanups);
   struct program_space *pspace;
 
   collector.symtabs = NULL;
   collector.symtab_table = htab_create (1, htab_hash_pointer, htab_eq_pointer,
 					NULL);
-  cleanups = make_cleanup_htab_delete (collector.symtab_table);
+  make_stack_cleanup (htab_delete_cleanup, collector.symtab_table);
 
   /* Find that file's data.  */
   ALL_PSPACES (pspace)
@@ -3034,8 +3028,7 @@ find_function_symbols (struct linespec_state *state,
 {
   struct collect_info info;
   VEC (const_char_ptr) *symbol_names = NULL;
-  struct cleanup *cleanup = make_cleanup (VEC_cleanup (const_char_ptr),
-					  &symbol_names);
+  SCOPED_CLEANUP (cleanup, VEC_cleanup (const_char_ptr), &symbol_names);
 
   info.state = state;
   info.result.symbols = NULL;
@@ -3078,26 +3071,25 @@ find_linespec_symbols (struct linespec_state *state,
 		       VEC (symbolp) **symbols,
 		       VEC (minsym_and_objfile_d) **minsyms)
 {
-  struct cleanup *cleanup;
+  SCOPED_NULL_CLEANUP (cleanup);
   char *canon;
   const char *lookup_name;
   volatile struct gdb_exception except;
 
-  cleanup = demangle_for_lookup (name, state->language->la_language,
-				 &lookup_name);
+  demangle_for_lookup (name, state->language->la_language, &lookup_name);
   if (state->language->la_language == language_ada)
     {
       /* In Ada, the symbol lookups are performed using the encoded
          name rather than the demangled name.  */
       lookup_name = ada_name_for_lookup (name);
-      make_cleanup (xfree, (void *) lookup_name);
+      make_stack_cleanup (xfree, (void *) lookup_name);
     }
 
   canon = cp_canonicalize_string_no_typedefs (lookup_name);
   if (canon != NULL)
     {
       lookup_name = canon;
-      make_cleanup (xfree, canon);
+      make_stack_cleanup (xfree, canon);
     }
 
   /* It's important to not call expand_symtabs_matching unnecessarily
@@ -3152,19 +3144,19 @@ find_linespec_symbols (struct linespec_state *state,
       /* LOOKUP_NAME points to the class name.
 	 LAST points to the method name.  */
       klass = xmalloc ((last - lookup_name + 1) * sizeof (char));
-      make_cleanup (xfree, klass);
+      make_stack_cleanup (xfree, klass);
       strncpy (klass, lookup_name, last - lookup_name);
       klass[last - lookup_name] = '\0';
 
       /* Skip past the scope operator.  */
       last += strlen (scope_op);
       method = xmalloc ((strlen (last) + 1) * sizeof (char));
-      make_cleanup (xfree, method);
+      make_stack_cleanup (xfree, method);
       strcpy (method, last);
 
       /* Find a list of classes named KLASS.  */
       classes = lookup_prefix_sym (state, file_symtabs, klass);
-      make_cleanup (VEC_cleanup (symbolp), &classes);
+      make_stack_cleanup (VEC_cleanup (symbolp), &classes);
 
       if (!VEC_empty (symbolp, classes))
 	{
@@ -3524,7 +3516,7 @@ search_minsyms_for_name (struct collect_info *info, const char *name,
   ALL_PSPACES (pspace)
   {
     struct collect_minsyms local;
-    struct cleanup *cleanup;
+    SCOPED_NULL_CLEANUP (cleanup);
 
     if (search_pspace != NULL && search_pspace != pspace)
       continue;
@@ -3537,8 +3529,7 @@ search_minsyms_for_name (struct collect_info *info, const char *name,
     local.funfirstline = info->state->funfirstline;
     local.list_mode = info->state->list_mode;
 
-    cleanup = make_cleanup (VEC_cleanup (minsym_and_objfile_d),
-			    &local.msyms);
+    make_cleanup (VEC_cleanup (minsym_and_objfile_d), &local.msyms);
 
     ALL_OBJFILES (objfile)
     {
diff --git a/gdb/utils.c b/gdb/utils.c
index 18ee9bb..d1609a3 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -403,10 +403,10 @@ make_cleanup_unpush_target (struct target_ops *ops)
   return make_cleanup (do_unpush_target, ops);
 }
 
-/* Helper for make_cleanup_htab_delete compile time checking the types.  */
+/* A cleanup function that deletes an htab_t.  */
 
-static void
-do_htab_delete_cleanup (void *htab_voidp)
+void
+htab_delete_cleanup (void *htab_voidp)
 {
   htab_t htab = htab_voidp;
 
@@ -418,7 +418,7 @@ do_htab_delete_cleanup (void *htab_voidp)
 struct cleanup *
 make_cleanup_htab_delete (htab_t htab)
 {
-  return make_cleanup (do_htab_delete_cleanup, htab);
+  return make_cleanup (htab_delete_cleanup, htab);
 }
 
 struct restore_ui_file_closure
diff --git a/gdb/utils.h b/gdb/utils.h
index 146a558..216278d 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -113,6 +113,8 @@ extern struct cleanup *make_cleanup_free_so (struct so_list *so);
 
 extern struct cleanup *make_cleanup_restore_current_language (void);
 
+extern void htab_delete_cleanup (void *htab);
+
 extern struct cleanup *make_cleanup_htab_delete (htab_t htab);
 
 extern void free_current_contents (void *);

Re: RFC: introduce scoped cleanups

[Joel Brobecker]

> Pedro said he was interested in general stack-based allocation of
> cleanups, so I implemented that, following his plan.

Is this purely for performance?

> Second, like the previous iteration of the patch, this adds a "scoped"
> cleanup that can be used to do runtime error-checking of cleanup use.
> This checking only works when gdb is compiled with gcc.
> 
> I converted linespec.c entirely to the new setup, just to show what it
> looks like.

Looks pretty nice, I'd say!

-- 
Joel

Re: RFC: introduce scoped cleanups

[Tom Tromey]

>> Pedro said he was interested in general stack-based allocation of
>> cleanups, so I implemented that, following his plan.

Joel> Is this purely for performance?

Yeah, I think so.

Tom

Re: RFC: introduce scoped cleanups

[Pedro Alves]

On 05/31/2013 04:56 PM, Tom Tromey wrote:
>>> Pedro said he was interested in general stack-based allocation of
>>> cleanups, so I implemented that, following his plan.
> 
> Joel> Is this purely for performance?
> 
> Yeah, I think so.

Yeah, like, Tom, I've occasionally found myself sometimes
considering avoiding cleanups, avoiding the heap allocations,
in cases performance may matter.  Clearly we're not alone, as
evidenced from several of Tom's patches exposing spots where
we were avoiding having a "master" cleanup, some times using
tricky conditionals.  I had thought of this stack cleanups
scheme before, but never actually went through with it.  Seeing
Tom's scope cleanups were already stack based and most of the
way there, made me feel that it'd be a pity to be able to have
stack-based cleanups, but then as soon as you need a real cleanup
that does actual cleanup (as opposed to null_cleanup), you still
need to get back to install a heap cleanup anyway.

-- 
Pedro Alves

Re: RFC: introduce scoped cleanups

[Tom Tromey]

Pedro> Seeing Tom's scope cleanups were already stack based and most of
Pedro> the way there, made me feel that it'd be a pity to be able to
Pedro> have stack-based cleanups, but then as soon as you need a real
Pedro> cleanup that does actual cleanup (as opposed to null_cleanup),
Pedro> you still need to get back to install a heap cleanup anyway.

My one concern about this code is that, if a stack cleanup should be
leaked, then any future cleanup operation will refer to invalid memory.

Right now, cleanup failures like this are a memory leak with the
possibility of wrong behavior -- but the new way seems more directly
fatal.

Another option would be to allocate cleanups on an obstack.
This would often be efficient and would avoid the above problem.


This approach would either have to still leave the special scoped
cleanup on the stack, at least when compiling with GCC (and perhaps only
if in "development=true" mode); or we would need to come up with some
extra hack to make this work properly with obstack allocation.

Tom

Re: RFC: introduce scoped cleanups

[Joel Brobecker]

> My one concern about this code is that, if a stack cleanup should be
> leaked, then any future cleanup operation will refer to invalid memory.
> 
> Right now, cleanup failures like this are a memory leak with the
> possibility of wrong behavior -- but the new way seems more directly
> fatal.
> 
> Another option would be to allocate cleanups on an obstack.
> This would often be efficient and would avoid the above problem.

A very valid concern, IMO! I've never seen heap allocation as
a reason for concern with respect to performance, and thus have
never hesitated calling malloc. The only reason I have prefered
alloca whenever possible is that it allows me to be lazy :-)!

But this made me realize something: Why would someone want to do
stack-cleanups instead of just calling alloca directly? Since
alloca is basically a call-and-forget, what's the advantage of
going through a stack-based cleanup?

Regardless of the above, I like the idea of performing the cleanups
on an obstack; fast and yet a little more resilient to programming
errors. Not sure if that would be something easy to implement or not,
though.

-- 
Joel

Re: RFC: introduce scoped cleanups

[Tom Tromey]

Joel> A very valid concern, IMO! I've never seen heap allocation as
Joel> a reason for concern with respect to performance, and thus have
Joel> never hesitated calling malloc. The only reason I have prefered
Joel> alloca whenever possible is that it allows me to be lazy :-)!

It doesn't happen much, but you can see a few spots where someone coded
around the malloc call -- the various conditional cleanups that my
series removed.

Joel> But this made me realize something: Why would someone want to do
Joel> stack-cleanups instead of just calling alloca directly? Since
Joel> alloca is basically a call-and-forget, what's the advantage of
Joel> going through a stack-based cleanup?

It is the difference between running the cleanup and freeing the memory
that is used by the cleanup object itself.

Joel> Regardless of the above, I like the idea of performing the cleanups
Joel> on an obstack; fast and yet a little more resilient to programming
Joel> errors. Not sure if that would be something easy to implement or not,
Joel> though.

It ought to be easy.  I will look into it.

Tom

Re: RFC: introduce scoped cleanups

[Tom Tromey]

Joel> Regardless of the above, I like the idea of performing the cleanups
Joel> on an obstack; fast and yet a little more resilient to programming
Joel> errors. Not sure if that would be something easy to implement or not,
Joel> though.

Tom> It ought to be easy.  I will look into it.

I think it is easy to implement.  And, I think I've convinced myself
that it is safe.  But it seems to me that we could keep a freelist
instead -- this is much more obviously safe, and also, I think,
reasonably fast in most cases.  So I think I'm more inclined to take
this approach instead.

Tom