[PATCH 0/3] Remove cleanups from c-exp.y

[PATCH 0/3] Remove cleanups from c-exp.y

[Tom Tromey]

This series removes the remaining cleanups from c-exp.y, by
introducing a new object whose purpose is to manage allocations made
while parsing.

Tested by the buildbot.  (I solved my buildbot woes by making a
virtualenv and then installing buildbot there; I've also filed a bug
against Fedora reporting that the distro buildbot is broken.)

Tom

[PATCH 3/3] Remove remaining cleanups from c-exp.y

[Tom Tromey]

This removes the remaining cleanups from c-exp.y by moving some
globals into c_parse_state, and changing expansion_obstack to be an
auto_obstack.

gdb/ChangeLog
2019-01-05  Tom Tromey  <tom@tromey.com>

	* c-exp.y (struct c_parse_state) <macro_original_text,
	expansion_obstack>: New member.
	(macro_original_text, expansion_obstack): Remove globals.
	(scan_macro_expansion, scanning_macro_expansion)
	(finished_macro_expansion): Update.
	(scan_macro_cleanup): Remove.
	(yylex, c_parse): Update.
---
 gdb/ChangeLog | 10 ++++++
 gdb/c-exp.y   | 93 ++++++++++++++++++++-------------------------------
 2 files changed, 46 insertions(+), 57 deletions(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 83b2aa3fdd..7339dfd51c 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -77,6 +77,33 @@ struct c_parse_state
 
   /* Storage for some strings allocated during the parse.  */
   std::vector<gdb::unique_xmalloc_ptr<char>> strings;
+
+  /* When we find that lexptr (the global var defined in parse.c) is
+     pointing at a macro invocation, we expand the invocation, and call
+     scan_macro_expansion to save the old lexptr here and point lexptr
+     into the expanded text.  When we reach the end of that, we call
+     end_macro_expansion to pop back to the value we saved here.  The
+     macro expansion code promises to return only fully-expanded text,
+     so we don't need to "push" more than one level.
+
+     This is disgusting, of course.  It would be cleaner to do all macro
+     expansion beforehand, and then hand that to lexptr.  But we don't
+     really know where the expression ends.  Remember, in a command like
+
+     (gdb) break *ADDRESS if CONDITION
+
+     we evaluate ADDRESS in the scope of the current frame, but we
+     evaluate CONDITION in the scope of the breakpoint's location.  So
+     it's simply wrong to try to macro-expand the whole thing at once.  */
+  const char *macro_original_text = nullptr;
+
+  /* We save all intermediate macro expansions on this obstack for the
+     duration of a single parse.  The expansion text may sometimes have
+     to live past the end of the expansion, due to yacc lookahead.
+     Rather than try to be clever about saving the data for a single
+     token, we simply keep it all and delete it after parsing has
+     completed.  */
+  auto_obstack expansion_obstack;
 };
 
 /* This is set and cleared in c_parse.  */
@@ -2427,32 +2454,6 @@ static const struct token ident_tokens[] =
     {"typeid", TYPEID, OP_TYPEID, FLAG_CXX}
   };
 
-/* When we find that lexptr (the global var defined in parse.c) is
-   pointing at a macro invocation, we expand the invocation, and call
-   scan_macro_expansion to save the old lexptr here and point lexptr
-   into the expanded text.  When we reach the end of that, we call
-   end_macro_expansion to pop back to the value we saved here.  The
-   macro expansion code promises to return only fully-expanded text,
-   so we don't need to "push" more than one level.
-
-   This is disgusting, of course.  It would be cleaner to do all macro
-   expansion beforehand, and then hand that to lexptr.  But we don't
-   really know where the expression ends.  Remember, in a command like
-
-     (gdb) break *ADDRESS if CONDITION
-
-   we evaluate ADDRESS in the scope of the current frame, but we
-   evaluate CONDITION in the scope of the breakpoint's location.  So
-   it's simply wrong to try to macro-expand the whole thing at once.  */
-static const char *macro_original_text;
-
-/* We save all intermediate macro expansions on this obstack for the
-   duration of a single parse.  The expansion text may sometimes have
-   to live past the end of the expansion, due to yacc lookahead.
-   Rather than try to be clever about saving the data for a single
-   token, we simply keep it all and delete it after parsing has
-   completed.  */
-static struct obstack expansion_obstack;
 
 static void
 scan_macro_expansion (char *expansion)
