[PATCH] Extract symbol-writing function from parsers

[PATCH] Extract symbol-writing function from parsers

[Tom Tromey]

I noticed that several parsers shared the same code to write a symbol
reference to an expression.  This patch factors this code out into a
new function.

Regression tested on x86-64 Fedora 28.

gdb/ChangeLog
2020-12-13  Tom Tromey  <tom@tromey.com>

	* parser-defs.h (write_exp_symbol_reference): Declare.
	* parse.c (write_exp_symbol_reference): New function.
	* p-exp.y (variable): Use write_exp_symbol_reference.
	* m2-exp.y (variable): Use write_exp_symbol_reference.
	* f-exp.y (variable): Use write_exp_symbol_reference.
	* d-exp.y (PrimaryExpression): Use write_exp_symbol_reference.
	* c-exp.y (variable): Use write_exp_symbol_reference.
---
 gdb/ChangeLog     | 10 ++++++++++
 gdb/c-exp.y       | 24 +++---------------------
 gdb/d-exp.y       | 45 ++++++++++++++++-----------------------------
 gdb/f-exp.y       | 29 +++--------------------------
 gdb/m2-exp.y      | 30 ++++--------------------------
 gdb/p-exp.y       | 23 ++---------------------
 gdb/parse.c       | 25 +++++++++++++++++++++++++
 gdb/parser-defs.h | 10 ++++++++++
 8 files changed, 73 insertions(+), 123 deletions(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 8fd565abbb5..0bfb549b6d7 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1104,30 +1104,12 @@ variable:	qualified_name
 	|	COLONCOLON name_not_typename
 			{
 			  std::string name = copy_name ($2.stoken);
-			  struct symbol *sym;
-			  struct bound_minimal_symbol msymbol;
-
-			  sym
+			  struct symbol *sym
 			    = lookup_symbol (name.c_str (),
 					     (const struct block *) NULL,
 					     VAR_DOMAIN, NULL).symbol;
-			  if (sym)
-			    {
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      write_exp_elt_block (pstate, NULL);
-			      write_exp_elt_sym (pstate, sym);
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      break;
-			    }
-
-			  msymbol = lookup_bound_minimal_symbol (name.c_str ());
-			  if (msymbol.minsym != NULL)
-			    write_exp_msymbol (pstate, msymbol);
-			  else if (!have_full_symbols () && !have_partial_symbols ())
-			    error (_("No symbol table is loaded.  Use the \"file\" command."));
-			  else
-			    error (_("No symbol \"%s\" in current context."),
-				   name.c_str ());
+			  write_exp_symbol_reference (pstate, name.c_str (),
+						      sym);
 			}
 	;
 
diff --git a/gdb/d-exp.y b/gdb/d-exp.y
index 9d9342bce12..3b3d726221f 100644
--- a/gdb/d-exp.y
+++ b/gdb/d-exp.y
@@ -464,7 +464,6 @@ PrimaryExpression:
 			     been resolved, it's not likely to be found.  */
 			  if (type->code () == TYPE_CODE_MODULE)
 			    {
-			      struct bound_minimal_symbol msymbol;
 			      struct block_symbol sym;
 			      const char *type_name = TYPE_SAFE_NAME (type);
 			      int type_name_len = strlen (type_name);
@@ -477,35 +476,23 @@ PrimaryExpression:
 				lookup_symbol (name.c_str (),
 					       (const struct block *) NULL,
 					       VAR_DOMAIN, NULL);
-			      if (sym.symbol)
-				{
-				  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-				  write_exp_elt_block (pstate, sym.block);
-				  write_exp_elt_sym (pstate, sym.symbol);
-				  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-				  break;
-				}
-
-			      msymbol = lookup_bound_minimal_symbol (name.c_str ());
-			      if (msymbol.minsym != NULL)
-				write_exp_msymbol (pstate, msymbol);
-			      else if (!have_full_symbols () && !have_partial_symbols ())
-				error (_("No symbol table is loaded.  Use the \"file\" command."));
-			      else
-				error (_("No symbol \"%s\" in current context."),
-				       name.c_str ());
+			      write_exp_symbol_reference (pstate,
+							  name.c_str (),
+							  sym.symbol);
+			    }
+			  else
+			    {
+			      /* Check if the qualified name resolves as a member
+				 of an aggregate or an enum type.  */
+			      if (!type_aggregate_p (type))
+				error (_("`%s' is not defined as an aggregate type."),
+				       TYPE_SAFE_NAME (type));
+
+			      write_exp_elt_opcode (pstate, OP_SCOPE);
+			      write_exp_elt_type (pstate, type);
+			      write_exp_string (pstate, $3);
+			      write_exp_elt_opcode (pstate, OP_SCOPE);
 			    }
-
-			  /* Check if the qualified name resolves as a member
-			     of an aggregate or an enum type.  */
-			  if (!type_aggregate_p (type))
-			    error (_("`%s' is not defined as an aggregate type."),
-				   TYPE_SAFE_NAME (type));
-
-			  write_exp_elt_opcode (pstate, OP_SCOPE);
-			  write_exp_elt_type (pstate, type);
-			  write_exp_string (pstate, $3);
-			  write_exp_elt_opcode (pstate, OP_SCOPE);
 			}
 |	DOLLAR_VARIABLE
 		{ write_dollar_variable (pstate, $1); }
