Have parser reset the innermost block tracker

I ran across a comment in symfile.c today:

  /* Clear globals which might have pointed into a removed objfile.
     FIXME: It's not clear which of these are supposed to persist
     between expressions and which ought to be reset each time.  */

It seems to me that this can be clarified: the parser entry points
ought to reset the innermost block tracker (and the expression context
block), and these should not be considered valid for code to use at
arbitrary times -- only immediately after an expression has been
parsed.

This patch implements this idea.  This could be further improved by
removing the parser globals and changing the parser functions to
return this information, but I have not done this.

Tested by the buildbot.

gdb/ChangeLog
2019-03-23  Tom Tromey  <tom@tromey.com>

	* varobj.c (varobj_create): Update.
	* symfile.c (clear_symtab_users): Don't reset innermost_block.
	* printcmd.c (display_command, do_one_display): Don't reset
	innermost_block.
	* parser-defs.h (enum innermost_block_tracker_type): Move to
	expression.h.
	(innermost_block): Update comment.
	* parse.c (parse_exp_1): Add tracker_types parameter.
	(parse_exp_in_context): Rename from parse_exp_in_context_1.  Add
	tracker_types parameter.  Reset innermost_block.
	(parse_exp_in_context): Remove.
	(parse_expression_for_completion): Update.
	* objfiles.c (~objfile): Don't reset expression_context_block or
	innermost_block.
	* expression.h (enum innermost_block_tracker_type): Move from
	parser-defs.h.
	(parse_exp_1): Add tracker_types parameter.
	* breakpoint.c (set_breakpoint_condition, watch_command_1): Don't
	reset innermost_block.

Author: tromey

gdb/ChangeLog

@@ -1,3 +1,25 @@
+2019-03-23  Tom Tromey  <tom@tromey.com>
+
+	* varobj.c (varobj_create): Update.
+	* symfile.c (clear_symtab_users): Don't reset innermost_block.
+	* printcmd.c (display_command, do_one_display): Don't reset
+	innermost_block.
+	* parser-defs.h (enum innermost_block_tracker_type): Move to
+	expression.h.
+	(innermost_block): Update comment.
+	* parse.c (parse_exp_1): Add tracker_types parameter.
+	(parse_exp_in_context): Rename from parse_exp_in_context_1.  Add
+	tracker_types parameter.  Reset innermost_block.
+	(parse_exp_in_context): Remove.
+	(parse_expression_for_completion): Update.
+	* objfiles.c (~objfile): Don't reset expression_context_block or
+	innermost_block.
+	* expression.h (enum innermost_block_tracker_type): Move from
+	parser-defs.h.
+	(parse_exp_1): Add tracker_types parameter.
+	* breakpoint.c (set_breakpoint_condition, watch_command_1): Don't
+	reset innermost_block.
+
 2019-03-23  Tom Tromey  <tom@tromey.com>
 
 	* objfiles.h: Include bcache.h.

gdb/breakpoint.c