@@ -2460,44 +2461,35 @@ scan_macro_expansion (char *expansion)
   char *copy;
 
   /* We'd better not be trying to push the stack twice.  */
-  gdb_assert (! macro_original_text);
+  gdb_assert (! cpstate->macro_original_text);
 
   /* Copy to the obstack, and then free the intermediate
      expansion.  */
-  copy = (char *) obstack_copy0 (&expansion_obstack, expansion,
+  copy = (char *) obstack_copy0 (&cpstate->expansion_obstack, expansion,
 				 strlen (expansion));
   xfree (expansion);
 
   /* Save the old lexptr value, so we can return to it when we're done
      parsing the expanded text.  */
-  macro_original_text = lexptr;
+  cpstate->macro_original_text = lexptr;
   lexptr = copy;
 }
 
 static int
 scanning_macro_expansion (void)
 {
-  return macro_original_text != 0;
+  return cpstate->macro_original_text != 0;
 }
 
 static void
 finished_macro_expansion (void)
 {
   /* There'd better be something to pop back to.  */
-  gdb_assert (macro_original_text);
+  gdb_assert (cpstate->macro_original_text);
 
   /* Pop back to the original text.  */
-  lexptr = macro_original_text;
-  macro_original_text = 0;
-}
-
-static void
-scan_macro_cleanup (void *dummy)
-{
-  if (macro_original_text)
-    finished_macro_expansion ();
-
-  obstack_free (&expansion_obstack, NULL);
+  lexptr = cpstate->macro_original_text;
+  cpstate->macro_original_text = 0;
 }
 
 /* Return true iff the token represents a C++ cast operator.  */