diff --git a/gdb/f-exp.y b/gdb/f-exp.y
index ea32056f6db..7343efc1d11 100644
--- a/gdb/f-exp.y
+++ b/gdb/f-exp.y
@@ -540,32 +540,9 @@ exp	:	STRING_LITERAL
 
 variable:	name_not_typename
 			{ struct block_symbol sym = $1.sym;
-
-			  if (sym.symbol)
-			    {
-			      if (symbol_read_needs_frame (sym.symbol))
-				pstate->block_tracker->update (sym);
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      write_exp_elt_block (pstate, sym.block);
-			      write_exp_elt_sym (pstate, sym.symbol);
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      break;
-			    }
-			  else
-			    {
-			      struct bound_minimal_symbol msymbol;
-			      std::string arg = copy_name ($1.stoken);
-
-			      msymbol =
-				lookup_bound_minimal_symbol (arg.c_str ());
-			      if (msymbol.minsym != NULL)
-				write_exp_msymbol (pstate, msymbol);
-			      else if (!have_full_symbols () && !have_partial_symbols ())
-				error (_("No symbol table is loaded.  Use the \"file\" command."));
-			      else
-				error (_("No symbol \"%s\" in current context."),
-				       arg.c_str ());
-			    }
+			  std::string name = copy_name ($1.stoken);
+			  write_exp_symbol_reference (pstate, name.c_str (),
+						      sym.symbol);
 			}
 	;
 
diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y
index d4f3c2c1e45..86be38cea8a 100644
--- a/gdb/m2-exp.y
+++ b/gdb/m2-exp.y
@@ -561,37 +561,15 @@ variable:	NAME
 			{ struct block_symbol sym;
 			  struct field_of_this_result is_a_field_of_this;
 
+			  std::string name = copy_name ($1);
 			  sym
-			    = lookup_symbol (copy_name ($1).c_str (),
+			    = lookup_symbol (name.c_str (),
 					     pstate->expression_context_block,
 					     VAR_DOMAIN,
 					     &is_a_field_of_this);
 
-			  if (sym.symbol)
-			    {
-			      if (symbol_read_needs_frame (sym.symbol))
-				pstate->block_tracker->update (sym);
-
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      write_exp_elt_block (pstate, sym.block);
-			      write_exp_elt_sym (pstate, sym.symbol);
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			    }
-			  else
-			    {
-			      struct bound_minimal_symbol msymbol;
-			      std::string arg = copy_name ($1);
-
-			      msymbol =
-				lookup_bound_minimal_symbol (arg.c_str ());
-			      if (msymbol.minsym != NULL)
-				write_exp_msymbol (pstate, msymbol);
-			      else if (!have_full_symbols () && !have_partial_symbols ())
-				error (_("No symbol table is loaded.  Use the \"symbol-file\" command."));
-			      else
-				error (_("No symbol \"%s\" in current context."),
-				       arg.c_str ());
-			    }
+			  write_exp_symbol_reference (pstate, name.c_str (),
+						      sym.symbol);
 			}
 	;
 
diff --git a/gdb/p-exp.y b/gdb/p-exp.y
index 618557c472a..3e8ab1fa72e 100644
--- a/gdb/p-exp.y
+++ b/gdb/p-exp.y
@@ -693,32 +693,13 @@ variable:	qualified_name
 			{
 			  std::string name = copy_name ($2);
 			  struct symbol *sym;
-			  struct bound_minimal_symbol msymbol;
 
 			  sym =
 			    lookup_symbol (name.c_str (),
 					   (const struct block *) NULL,
 					   VAR_DOMAIN, NULL).symbol;
-			  if (sym)
-			    {
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      write_exp_elt_block (pstate, NULL);
-			      write_exp_elt_sym (pstate, sym);
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      break;
-			    }
-
-			  msymbol
-			    = lookup_bound_minimal_symbol (name.c_str ());
-			  if (msymbol.minsym != NULL)
-			    write_exp_msymbol (pstate, msymbol);
-			  else if (!have_full_symbols ()
-				   && !have_partial_symbols ())
-			    error (_("No symbol table is loaded.  "
-				   "Use the \"file\" command."));
-			  else
-			    error (_("No symbol \"%s\" in current context."),
-				   name.c_str ());
+			  write_exp_symbol_reference (pstate, name.c_str (),
+						      sym);
 			}
 	;
 
diff --git a/gdb/parse.c b/gdb/parse.c
index 73d82e64a87..c99a82d6ebb 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -650,6 +650,31 @@ write_dollar_variable (struct parser_state *ps, struct stoken str)
   return;
 }
 