@@ -880,7 +880,6 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
 	{
 	  struct watchpoint *w = (struct watchpoint *) b;
 
-	  innermost_block.reset ();
 	  arg = exp;
 	  w->cond_exp = parse_exp_1 (&arg, 0, 0, 0);
 	  if (*arg)
@@ -10602,7 +10601,6 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   /* Parse the rest of the arguments.  From here on out, everything
      is in terms of a newly allocated string instead of the original
      ARG.  */
-  innermost_block.reset ();
   std::string expression (arg, exp_end - arg);
   exp_start = arg = expression.c_str ();
   expression_up exp = parse_exp_1 (&arg, 0, 0, 0);
@@ -10664,7 +10662,6 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
   toklen = end_tok - tok;
   if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
     {
-      innermost_block.reset ();
       tok = cond_start = end_tok + 1;
       parse_exp_1 (&tok, 0, 0, 0);
 

gdb/expression.h

@@ -23,6 +23,23 @@
 
 #include "symtab.h"		/* Needed for "struct block" type.  */
 
+/* While parsing expressions we need to track the innermost lexical block
+   that we encounter.  In some situations we need to track the innermost
+   block just for symbols, and in other situations we want to track the
+   innermost block for symbols and registers.  These flags are used by the
+   innermost block tracker to control which blocks we consider for the
+   innermost block.  These flags can be combined together as needed.  */
+
+enum innermost_block_tracker_type
+{
+  /* Track the innermost block for symbols within an expression.  */
+  INNERMOST_BLOCK_FOR_SYMBOLS = (1 << 0),
+
+  /* Track the innermost block for registers within an expression.  */
+  INNERMOST_BLOCK_FOR_REGISTERS = (1 << 1)
+};
+DEF_ENUM_FLAGS_TYPE (enum innermost_block_tracker_type,
+		     innermost_block_tracker_types);
 
 /* Definitions for saved C expressions.  */
 
@@ -105,7 +122,9 @@ extern struct type *parse_expression_for_completion
     (const char *, gdb::unique_xmalloc_ptr<char> *, enum type_code *);
 
 extern expression_up parse_exp_1 (const char **, CORE_ADDR pc,
-				  const struct block *, int);
+				  const struct block *, int,
+				  innermost_block_tracker_types
+				    = INNERMOST_BLOCK_FOR_SYMBOLS);
 
 /* For use by parsers; set if we want to parse an expression and
    attempt completion.  */

gdb/objfiles.c

@@ -670,12 +670,6 @@ objfile::~objfile ()
      for example), so we need to call this here.  */
   clear_pc_function_cache ();
 
-  /* Clear globals which might have pointed into a removed objfile.
-     FIXME: It's not clear which of these are supposed to persist
-     between expressions and which ought to be reset each time.  */
-  expression_context_block = NULL;
-  innermost_block.reset ();
-
   /* Check to see if the current_source_symtab belongs to this objfile,
      and if so, call clear_current_source_symtab_and_line.  */
 

gdb/parse.c

@@ -116,10 +116,8 @@ static int prefixify_subexp (struct expression *, struct expression *, int,
 
 static expression_up parse_exp_in_context (const char **, CORE_ADDR,
 					   const struct block *, int,
-					   int, int *);
-static expression_up parse_exp_in_context_1 (const char **, CORE_ADDR,
-					     const struct block *, int,
-					     int, int *);
+					   int, int *,
+					   innermost_block_tracker_types);
 
 /* Documented at it's declaration.  */
 
@@ -1095,18 +1093,10 @@ prefixify_subexp (struct expression *inexpr,
 
 expression_up
 parse_exp_1 (const char **stringptr, CORE_ADDR pc, const struct block *block,
-	     int comma)
+	     int comma, innermost_block_tracker_types tracker_types)
 {
-  return parse_exp_in_context (stringptr, pc, block, comma, 0, NULL);
-}
-
-static expression_up
-parse_exp_in_context (const char **stringptr, CORE_ADDR pc,
-		      const struct block *block,
-		      int comma, int void_context_p, int *out_subexp)
-{
-  return parse_exp_in_context_1 (stringptr, pc, block, comma,
-				 void_context_p, out_subexp);
+  return parse_exp_in_context (stringptr, pc, block, comma, 0, NULL,
+			       tracker_types);
 }
 
 /* As for parse_exp_1, except that if VOID_CONTEXT_P, then
@@ -1117,9 +1107,10 @@ parse_exp_in_context (const char **stringptr, CORE_ADDR pc,
    is left untouched.  */
 
 static expression_up
-parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
-			const struct block *block,
-			int comma, int void_context_p, int *out_subexp)
+parse_exp_in_context (const char **stringptr, CORE_ADDR pc,
+		      const struct block *block,
+		      int comma, int void_context_p, int *out_subexp,
+		      innermost_block_tracker_types tracker_types)
 {
   const struct language_defn *lang = NULL;
   int subexp;
@@ -1132,6 +1123,7 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
   expout_last_struct = -1;
   expout_tag_completion_type = TYPE_CODE_UNDEF;
   expout_completion_name.reset ();
+  innermost_block.reset (tracker_types);
 
   comma_terminates = comma;
 
@@ -1286,7 +1278,8 @@ parse_expression_for_completion (const char *string,
   TRY
     {
       parse_completion = 1;
-      exp = parse_exp_in_context (&string, 0, 0, 0, 0, &subexp);
+      exp = parse_exp_in_context (&string, 0, 0, 0, 0, &subexp,
+				  INNERMOST_BLOCK_FOR_SYMBOLS);
     }
   CATCH (except, RETURN_MASK_ERROR)
     {

gdb/parser-defs.h

@@ -75,24 +75,6 @@ extern const struct block *expression_context_block;
    then look up the macro definitions active at that point.  */
 extern CORE_ADDR expression_context_pc;
 
-/* While parsing expressions we need to track the innermost lexical block
-   that we encounter.  In some situations we need to track the innermost
-   block just for symbols, and in other situations we want to track the
-   innermost block for symbols and registers.  These flags are used by the
-   innermost block tracker to control which blocks we consider for the
-   innermost block.  These flags can be combined together as needed.  */
-
-enum innermost_block_tracker_type
-{
-  /* Track the innermost block for symbols within an expression.  */
-  INNERMOST_BLOCK_FOR_SYMBOLS = (1 << 0),
-
-  /* Track the innermost block for registers within an expression.  */
-  INNERMOST_BLOCK_FOR_REGISTERS = (1 << 1)
-};
-DEF_ENUM_FLAGS_TYPE (enum innermost_block_tracker_type,
-		     innermost_block_tracker_types);
-
 /* When parsing expressions we track the innermost block that was
    referenced.  */
 
@@ -146,8 +128,9 @@ class innermost_block_tracker
 };
 
 /* The innermost context required by the stack and register variables
-   we've encountered so far.  This should be cleared before parsing an
-   expression, and queried once the parse is complete.  */
+   we've encountered so far.  This is cleared by the expression
+   parsing functions before parsing an expression, and can queried
+   once the parse is complete.  */
 extern innermost_block_tracker innermost_block;
 
 /* Number of arguments seen so far in innermost function call.  */

gdb/printcmd.c

@@ -1715,7 +1715,6 @@ display_command (const char *arg, int from_tty)
       fmt.raw = 0;
     }
 
-  innermost_block.reset ();
   expression_up expr = parse_expression (exp);
 
   newobj = new display ();
@@ -1883,7 +1882,6 @@ do_one_display (struct display *d)
 
       TRY
 	{
-	  innermost_block.reset ();
 	  d->exp = parse_expression (d->exp_string);
 	  d->block = innermost_block.block ();
 	}

gdb/symfile.c

@@ -2901,12 +2901,6 @@ clear_symtab_users (symfile_add_flags add_flags)
   clear_pc_function_cache ();
   gdb::observers::new_objfile.notify (NULL);
 
-  /* Clear globals which might have pointed into a removed objfile.
-     FIXME: It's not clear which of these are supposed to persist
-     between expressions and which ought to be reset each time.  */
-  expression_context_block = NULL;
-  innermost_block.reset ();
-
   /* Varobj may refer to old symbols, perform a cleanup.  */
   varobj_invalidate ();
 

gdb/varobj.c

@@ -309,13 +309,13 @@ varobj_create (const char *objname,
 	}
 
       p = expression;
-      innermost_block.reset (INNERMOST_BLOCK_FOR_SYMBOLS
-			     | INNERMOST_BLOCK_FOR_REGISTERS);
       /* Wrap the call to parse expression, so we can 
          return a sensible error.  */
       TRY
 	{
-	  var->root->exp = parse_exp_1 (&p, pc, block, 0);
+	  var->root->exp = parse_exp_1 (&p, pc, block, 0,
+					INNERMOST_BLOCK_FOR_SYMBOLS
+					| INNERMOST_BLOCK_FOR_REGISTERS);
 	}
 
       CATCH (except, RETURN_MASK_ERROR)