@@ -3262,7 +3254,7 @@ yylex (void)
   if (checkpoint > 0)
     {
       current.value.sval.ptr
-	= (const char *) obstack_copy0 (&expansion_obstack,
+	= (const char *) obstack_copy0 (&cpstate->expansion_obstack,
 					current.value.sval.ptr,
 					current.value.sval.length);
 
@@ -3282,9 +3274,6 @@ yylex (void)
 int
 c_parse (struct parser_state *par_state)
 {
-  int result;
-  struct cleanup *back_to;
-
   /* Setting up the parser state.  */
   scoped_restore pstate_restore = make_scoped_restore (&pstate);
   gdb_assert (par_state != NULL);
@@ -3305,13 +3294,6 @@ c_parse (struct parser_state *par_state)
   scoped_restore restore_macro_scope
     = make_scoped_restore (&expression_macro_scope, macro_scope.get ());
 
-  /* Initialize macro expansion code.  */
-  obstack_init (&expansion_obstack);
-  gdb_assert (! macro_original_text);
-  /* Note that parsing (within yyparse) freely installs cleanups
-     assuming they'll be run here (below).  */
-  back_to = make_cleanup (scan_macro_cleanup, 0);
-
   scoped_restore restore_yydebug = make_scoped_restore (&yydebug,
 							parser_debug);
 
@@ -3323,10 +3305,7 @@ c_parse (struct parser_state *par_state)
   popping = 0;
   name_obstack.clear ();
 
-  result = yyparse ();
-  do_cleanups (back_to);
-
-  return result;
+  return yyparse ();
 }
 
 #ifdef YYBISON
-- 
2.17.2

Re: [PATCH 3/3] Remove remaining cleanups from c-exp.y

[Simon Marchi]

On 2019-01-05 15:41, Tom Tromey wrote:
> This removes the remaining cleanups from c-exp.y by moving some
> globals into c_parse_state, and changing expansion_obstack to be an
> auto_obstack.

LGTM, thanks!

Simon

[PATCH 1/3] Use std::vector in type stacks

[Tom Tromey]

This removes the use of VEC from parse.c and, at the same time,
removes some related cleanups from c-exp.y.

gdb/ChangeLog
2019-01-05  Tom Tromey  <tom@tromey.com>

	* parser-defs.h (type_ptr): Remove typedef.  Don't declare VEC.
	(union type_stack_elt) <typelist_val>: Now a pointer to
	std::vector.
	(type_stack_cleanup): Don't declare.
	(push_typelist): Update.
	* parse.c (pop_typelist): Return a std::vector.
	(push_typelist): Take a std::vector.
	(follow_types): Update.  Do not free args.
	(type_stack_cleanup): Remove.
	* c-exp.y (struct c_parse_state): New.
	(cpstate): New global.
	(type_aggregate_p, exp, ptr_operator, parameter_typelist)
	(nonempty_typelist): Update.
	(func_mod): Create a new vector.
	(c_parse): Create a c_parse_state.
	(check_parameter_typelist): Do not delete params.
	(function_method): Update.  Do not delete type_list.
---
 gdb/ChangeLog     | 20 ++++++++++++++
 gdb/c-exp.y       | 66 ++++++++++++++++++++++++++++-------------------
 gdb/parse.c       | 24 +++++------------
 gdb/parser-defs.h |  9 ++-----
 4 files changed, 68 insertions(+), 51 deletions(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 3078702b75..eb6a6847cf 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -66,6 +66,20 @@
 
 static struct parser_state *pstate = NULL;
 
+/* Data that must be held for the duration of a parse.  */
+
+struct c_parse_state
+{
+  /* These are used to hold type lists and type stacks that are
+     allocated during the parse.  */
+  std::vector<std::unique_ptr<std::vector<struct type *>>> type_lists;
+  std::vector<std::unique_ptr<struct type_stack>> type_stacks;
+};
+
+/* This is set and cleared in c_parse.  */
+
+static struct c_parse_state *cpstate;
+
 int yyparse (void);
 
 static int yylex (void);
@@ -101,7 +115,7 @@ static int type_aggregate_p (struct type *);
     enum exp_opcode opcode;
 
     struct stoken_vector svec;
-    VEC (type_ptr) *tvec;
+    std::vector<struct type *> *tvec;
 
     struct type_stack *type_stack;
 
@@ -114,7 +128,7 @@ static int parse_number (struct parser_state *par_state,
 			 const char *, int, int, YYSTYPE *);
 static struct stoken operator_stoken (const char *);
 static struct stoken typename_stoken (const char *);
-static void check_parameter_typelist (VEC (type_ptr) *);
+static void check_parameter_typelist (std::vector<struct type *> *);
 static void write_destructor_name (struct parser_state *par_state,
 				   struct stoken);
 
@@ -552,10 +566,9 @@ arglist	:	arglist ',' exp   %prec ABOVE_COMMA
 	;
 
 function_method:       exp '(' parameter_typelist ')' const_or_volatile
-			{ int i;
-			  VEC (type_ptr) *type_list = $3;
-			  struct type *type_elt;
-			  LONGEST len = VEC_length (type_ptr, type_list);
+			{
+			  std::vector<struct type *> *type_list = $3;
+			  LONGEST len = type_list->size ();
 
 			  write_exp_elt_opcode (pstate, TYPE_INSTANCE);
 			  /* Save the const/volatile qualifiers as
@@ -564,13 +577,10 @@ function_method:       exp '(' parameter_typelist ')' const_or_volatile
 			  write_exp_elt_longcst (pstate,
 						 follow_type_instance_flags ());
 			  write_exp_elt_longcst (pstate, len);
-			  for (i = 0;
-			       VEC_iterate (type_ptr, type_list, i, type_elt);
-			       ++i)
+			  for (type *type_elt : *type_list)
 			    write_exp_elt_type (pstate, type_elt);
 			  write_exp_elt_longcst(pstate, len);
 			  write_exp_elt_opcode (pstate, TYPE_INSTANCE);
-			  VEC_free (type_ptr, type_list);
 			}
 	;
 
@@ -1157,9 +1167,7 @@ ptr_operator:
 ptr_operator_ts: ptr_operator
 			{
 			  $$ = get_type_stack ();
-			  /* This cleanup is eventually run by
-			     c_parse.  */
-			  make_cleanup (type_stack_cleanup, $$);
+			  cpstate->type_stacks.emplace_back ($$);
 			}
 	;
 
@@ -1209,7 +1217,10 @@ array_mod:	'[' ']'
 	;
 
 func_mod:	'(' ')'
-			{ $$ = NULL; }
+			{
+			  $$ = new std::vector<struct type *>;
+			  cpstate->type_lists.emplace_back ($$);
+			}
 	|	'(' parameter_typelist ')'
 			{ $$ = $2; }
 	;
@@ -1471,7 +1482,7 @@ parameter_typelist:
 			{ check_parameter_typelist ($1); }
 	|	nonempty_typelist ',' DOTDOTDOT
 			{
-			  VEC_safe_push (type_ptr, $1, NULL);
+			  $1->push_back (NULL);
 			  check_parameter_typelist ($1);
 			  $$ = $1;
 			}
@@ -1480,13 +1491,16 @@ parameter_typelist:
 nonempty_typelist
 	:	type
 		{
-		  VEC (type_ptr) *typelist = NULL;
-		  VEC_safe_push (type_ptr, typelist, $1);
+		  std::vector<struct type *> *typelist =
+		    new std::vector<struct type *>;
+		  cpstate->type_lists.emplace_back (typelist);
+
+		  typelist->push_back ($1);
 		  $$ = typelist;
 		}
 	|	nonempty_typelist ',' type
 		{
-		  VEC_safe_push (type_ptr, $1, $3);
+		  $1->push_back ($3);
 		  $$ = $1;
 		}
 	;
@@ -1758,30 +1772,27 @@ type_aggregate_p (struct type *type)
 /* Validate a parameter typelist.  */
 
 static void
-check_parameter_typelist (VEC (type_ptr) *params)
+check_parameter_typelist (std::vector<struct type *> *params)
 {
   struct type *type;
   int ix;
 
-  for (ix = 0; VEC_iterate (type_ptr, params, ix, type); ++ix)
+  for (ix = 0; ix < params->size (); ++ix)
     {
+      type = (*params)[ix];
       if (type != NULL && TYPE_CODE (check_typedef (type)) == TYPE_CODE_VOID)
 	{
 	  if (ix == 0)
 	    {
-	      if (VEC_length (type_ptr, params) == 1)
+	      if (params->size () == 1)
 		{
 		  /* Ok.  */
 		  break;
 		}
-	      VEC_free (type_ptr, params);
 	      error (_("parameter types following 'void'"));
 	    }
 	  else
-	    {
-	      VEC_free (type_ptr, params);
-	      error (_("'void' invalid as parameter type"));
-	    }
+	    error (_("'void' invalid as parameter type"));
 	}
     }
 }
@@ -3276,6 +3287,9 @@ c_parse (struct parser_state *par_state)
   gdb_assert (par_state != NULL);
   pstate = par_state;
 
+  c_parse_state cstate;
+  scoped_restore cstate_resotre = make_scoped_restore (&cpstate, &cstate);
+
   gdb::unique_xmalloc_ptr<struct macro_scope> macro_scope;
 
   if (expression_context_block)
diff --git a/gdb/parse.c b/gdb/parse.c
index de2d53b9de..e7168acf7a 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -1457,7 +1457,7 @@ pop_type_int (void)
 
 /* Pop a type list element from the global type stack.  */
 
-static VEC (type_ptr) *
+static std::vector<struct type *> *
 pop_typelist (void)
 {
   gdb_assert (!type_stack.elements.empty ());
@@ -1501,7 +1501,7 @@ push_type_stack (struct type_stack *stack)
 
 /* Copy the global type stack into a newly allocated type stack and
    return it.  The global stack is cleared.  The returned type stack
-   must be freed with type_stack_cleanup.  */
+   must be freed with delete.  */
 
 struct type_stack *
 get_type_stack (void)
@@ -1511,22 +1511,12 @@ get_type_stack (void)
   return result;
 }
 
-/* A cleanup function that destroys a single type stack.  */
-
-void
-type_stack_cleanup (void *arg)
-{
-  struct type_stack *stack = (struct type_stack *) arg;
-
-  delete stack;
-}
-
 /* Push a function type with arguments onto the global type stack.
    LIST holds the argument types.  If the final item in LIST is NULL,
    then the function will be varargs.  */
 
 void
-push_typelist (VEC (type_ptr) *list)
+push_typelist (std::vector<struct type *> *list)
 {
   type_stack_elt elt;
   elt.typelist_val = list;
@@ -1655,14 +1645,12 @@ follow_types (struct type *follow_type)
 
       case tp_function_with_arguments:
 	{
-	  VEC (type_ptr) *args = pop_typelist ();
+	  std::vector<struct type *> *args = pop_typelist ();
 
 	  follow_type
 	    = lookup_function_type_with_arguments (follow_type,
-						   VEC_length (type_ptr, args),
-						   VEC_address (type_ptr,
-								args));
-	  VEC_free (type_ptr, args);
+						   args->size (),
+						   args->data ());
 	}
 	break;
 
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index 195264f48a..5b38477895 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -200,9 +200,6 @@ struct objc_class_str
     int theclass;
   };
 
-typedef struct type *type_ptr;
-DEF_VEC_P (type_ptr);
-
 /* For parsing of complicated types.
    An array should be preceded in the list by the size of the array.  */
 enum type_pieces
@@ -225,7 +222,7 @@ union type_stack_elt
     enum type_pieces piece;
     int int_val;
     struct type_stack *stack_val;
-    VEC (type_ptr) *typelist_val;
+    std::vector<struct type *> *typelist_val;
   };
 
 /* The type stack is an instance of this structure.  */
@@ -303,9 +300,7 @@ extern struct type_stack *append_type_stack (struct type_stack *to,
 
 extern void push_type_stack (struct type_stack *stack);
 
-extern void type_stack_cleanup (void *arg);
-
-extern void push_typelist (VEC (type_ptr) *typelist);
+extern void push_typelist (std::vector<struct type *> *typelist);
 
 extern int dump_subexp (struct expression *, struct ui_file *, int);
 
-- 
2.17.2

Re: [PATCH 1/3] Use std::vector in type stacks

[Simon Marchi]

On 2019-01-05 15:41, Tom Tromey wrote:		}
> @@ -1480,13 +1491,16 @@ parameter_typelist:
>  nonempty_typelist
>  	:	type
>  		{
> -		  VEC (type_ptr) *typelist = NULL;
> -		  VEC_safe_push (type_ptr, typelist, $1);
> +		  std::vector<struct type *> *typelist =
> +		    new std::vector<struct type *>;

The = should be on the next line.

> @@ -3276,6 +3287,9 @@ c_parse (struct parser_state *par_state)
>    gdb_assert (par_state != NULL);
>    pstate = par_state;
> 
> +  c_parse_state cstate;
> +  scoped_restore cstate_resotre = make_scoped_restore (&cpstate, 
> &cstate);

"resotre"

Otherwise, LGTM.

Simon

Re: [PATCH 1/3] Use std::vector in type stacks

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> The = should be on the next line.

[...]

>> @@ -3276,6 +3287,9 @@ c_parse (struct parser_state *par_state)
>> gdb_assert (par_state != NULL);
>> pstate = par_state;
>> 
>> +  c_parse_state cstate;
>> +  scoped_restore cstate_resotre = make_scoped_restore (&cpstate,
>> &cstate);

Simon> "resotre"

Simon> Otherwise, LGTM.

Thanks for the review.  I'm checking it in with these fixes.

Tom

[PATCH 2/3] Remove string-related cleanup from c-exp.y

[Tom Tromey]

This removes a string-related cleanup from c-exp.y, by adding a new
member to c_parse_state to store the strings.

gdb/ChangeLog
2019-01-05  Tom Tromey  <tom@tromey.com>

	* c-exp.y (struct c_parse_state) <strings>: New member.
	(operator_stoken): Update.
---
 gdb/ChangeLog | 5 +++++
 gdb/c-exp.y   | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index eb6a6847cf..83b2aa3fdd 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -74,6 +74,9 @@ struct c_parse_state
      allocated during the parse.  */
   std::vector<std::unique_ptr<std::vector<struct type *>>> type_lists;
   std::vector<std::unique_ptr<struct type_stack>> type_stacks;
+
+  /* Storage for some strings allocated during the parse.  */
+  std::vector<gdb::unique_xmalloc_ptr<char>> strings;
 };
 
 /* This is set and cleared in c_parse.  */
@@ -1743,7 +1746,7 @@ operator_stoken (const char *op)
   st.ptr = buf;
 
   /* The toplevel (c_parse) will free the memory allocated here.  */
-  make_cleanup (free, buf);
+  cpstate->strings.emplace_back (buf);
   return st;
 };
 
-- 
2.17.2

Re: [PATCH 2/3] Remove string-related cleanup from c-exp.y

[Simon Marchi]

On 2019-01-05 15:41, Tom Tromey wrote:
> This removes a string-related cleanup from c-exp.y, by adding a new
> member to c_parse_state to store the strings.

LGTM.

Simon