+/* See parser-defs.h.  */
+
+void
+write_exp_symbol_reference (struct parser_state *pstate, const char *name,
+			    struct symbol *sym)
+{
+  if (sym != nullptr)
+    {
+      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
+      write_exp_elt_block (pstate, NULL);
+      write_exp_elt_sym (pstate, sym);
+      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
+    }
+  else
+    {
+      struct bound_minimal_symbol msymbol = lookup_bound_minimal_symbol (name);
+      if (msymbol.minsym != NULL)
+	write_exp_msymbol (pstate, msymbol);
+      else if (!have_full_symbols () && !have_partial_symbols ())
+	error (_("No symbol table is loaded.  Use the \"file\" command."));
+      else
+	error (_("No symbol \"%s\" in current context."),
+	       name);
+    }
+}
 
 const char *
 find_template_name_end (const char *p)
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index 17216057b18..815903750f3 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -329,6 +329,16 @@ extern void write_exp_msymbol (struct expr_builder *,
 
 extern void write_dollar_variable (struct parser_state *, struct stoken str);
 
+/* Write a reference to a symbol to the expression being built in PS.
+   NAME is the name of the symbol to write; SYM is the symbol.  If SYM
+   is nullptr, a minimal symbol will be searched for and used if
+   available.  Throws an exception if SYM is nullptr and no minimal
+   symbol can be found.  */
+
+extern void write_exp_symbol_reference (struct parser_state *ps,
+					const char *name,
+					struct symbol *sym);
+
 extern const char *find_template_name_end (const char *);
 
 extern std::string copy_name (struct stoken);
-- 
2.17.2

Re: [PATCH] Extract symbol-writing function from parsers

[Simon Marchi]

On 2020-12-13 4:44 p.m., Tom Tromey wrote:
> I noticed that several parsers shared the same code to write a symbol
> reference to an expression.  This patch factors this code out into a
> new function.
>
> Regression tested on x86-64 Fedora 28.
>
> gdb/ChangeLog
> 2020-12-13  Tom Tromey  <tom@tromey.com>
>
> 	* parser-defs.h (write_exp_symbol_reference): Declare.
> 	* parse.c (write_exp_symbol_reference): New function.
> 	* p-exp.y (variable): Use write_exp_symbol_reference.
> 	* m2-exp.y (variable): Use write_exp_symbol_reference.
> 	* f-exp.y (variable): Use write_exp_symbol_reference.
> 	* d-exp.y (PrimaryExpression): Use write_exp_symbol_reference.
> 	* c-exp.y (variable): Use write_exp_symbol_reference.
> ---
>  gdb/ChangeLog     | 10 ++++++++++
>  gdb/c-exp.y       | 24 +++---------------------
>  gdb/d-exp.y       | 45 ++++++++++++++++-----------------------------
>  gdb/f-exp.y       | 29 +++--------------------------
>  gdb/m2-exp.y      | 30 ++++--------------------------
>  gdb/p-exp.y       | 23 ++---------------------
>  gdb/parse.c       | 25 +++++++++++++++++++++++++
>  gdb/parser-defs.h | 10 ++++++++++
>  8 files changed, 73 insertions(+), 123 deletions(-)
>
> diff --git a/gdb/c-exp.y b/gdb/c-exp.y
> index 8fd565abbb5..0bfb549b6d7 100644
> --- a/gdb/c-exp.y
> +++ b/gdb/c-exp.y
> @@ -1104,30 +1104,12 @@ variable:	qualified_name
>  	|	COLONCOLON name_not_typename
>  			{
>  			  std::string name = copy_name ($2.stoken);
> -			  struct symbol *sym;
> -			  struct bound_minimal_symbol msymbol;
> -
> -			  sym
> +			  struct symbol *sym
>  			    = lookup_symbol (name.c_str (),
>  					     (const struct block *) NULL,
>  					     VAR_DOMAIN, NULL).symbol;
> -			  if (sym)
> -			    {
> -			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
> -			      write_exp_elt_block (pstate, NULL);
> -			      write_exp_elt_sym (pstate, sym);
> -			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
> -			      break;
> -			    }
> -
> -			  msymbol = lookup_bound_minimal_symbol (name.c_str ());
> -			  if (msymbol.minsym != NULL)
> -			    write_exp_msymbol (pstate, msymbol);
> -			  else if (!have_full_symbols () && !have_partial_symbols ())
> -			    error (_("No symbol table is loaded.  Use the \"file\" command."));
> -			  else
> -			    error (_("No symbol \"%s\" in current context."),
> -				   name.c_str ());

You could write that on a single line, for a slightly cleaner look.

> +			  write_exp_symbol_reference (pstate, name.c_str (),
> +						      sym);
>  			}
>  	;
>
> diff --git a/gdb/d-exp.y b/gdb/d-exp.y
> index 9d9342bce12..3b3d726221f 100644
> --- a/gdb/d-exp.y
> +++ b/gdb/d-exp.y
> @@ -464,7 +464,6 @@ PrimaryExpression:
>  			     been resolved, it's not likely to be found.  */
>  			  if (type->code () == TYPE_CODE_MODULE)
>  			    {
> -			      struct bound_minimal_symbol msymbol;
>  			      struct block_symbol sym;
>  			      const char *type_name = TYPE_SAFE_NAME (type);
>  			      int type_name_len = strlen (type_name);
> @@ -477,35 +476,23 @@ PrimaryExpression:
>  				lookup_symbol (name.c_str (),
>  					       (const struct block *) NULL,
>  					       VAR_DOMAIN, NULL);
> -			      if (sym.symbol)
> -				{
> -				  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
> -				  write_exp_elt_block (pstate, sym.block);
> -				  write_exp_elt_sym (pstate, sym.symbol);
> -				  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
> -				  break;
> -				}
> -
> -			      msymbol = lookup_bound_minimal_symbol (name.c_str ());
> -			      if (msymbol.minsym != NULL)
> -				write_exp_msymbol (pstate, msymbol);
> -			      else if (!have_full_symbols () && !have_partial_symbols ())
> -				error (_("No symbol table is loaded.  Use the \"file\" command."));
> -			      else
> -				error (_("No symbol \"%s\" in current context."),
> -				       name.c_str ());
> +			      write_exp_symbol_reference (pstate,
> +							  name.c_str (),
> +							  sym.symbol);
> +			    }
> +			  else
> +			    {
> +			      /* Check if the qualified name resolves as a member
> +				 of an aggregate or an enum type.  */
> +			      if (!type_aggregate_p (type))
> +				error (_("`%s' is not defined as an aggregate type."),
> +				       TYPE_SAFE_NAME (type));
> +
> +			      write_exp_elt_opcode (pstate, OP_SCOPE);
> +			      write_exp_elt_type (pstate, type);
> +			      write_exp_string (pstate, $3);
> +			      write_exp_elt_opcode (pstate, OP_SCOPE);
>  			    }
> -
> -			  /* Check if the qualified name resolves as a member
> -			     of an aggregate or an enum type.  */
> -			  if (!type_aggregate_p (type))
> -			    error (_("`%s' is not defined as an aggregate type."),
> -				   TYPE_SAFE_NAME (type));
> -
> -			  write_exp_elt_opcode (pstate, OP_SCOPE);
> -			  write_exp_elt_type (pstate, type);
> -			  write_exp_string (pstate, $3);
> -			  write_exp_elt_opcode (pstate, OP_SCOPE);

The behavior seems to change a little bit here.  Imagine we go in
write_exp_msymbol (in the original code), we will then go into that
block that starts with "Check if the qualified name".  In the new code,
we won't.  I don't know if that's important.

>  			}
>  |	DOLLAR_VARIABLE
>  		{ write_dollar_variable (pstate, $1); }
> diff --git a/gdb/f-exp.y b/gdb/f-exp.y
> index ea32056f6db..7343efc1d11 100644
> --- a/gdb/f-exp.y
> +++ b/gdb/f-exp.y
> @@ -540,32 +540,9 @@ exp	:	STRING_LITERAL
>
>  variable:	name_not_typename
>  			{ struct block_symbol sym = $1.sym;
> -
> -			  if (sym.symbol)
> -			    {
> -			      if (symbol_read_needs_frame (sym.symbol))
> -				pstate->block_tracker->update (sym);

This if gets removed in the new code, is it important?  It's present in
m2 as well.

> -			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
> -			      write_exp_elt_block (pstate, sym.block);
> -			      write_exp_elt_sym (pstate, sym.symbol);
> -			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
> -			      break;
> -			    }
> -			  else
> -			    {
> -			      struct bound_minimal_symbol msymbol;
> -			      std::string arg = copy_name ($1.stoken);

This copy is kept in the new code.  Do you know if/why it's important?

Simon

Re: [PATCH] Extract symbol-writing function from parsers

[Tom Tromey]

[...]
Simon> The behavior seems to change a little bit here.  Imagine we go in
Simon> write_exp_msymbol (in the original code), we will then go into that
Simon> block that starts with "Check if the qualified name".  In the new code,
Simon> we won't.  I don't know if that's important.

I haven't looked into this yet, but I will.

>> -			      if (symbol_read_needs_frame (sym.symbol))
>> -				pstate->block_tracker->update (sym);

Simon> This if gets removed in the new code, is it important?  It's present in
Simon> m2 as well.

Thanks for noticing.
Parsers do need to update this, it's required for watchpoints.
So, I think you found a latent bug in c-exp.y.

>> -			      struct bound_minimal_symbol msymbol;
>> -			      std::string arg = copy_name ($1.stoken);

Simon> This copy is kept in the new code.  Do you know if/why it's important?

A 'struct stoken' doesn't guarantee that the name is \0-terminated, but
this property is needed by write_exp_symbol_reference.

Tom

Re: [PATCH] Extract symbol-writing function from parsers

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> The behavior seems to change a little bit here.  Imagine we go in
Simon> write_exp_msymbol (in the original code), we will then go into that
Simon> block that starts with "Check if the qualified name".  In the new code,
Simon> we won't.  I don't know if that's important.

I think the old code was wrong.  It would emit a minsym reference,
followed by an OP_SCOPE.  However, that doesn't mean anything and would
probably result in an invalid, or at least confusing, expression.

Tom

Re: [PATCH] Extract symbol-writing function from parsers

[Tom Tromey]

Here's an updated patch.

Tom

commit e3962882d01140283d0d241a0e0945154b9f595c
Author: Tom Tromey <tom@tromey.com>
Date:   Thu Jan 28 20:19:27 2021 -0700

    Extract symbol-writing function from parsers
    
    I noticed that several parsers shared the same code to write a symbol
    reference to an expression.  This patch factors this code out into a
    new function.
    
    Regression tested on x86-64 Fedora 32.
    
    gdb/ChangeLog
    2021-01-28  Tom Tromey  <tom@tromey.com>
    
            * parser-defs.h (write_exp_symbol_reference): Declare.
            * parse.c (write_exp_symbol_reference): New function.
            * p-exp.y (variable): Use write_exp_symbol_reference.
            * m2-exp.y (variable): Use write_exp_symbol_reference.
            * f-exp.y (variable): Use write_exp_symbol_reference.
            * d-exp.y (PrimaryExpression): Use write_exp_symbol_reference.
            * c-exp.y (variable): Use write_exp_symbol_reference.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1de9d30f051..7d356a63adb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2021-01-28  Tom Tromey  <tom@tromey.com>
+
+	* parser-defs.h (write_exp_symbol_reference): Declare.
+	* parse.c (write_exp_symbol_reference): New function.
+	* p-exp.y (variable): Use write_exp_symbol_reference.
+	* m2-exp.y (variable): Use write_exp_symbol_reference.
+	* f-exp.y (variable): Use write_exp_symbol_reference.
+	* d-exp.y (PrimaryExpression): Use write_exp_symbol_reference.
+	* c-exp.y (variable): Use write_exp_symbol_reference.
+
 2021-01-28  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* thread.c (thr_try_catch_cmd): Replace swith_to_thread with an
diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 71610e90968..13b06f39bbf 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1104,30 +1104,12 @@ variable:	qualified_name
 	|	COLONCOLON name_not_typename
 			{
 			  std::string name = copy_name ($2.stoken);
-			  struct symbol *sym;
-			  struct bound_minimal_symbol msymbol;
-
-			  sym
+			  struct block_symbol sym
 			    = lookup_symbol (name.c_str (),
 					     (const struct block *) NULL,
-					     VAR_DOMAIN, NULL).symbol;
-			  if (sym)
-			    {
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      write_exp_elt_block (pstate, NULL);
-			      write_exp_elt_sym (pstate, sym);
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      break;
-			    }
-
-			  msymbol = lookup_bound_minimal_symbol (name.c_str ());
-			  if (msymbol.minsym != NULL)
-			    write_exp_msymbol (pstate, msymbol);
-			  else if (!have_full_symbols () && !have_partial_symbols ())
-			    error (_("No symbol table is loaded.  Use the \"file\" command."));
-			  else
-			    error (_("No symbol \"%s\" in current context."),
-				   name.c_str ());
+					     VAR_DOMAIN, NULL);
+			  write_exp_symbol_reference (pstate, name.c_str (),
+						      sym);
 			}
 	;
 
diff --git a/gdb/d-exp.y b/gdb/d-exp.y
index 35cd07413d5..c432f22bd9d 100644
--- a/gdb/d-exp.y
+++ b/gdb/d-exp.y
@@ -464,7 +464,6 @@ PrimaryExpression:
 			     been resolved, it's not likely to be found.  */
 			  if (type->code () == TYPE_CODE_MODULE)
 			    {
-			      struct bound_minimal_symbol msymbol;
 			      struct block_symbol sym;
 			      const char *type_name = TYPE_SAFE_NAME (type);
 			      int type_name_len = strlen (type_name);
@@ -477,35 +476,23 @@ PrimaryExpression:
 				lookup_symbol (name.c_str (),
 					       (const struct block *) NULL,
 					       VAR_DOMAIN, NULL);
-			      if (sym.symbol)
-				{
-				  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-				  write_exp_elt_block (pstate, sym.block);
-				  write_exp_elt_sym (pstate, sym.symbol);
-				  write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-				  break;
-				}
-
-			      msymbol = lookup_bound_minimal_symbol (name.c_str ());
-			      if (msymbol.minsym != NULL)
-				write_exp_msymbol (pstate, msymbol);
-			      else if (!have_full_symbols () && !have_partial_symbols ())
-				error (_("No symbol table is loaded.  Use the \"file\" command."));
-			      else
-				error (_("No symbol \"%s\" in current context."),
-				       name.c_str ());
+			      write_exp_symbol_reference (pstate,
+							  name.c_str (),
+							  sym);
+			    }
+			  else
+			    {
+			      /* Check if the qualified name resolves as a member
+				 of an aggregate or an enum type.  */
+			      if (!type_aggregate_p (type))
+				error (_("`%s' is not defined as an aggregate type."),
+				       TYPE_SAFE_NAME (type));
+
+			      write_exp_elt_opcode (pstate, OP_SCOPE);
+			      write_exp_elt_type (pstate, type);
+			      write_exp_string (pstate, $3);
+			      write_exp_elt_opcode (pstate, OP_SCOPE);
 			    }
-
-			  /* Check if the qualified name resolves as a member
-			     of an aggregate or an enum type.  */
-			  if (!type_aggregate_p (type))
-			    error (_("`%s' is not defined as an aggregate type."),
-				   TYPE_SAFE_NAME (type));
-
-			  write_exp_elt_opcode (pstate, OP_SCOPE);
-			  write_exp_elt_type (pstate, type);
-			  write_exp_string (pstate, $3);
-			  write_exp_elt_opcode (pstate, OP_SCOPE);
 			}
 |	DOLLAR_VARIABLE
 		{ write_dollar_variable (pstate, $1); }
diff --git a/gdb/f-exp.y b/gdb/f-exp.y
index 3b0f23d5d73..92a70b4552d 100644
--- a/gdb/f-exp.y
+++ b/gdb/f-exp.y
@@ -540,32 +540,9 @@ exp	:	STRING_LITERAL
 
 variable:	name_not_typename
 			{ struct block_symbol sym = $1.sym;
-
-			  if (sym.symbol)
-			    {
-			      if (symbol_read_needs_frame (sym.symbol))
-				pstate->block_tracker->update (sym);
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      write_exp_elt_block (pstate, sym.block);
-			      write_exp_elt_sym (pstate, sym.symbol);
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      break;
-			    }
-			  else
-			    {
-			      struct bound_minimal_symbol msymbol;
-			      std::string arg = copy_name ($1.stoken);
-
-			      msymbol =
-				lookup_bound_minimal_symbol (arg.c_str ());
-			      if (msymbol.minsym != NULL)
-				write_exp_msymbol (pstate, msymbol);
-			      else if (!have_full_symbols () && !have_partial_symbols ())
-				error (_("No symbol table is loaded.  Use the \"file\" command."));
-			      else
-				error (_("No symbol \"%s\" in current context."),
-				       arg.c_str ());
-			    }
+			  std::string name = copy_name ($1.stoken);
+			  write_exp_symbol_reference (pstate, name.c_str (),
+						      sym);
 			}
 	;
 
diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y
index 717c6e69e95..68bae48feb6 100644
--- a/gdb/m2-exp.y
+++ b/gdb/m2-exp.y
@@ -561,37 +561,15 @@ variable:	NAME
 			{ struct block_symbol sym;
 			  struct field_of_this_result is_a_field_of_this;
 
+			  std::string name = copy_name ($1);
 			  sym
-			    = lookup_symbol (copy_name ($1).c_str (),
+			    = lookup_symbol (name.c_str (),
 					     pstate->expression_context_block,
 					     VAR_DOMAIN,
 					     &is_a_field_of_this);
 
-			  if (sym.symbol)
-			    {
-			      if (symbol_read_needs_frame (sym.symbol))
-				pstate->block_tracker->update (sym);
-
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      write_exp_elt_block (pstate, sym.block);
-			      write_exp_elt_sym (pstate, sym.symbol);
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			    }
-			  else
-			    {
-			      struct bound_minimal_symbol msymbol;
-			      std::string arg = copy_name ($1);
-
-			      msymbol =
-				lookup_bound_minimal_symbol (arg.c_str ());
-			      if (msymbol.minsym != NULL)
-				write_exp_msymbol (pstate, msymbol);
-			      else if (!have_full_symbols () && !have_partial_symbols ())
-				error (_("No symbol table is loaded.  Use the \"symbol-file\" command."));
-			      else
-				error (_("No symbol \"%s\" in current context."),
-				       arg.c_str ());
-			    }
+			  write_exp_symbol_reference (pstate, name.c_str (),
+						      sym);
 			}
 	;
 
diff --git a/gdb/p-exp.y b/gdb/p-exp.y
index 5a43e890524..b025ac36070 100644
--- a/gdb/p-exp.y
+++ b/gdb/p-exp.y
@@ -691,33 +691,12 @@ variable:	qualified_name
 	|	COLONCOLON name
 			{
 			  std::string name = copy_name ($2);
-			  struct symbol *sym;
-			  struct bound_minimal_symbol msymbol;
-
-			  sym =
-			    lookup_symbol (name.c_str (),
-					   (const struct block *) NULL,
-					   VAR_DOMAIN, NULL).symbol;
-			  if (sym)
-			    {
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      write_exp_elt_block (pstate, NULL);
-			      write_exp_elt_sym (pstate, sym);
-			      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
-			      break;
-			    }
 
-			  msymbol
-			    = lookup_bound_minimal_symbol (name.c_str ());
-			  if (msymbol.minsym != NULL)
-			    write_exp_msymbol (pstate, msymbol);
-			  else if (!have_full_symbols ()
-				   && !have_partial_symbols ())
-			    error (_("No symbol table is loaded.  "
-				   "Use the \"file\" command."));
-			  else
-			    error (_("No symbol \"%s\" in current context."),
-				   name.c_str ());
+			  struct block_symbol sym
+			    = lookup_symbol (name.c_str (), nullptr,
+					     VAR_DOMAIN, nullptr);
+			  write_exp_symbol_reference (pstate, name.c_str (),
+						      sym);
 			}
 	;
 
diff --git a/gdb/parse.c b/gdb/parse.c
index 933960fa30c..08fde89d8f3 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -650,6 +650,32 @@ write_dollar_variable (struct parser_state *ps, struct stoken str)
   return;
 }
 
+/* See parser-defs.h.  */
+
+void
+write_exp_symbol_reference (struct parser_state *pstate, const char *name,
+			    struct block_symbol sym)
+{
+  if (sym.symbol != nullptr)
+    {
+      if (symbol_read_needs_frame (sym.symbol))
+	pstate->block_tracker->update (sym);
+      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
+      write_exp_elt_block (pstate, NULL);
+      write_exp_elt_sym (pstate, sym.symbol);
+      write_exp_elt_opcode (pstate, OP_VAR_VALUE);
+    }
+  else
+    {
+      struct bound_minimal_symbol msymbol = lookup_bound_minimal_symbol (name);
+      if (msymbol.minsym != NULL)
+	write_exp_msymbol (pstate, msymbol);
+      else if (!have_full_symbols () && !have_partial_symbols ())
+	error (_("No symbol table is loaded.  Use the \"file\" command."));
+      else
+	error (_("No symbol \"%s\" in current context."), name);
+    }
+}
 
 const char *
 find_template_name_end (const char *p)
diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h
index 5ff5d522405..466230e4ad0 100644
--- a/gdb/parser-defs.h
+++ b/gdb/parser-defs.h
@@ -334,6 +334,16 @@ extern void write_exp_msymbol (struct expr_builder *,
 
 extern void write_dollar_variable (struct parser_state *, struct stoken str);
 
+/* Write a reference to a symbol to the expression being built in PS.
+   NAME is the name of the symbol to write; SYM is the symbol.  If SYM
+   is nullptr (meaning the 'symbol' member), a minimal symbol will be
+   searched for and used if available.  Throws an exception if SYM is
+   nullptr and no minimal symbol can be found.  */
+
+extern void write_exp_symbol_reference (struct parser_state *ps,
+					const char *name,
+					struct block_symbol sym);
+
 extern const char *find_template_name_end (const char *);
 
 extern std::string copy_name (struct stoken);

Re: [PATCH] Extract symbol-writing function from parsers

[Tom Tromey]

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

Tom> Here's an updated patch.
Tom> Tom

Tom> commit e3962882d01140283d0d241a0e0945154b9f595c
Tom> Author: Tom Tromey <tom@tromey.com>
Tom> Date:   Thu Jan 28 20:19:27 2021 -0700

Tom>     Extract symbol-writing function from parsers
    
Tom>     I noticed that several parsers shared the same code to write a symbol
Tom>     reference to an expression.  This patch factors this code out into a
Tom>     new function.
    
Tom>     Regression tested on x86-64 Fedora 32.

I re-regression tested this, and I am checking it in now.

Tom

Re: [PATCH] Extract symbol-writing function from parsers

[Luis Machado]

On 2/5/21 11:10 AM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> Here's an updated patch.
> Tom> Tom
> 
> Tom> commit e3962882d01140283d0d241a0e0945154b9f595c
> Tom> Author: Tom Tromey <tom@tromey.com>
> Tom> Date:   Thu Jan 28 20:19:27 2021 -0700
> 
> Tom>     Extract symbol-writing function from parsers
>      
> Tom>     I noticed that several parsers shared the same code to write a symbol
> Tom>     reference to an expression.  This patch factors this code out into a
> Tom>     new function.
>      
> Tom>     Regression tested on x86-64 Fedora 32.
> 
> I re-regression tested this, and I am checking it in now.
> 
> Tom
> 

This seems to be causing some regressions for fortran testcases on 
aarch64-linux/Ubuntu 20.04 (via git bisect):

FAIL: gdb.fortran/nested-funcs.exp: print index at BP_outer
FAIL: gdb.fortran/nested-funcs.exp: print index at BP1, one frame up
FAIL: gdb.fortran/nested-funcs.exp: print index at BP_inner
FAIL: gdb.fortran/nested-funcs.exp: print v_state%code at BP_inner
FAIL: gdb.fortran/nested-funcs.exp: print index at BP_main
FAIL: gdb.fortran/nested-funcs.exp: print v_state%code at BP_main
FAIL: gdb.fortran/nested-funcs-2.exp: src_prefix=0: nest_prefix=0: 
printing outer scoped variable
FAIL: gdb.fortran/nested-funcs-2.exp: src_prefix=0: nest_prefix=1: 
printing outer scoped variable
FAIL: gdb.fortran/nested-funcs-2.exp: src_prefix=1: nest_prefix=0: 
printing outer scoped variable
FAIL: gdb.fortran/nested-funcs-2.exp: src_prefix=1: nest_prefix=1: 
printing outer scoped variable

I filed a bug here: https://sourceware.org/bugzilla/show_bug.cgi?id=27383

Please let me know if you need some more data from aarch64-linux's or 
Ubuntu's side.