[PATCH] gdb: Allow struct fields named double

[PATCH] gdb: Allow struct fields named double

[Andrew Burgess]

The patch below fixes an issue with the current was floating point is
handled on RISC-V.  We currently model 64-bit floating point as a
union with fields 'double' and 'float'.  The problem is that having a
field named 'double' isn't currently valid in GDB (though float is fine).

The easiest solution might be to change the field names... but the
patch below instead extends the C parser to allow double as a field
name.

I'd be interested in hearing feedback on the below,

Thanks,
Andrew

---

The 64-bit RISC-V target currently models the floating point registers
as having the following type:

    union riscv_double
    {
        builtin_type_ieee_single float;
        builtin_type_ieee_double double;
    }

Notice the choice of names for the fields of this struct, possibly not
ideal choices, as these are not valid field names in C.  However, this
type is only ever defined within GDB (or in the target description),
and no restriction seems to exist on the field names in that case.

The problem though is that currently:

    (gdb) info registers $ft0
    ft0            {float = 0, double = 0}	(raw 0x0000000000000000)
    (gdb) p $ft0.float
    $1 = 0
    (gdb) p $ft0.double
    A syntax error in expression, near `double'.

We can access the 'float' field, but not the 'double' field.  This is
because the string 'double' is handled differently to the string
'float' in c-exp.y.

In both cases the string '$ft0' is parsed as a VARIABLE expression.

In the 'float' case, the string 'float' becomes a generic NAME token
in 'lex_one_token', which then allows the rule "exp '.' name" to match
and the field name lookup to occur.

The 'double' case is different.  In order to allow parsing of the type
string 'long double', the 'double' string becomes the token
DOUBLE_KEYWORD.  At this point there's no rule to match "exp '.'
DOUBLE_KEYWORD", so we can never lookup the field named 'double'.

We could rename the fields for RISC-V, and maybe that would be the
best solution.  However, its not hard to allow for fields named
'double', which is what this patch does.

A new case is added to the 'name' rule to match the DOUBLE_KEYWORD,
and create a suitable 'struct stoken'.  With this done the "exp '.'
name" pattern can now match, and we can lookup the double field.

With this patch in place I now see this behaviour:

    (gdb) info registers $ft0
    ft0            {float = 0, double = 0}	(raw 0x0000000000000000)
    (gdb) p $ft0.float
    $1 = 0
    (gdb) p $ft0.double
    $2 = 0

This change was tested on x86-64 GNU/Linux with no regressions.

gdb/ChangeLog:

	* c-exp.y (name): Allow DOUBLE_KEYWORD to act as a name.
	(typename_stoken): New function.
---
 gdb/ChangeLog |  5 +++++
 gdb/c-exp.y   | 17 +++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index bfc78415b22..8d805cbde3d 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -113,6 +113,7 @@ static int type_aggregate_p (struct type *);
 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 write_destructor_name (struct parser_state *par_state,
 				   struct stoken);
@@ -1642,6 +1643,7 @@ name	:	NAME { $$ = $1.stoken; }
 	|	TYPENAME { $$ = $1.stoken; }
 	|	NAME_OR_INT  { $$ = $1.stoken; }
 	|	UNKNOWN_CPP_NAME  { $$ = $1.stoken; }
+	|	DOUBLE_KEYWORD { $$ = typename_stoken ("double"); }
 	|	oper { $$ = $1; }
 	;
 
@@ -1707,6 +1709,21 @@ operator_stoken (const char *op)
   return st;
 };
 
+/* Returns a stoken of the type named TYPE.  */
+
+static struct stoken
+typename_stoken (const char *type)
+{
+  struct stoken st = { NULL, 0 };
+  char *buf = xstrdup (type);
+  st.ptr = buf;
+  st.length = strlen (type);
+
+  /* The toplevel (c_parse) will free the memory allocated here.  */
+  make_cleanup (free, buf);
+  return st;
+};
+
 /* Return true if the type is aggregate-like.  */
 
 static int
-- 
2.14.5

Re: [PATCH] gdb: Allow struct fields named double

[Simon Marchi]

On 2018-12-15 6:24 p.m., Andrew Burgess wrote:
> The patch below fixes an issue with the current was floating point is
> handled on RISC-V.  We currently model 64-bit floating point as a
> union with fields 'double' and 'float'.  The problem is that having a
> field named 'double' isn't currently valid in GDB (though float is fine).
> 
> The easiest solution might be to change the field names... but the
> patch below instead extends the C parser to allow double as a field
> name.
> 
> I'd be interested in hearing feedback on the below,
> 
> Thanks,
> Andrew
> 
> ---
> 
> The 64-bit RISC-V target currently models the floating point registers
> as having the following type:
> 
>     union riscv_double
>     {
>         builtin_type_ieee_single float;
>         builtin_type_ieee_double double;
>     }
> 
> Notice the choice of names for the fields of this struct, possibly not
> ideal choices, as these are not valid field names in C.  However, this
> type is only ever defined within GDB (or in the target description),
> and no restriction seems to exist on the field names in that case.
> 
> The problem though is that currently:
> 
>     (gdb) info registers $ft0
>     ft0            {float = 0, double = 0}	(raw 0x0000000000000000)
>     (gdb) p $ft0.float
>     $1 = 0
>     (gdb) p $ft0.double
>     A syntax error in expression, near `double'.
> 
> We can access the 'float' field, but not the 'double' field.  This is
> because the string 'double' is handled differently to the string
> 'float' in c-exp.y.
> 
> In both cases the string '$ft0' is parsed as a VARIABLE expression.
> 
> In the 'float' case, the string 'float' becomes a generic NAME token
> in 'lex_one_token', which then allows the rule "exp '.' name" to match
> and the field name lookup to occur.
> 
> The 'double' case is different.  In order to allow parsing of the type
> string 'long double', the 'double' string becomes the token
> DOUBLE_KEYWORD.  At this point there's no rule to match "exp '.'
> DOUBLE_KEYWORD", so we can never lookup the field named 'double'.
> 
> We could rename the fields for RISC-V, and maybe that would be the
> best solution.  However, its not hard to allow for fields named
> 'double', which is what this patch does.
> 
> A new case is added to the 'name' rule to match the DOUBLE_KEYWORD,
> and create a suitable 'struct stoken'.  With this done the "exp '.'
> name" pattern can now match, and we can lookup the double field.
> 
> With this patch in place I now see this behaviour:
> 
>     (gdb) info registers $ft0
>     ft0            {float = 0, double = 0}	(raw 0x0000000000000000)
>     (gdb) p $ft0.float
>     $1 = 0
>     (gdb) p $ft0.double
>     $2 = 0
> 
> This change was tested on x86-64 GNU/Linux with no regressions.
> 
> gdb/ChangeLog:
> 
> 	* c-exp.y (name): Allow DOUBLE_KEYWORD to act as a name.
> 	(typename_stoken): New function.

This looks reasonable to me, from the user point of view there is not reason
why x.double would be different from x.float.  With the same logic, we should
also allow x.int, x.short, etc.  But I'm fine with doing it on an as-needed basis.

This LGTM, with one thing below you might want to change.  I'm far from an expert
in parsers though, so please give others ~1 week to comment, and push then if you
don't receive additional feedback.

> ---
>  gdb/ChangeLog |  5 +++++
>  gdb/c-exp.y   | 17 +++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/gdb/c-exp.y b/gdb/c-exp.y
> index bfc78415b22..8d805cbde3d 100644
> --- a/gdb/c-exp.y
> +++ b/gdb/c-exp.y
> @@ -113,6 +113,7 @@ static int type_aggregate_p (struct type *);
>  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 write_destructor_name (struct parser_state *par_state,
>  				   struct stoken);
> @@ -1642,6 +1643,7 @@ name	:	NAME { $$ = $1.stoken; }
>  	|	TYPENAME { $$ = $1.stoken; }
>  	|	NAME_OR_INT  { $$ = $1.stoken; }
>  	|	UNKNOWN_CPP_NAME  { $$ = $1.stoken; }
> +	|	DOUBLE_KEYWORD { $$ = typename_stoken ("double"); }
>  	|	oper { $$ = $1; }
>  	;
>  
> @@ -1707,6 +1709,21 @@ operator_stoken (const char *op)
>    return st;
>  };
>  
> +/* Returns a stoken of the type named TYPE.  */
> +
> +static struct stoken
> +typename_stoken (const char *type)
> +{
> +  struct stoken st = { NULL, 0 };
> +  char *buf = xstrdup (type);
> +  st.ptr = buf;
> +  st.length = strlen (type);
> +
> +  /* The toplevel (c_parse) will free the memory allocated here.  */
> +  make_cleanup (free, buf);
> +  return st;
> +};

Unlike operator_stoken, I don't think we need to malloc anything here, we
could return { "double", strlen ("double") }.  At this point, we might not
need a dedicated function.

Simon

Re: [PATCH] gdb: Allow struct fields named double

[Tom Tromey]

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

Simon> This looks reasonable to me, from the user point of view there is not reason
Simon> why x.double would be different from x.float.  With the same logic, we should
Simon> also allow x.int, x.short, etc.  But I'm fine with doing it on an as-needed basis.

See also https://sourceware.org/bugzilla/show_bug.cgi?id=13368

Simon> This LGTM, with one thing below you might want to change.  I'm far from an expert
Simon> in parsers though, so please give others ~1 week to comment, and push then if you
Simon> don't receive additional feedback.

I think the main possible issue is if this introduces a new parser conflict.
Taking a glance at the uses of "name" in the grammar, though, it seems
safe enough.

Tom

[PATCHv2 3/4] gdb: Add new parser rule for structure field names

[Andrew Burgess]

Introduces a new rule in c-exp.y for matching structure field names.

This is a restructure in preparation for the next commit, this commit
shouldn't result in any user visible changes.

gdb/ChangeLog:

	* c-exp.y (field_name): New %token, and new rule.
	(exp): Replace uses of 'name' with 'field_name' where appropriate.
---
 gdb/ChangeLog |  5 +++++
 gdb/c-exp.y   | 13 ++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index d2198b1fdf7..b6b57c8c4d4 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -156,7 +156,7 @@ static void c_print_token (FILE *file, int type, YYSTYPE value);
 %token <voidval> COMPLETE
 %token <tsym> TYPENAME
 %token <theclass> CLASSNAME	/* ObjC Class name */
-%type <sval> name
+%type <sval> name field_name
 %type <svec> string_exp
 %type <ssym> name_not_typename
 %type <tsym> type_name
@@ -312,13 +312,13 @@ exp	:	ALIGNOF '(' type_exp ')'	%prec UNARY
 			{ write_exp_elt_opcode (pstate, UNOP_ALIGNOF); }
 	;
 
-exp	:	exp ARROW name
+exp	:	exp ARROW field_name
 			{ write_exp_elt_opcode (pstate, STRUCTOP_PTR);
 			  write_exp_string (pstate, $3);
 			  write_exp_elt_opcode (pstate, STRUCTOP_PTR); }
 	;
 
-exp	:	exp ARROW name COMPLETE
+exp	:	exp ARROW field_name COMPLETE
 			{ mark_struct_expression (pstate);
 			  write_exp_elt_opcode (pstate, STRUCTOP_PTR);
 			  write_exp_string (pstate, $3);
@@ -360,13 +360,13 @@ exp	:	exp ARROW_STAR exp
 			{ write_exp_elt_opcode (pstate, STRUCTOP_MPTR); }
 	;
 
-exp	:	exp '.' name
+exp	:	exp '.' field_name
 			{ write_exp_elt_opcode (pstate, STRUCTOP_STRUCT);
 			  write_exp_string (pstate, $3);
 			  write_exp_elt_opcode (pstate, STRUCTOP_STRUCT); }
 	;
 
-exp	:	exp '.' name COMPLETE
+exp	:	exp '.' field_name COMPLETE
 			{ mark_struct_expression (pstate);
 			  write_exp_elt_opcode (pstate, STRUCTOP_STRUCT);
 			  write_exp_string (pstate, $3);
@@ -1644,6 +1644,9 @@ oper:	OPERATOR NEW
 	;
 
 
+field_name
+	:	name
+	;
 
 name	:	NAME { $$ = $1.stoken; }
 	|	BLOCKNAME { $$ = $1.stoken; }
-- 
2.14.5

[PATCHv2 4/4] gdb: Allow struct fields named double

[Andrew Burgess]

The 64-bit RISC-V target currently models the floating point registers
as having the following type:

    union riscv_double
    {
        builtin_type_ieee_single float;
        builtin_type_ieee_double double;
    }

Notice the choice of names for the fields of this struct, possibly not
ideal choices, as these are not valid field names in C.  However, this
type is only ever defined within GDB (or in the target description),
and no restriction seems to exist on the field names in that case.

The problem though is that currently:

    (gdb) info registers $ft0
    ft0            {float = 0, double = 0}	(raw 0x0000000000000000)
    (gdb) p $ft0.float
    $1 = 0
    (gdb) p $ft0.double
    A syntax error in expression, near `double'.

We can access the 'float' field, but not the 'double' field.  This is
because the string 'double' is handled differently to the string
'float' in c-exp.y.

In both cases the string '$ft0' is parsed as a VARIABLE expression.

In the 'float' case, the string 'float' becomes a generic NAME token
in 'lex_one_token', which then allows the rule "exp '.' name" to match
and the field name lookup to occur.

The 'double' case is different.  In order to allow parsing of the type
string 'long double', the 'double' string becomes the token
DOUBLE_KEYWORD.  At this point there's no rule to match "exp '.'
DOUBLE_KEYWORD", so we can never lookup the field named 'double'.

We could rename the fields for RISC-V, and maybe that would be the
best solution.  However, its not hard to allow for fields named
'double', which is what this patch does.

A new case is added to the 'field_name' rule to match the
DOUBLE_KEYWORD, and create a suitable 'struct stoken'.  With this done
the "exp '.'  field_name" pattern can now match, and we can lookup the
double field.

With this patch in place I now see this behaviour:

    (gdb) info registers $ft0
    ft0            {float = 0, double = 0}	(raw 0x0000000000000000)
    (gdb) p $ft0.float
    $1 = 0
    (gdb) p $ft0.double
    $2 = 0

I've gone ahead and handled INT_KEYWORD, LONG, SHORT, SIGNED_KEYWORD,
and UNSIGNED as well within field_name.

I've added a new test for this functionality.

This change was tested on x86-64 GNU/Linux with no regressions.

gdb/ChangeLog:

	* c-exp.y (field_name): Allow DOUBLE_KEYWORD, INT_KEYWORD, LONG,
	SHORT, SIGNED_KEYWORD, and UNSIGNED tokens to act as a field
	names.
	(typename_stoken): New function.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/dw2-unusual-field-names.c: New file.
	* gdb.dwarf2/dw2-unusual-field-names.exp: New file.
---
 gdb/ChangeLog                                      |   7 ++
 gdb/c-exp.y                                        |  25 +++-
 gdb/testsuite/ChangeLog                            |   5 +
 gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.c |  32 +++++
 .../gdb.dwarf2/dw2-unusual-field-names.exp         | 132 +++++++++++++++++++++
 5 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.exp

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index b6b57c8c4d4..44e8eeadd7a 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -113,6 +113,7 @@ static int type_aggregate_p (struct type *);
 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 write_destructor_name (struct parser_state *par_state,
 				   struct stoken);
@@ -1643,9 +1644,21 @@ oper:	OPERATOR NEW
 			}
 	;
 
-
+/* This rule exists in order to allow some tokens that would not normally
+   match the 'name' rule to appear as fields within a struct.  The example
+   that initially motivated this was the RISC-V target which models the
+   floating point registers as a union with fields called 'float' and
+   'double'.  The 'float' string becomes a TYPENAME token and can appear
+   anywhere a 'name' can, however 'double' is its own token,
+   DOUBLE_KEYWORD, and doesn't match the 'name' rule.*/
 field_name
 	:	name
+	|	DOUBLE_KEYWORD { $$ = typename_stoken ("double"); }
+	|	INT_KEYWORD { $$ = typename_stoken ("int"); }
+	|	LONG { $$ = typename_stoken ("long"); }
+	|	SHORT { $$ = typename_stoken ("short"); }
+	|	SIGNED_KEYWORD { $$ = typename_stoken ("signed"); }
+	|	UNSIGNED { $$ = typename_stoken ("unsigned"); }
 	;
 
 name	:	NAME { $$ = $1.stoken; }
@@ -1718,6 +1731,16 @@ operator_stoken (const char *op)
   return st;
 };
 
+/* Returns a stoken of the type named TYPE.  */
+
+static struct stoken
+typename_stoken (const char *type)
+{
+  struct stoken st = { type, 0 };
+  st.length = strlen (type);
+  return st;
+};
+
 /* Return true if the type is aggregate-like.  */
 
 static int
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.c b/gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.c
new file mode 100644
index 00000000000..091e475fb3f
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.c
@@ -0,0 +1,32 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+struct foo
+{
+  int field;
+};
+
+struct foo obj = { 0 };
+
+struct foo *ptr = &obj;
+
+int
+main (void)
+{
+  return obj.field;
+}
+
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.exp b/gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.exp
new file mode 100644
index 00000000000..6443acee249
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.exp
@@ -0,0 +1,132 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that GDB can support accesing fields of a structure if the
+# fields have non-standard names.  Specifically, if the names are
+# reserved C type names like 'double', 'float', 'int', etc.
+#
+# We don't expect to that such structures should be seen in real C
+# code, but in some cases GDB will generate artificial structures, and
+# in some cases, these type names are the obvious choice for field
+# names.
+#
+# One specific example is RISC-V, the 64-bit floating point registers
+# are represented as a structure with the field names 'float' and
+# 'double'.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use
+# gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile dw2-unusual-field-names.c dw2-unusual-field-names.S
+set asm_file [standard_output_file $srcfile2]
+
+# We need to know the size of integer and address types in order to
+# write some of the debugging info we'd like to generate.
+#
+# For that, we ask GDB by debugging our test program.  Any program
+# would do, but since we already have one specifically for this
+# testcase, might as well use that.
+if [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] {
+    return -1
+}
+set int_size [get_sizeof "int" -1]
+
+# Rebuild the test binary with the single field within the structure
+# renamed to FIELD_NAME, then test that we can access the field
+# through '.' and through '->'.
+proc run_test { field_name } {
+    global asm_file testfile srcfile subdir srcdir
+    global int_size
+
+    Dwarf::assemble $asm_file {
+	global srcdir subdir srcfile
+	global field_name int_size
+
+	cu {} {
+	    DW_TAG_compile_unit {
+                {DW_AT_language @DW_LANG_C}
+                {DW_AT_name     dw2-unusual-field-names.c}
+                {DW_AT_comp_dir /tmp}
+	    } {
+		declare_labels itype ptype stype
+
+		itype: DW_TAG_base_type {
+		    {DW_AT_byte_size $int_size DW_FORM_sdata}
+		    {DW_AT_encoding  @DW_ATE_signed}
+		    {DW_AT_name int}
+		}
+
+		stype: DW_TAG_structure_type {
+		    {DW_AT_name "foo"}
+		    {DW_AT_byte_size $int_size DW_FORM_sdata}
+		} {
+		    member {
+			{name $field_name}
+			{type :$itype}
+			{data_member_location 0 data1}
+		    }
+		}
+
+		ptype: DW_TAG_pointer_type {
+		    {DW_AT_type :$stype}
+		}
+
+		DW_TAG_variable {
+		    {DW_AT_name obj}
+		    {DW_AT_type :$stype}
+		    {DW_AT_location {
+			DW_OP_addr [gdb_target_symbol obj]
+		    } SPECIAL_expr}
+		    {external 1 flag}
+		}
+
+		DW_TAG_variable {
+		    {DW_AT_name ptr}
+		    {DW_AT_type :$ptype}
+		    {DW_AT_location {
+			DW_OP_addr [gdb_target_symbol ptr]
+		    } SPECIAL_expr}
+		    {external 1 flag}
+		}
+	    }
+	}
+    }
+
+    if { [prepare_for_testing "failed to prepare" ${testfile} \
+	      [list $srcfile $asm_file] {nodebug}] } {
+	return -1
+    }
+
+    if ![runto_main] {
+	return -1
+    }
+
+    gdb_test "p obj.$field_name" " = 0" \
+	"access a field named '$field_name' directly"
+
+    gdb_test "p ptr->$field_name" " = 0" \
+	"access a field named '$field_name' through a pointer"
+
+    gdb_exit
+}
+
+foreach field_name { double float char byte long int short unsigned signed } {
+    run_test $field_name
+}
-- 
2.14.5

[PATCHv2 0/4] gdb: Allow struct fields named double

[Andrew Burgess]

Simon, Tom,

Thanks for the feedback.

* Tom Tromey <tom@tromey.com> [2018-12-16 09:42:15 -0700]:

> >>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> This looks reasonable to me, from the user point of view there is not reason
> Simon> why x.double would be different from x.float.  With the same logic, we should
> Simon> also allow x.int, x.short, etc.  But I'm fine with doing it on an as-needed basis.
> 
> See also https://sourceware.org/bugzilla/show_bug.cgi?id=13368

The new patch series includes a patch that extends some of the
comments in order to hopefully address this bug.

> 
> Simon> This LGTM, with one thing below you might want to change.  I'm far from an expert
> Simon> in parsers though, so please give others ~1 week to comment, and push then if you
> Simon> don't receive additional feedback.
> 
> I think the main possible issue is if this introduces a new parser conflict.
> Taking a glance at the uses of "name" in the grammar, though, it seems
> safe enough.

I took a better look at how the YACC step of the compile works, and it
turns out that parser conflicts are not fatal to the build, and my
change had introduced a new conflict.

The new patch series addresses this by focusing my change specifically
on structure field names, so I want to be able to parse:
'object.double', but I don't expect to be able to parse an object
named 'double'.  I also extended my patch to cover other types like
'int', 'short', etc.

As penance I've included a patch that tweaks how pointers are parsed
that resolves 49 reduce/reduce conflicts.

After this series there's still 42 shift/reduce conflicts, and 4
reduce/reduce conflicts in the C parser, but patch #4, the one I
really care about, doesn't introduce any new conflicts.

Tested on X86-64 GNU/Linux.

Thanks,
Andrew

---

Andrew Burgess (4):
  gdb: Extend the comments in c-exp.y
  gdb: Resolve 49 reduce/reduce conflicts in c-exp.y
  gdb: Add new parser rule for structure field names
  gdb: Allow struct fields named double

 gdb/ChangeLog                                      |  24 ++++
 gdb/c-exp.y                                        |  73 +++++++++---
 gdb/testsuite/ChangeLog                            |   5 +
 gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.c |  32 +++++
 .../gdb.dwarf2/dw2-unusual-field-names.exp         | 132 +++++++++++++++++++++
 5 files changed, 248 insertions(+), 18 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-unusual-field-names.exp

-- 
2.14.5

[PATCHv2 1/4] gdb: Extend the comments in c-exp.y

[Andrew Burgess]

In an attempt to fix PR gdb/13368 this commit adds some comments to
c-exp.y which hopefully makes the type parsing code a little clearer.
There are no code changes here, so there should be no user visible
changes after this commit.

gdb/ChangeLog:

	PR gdb/13368
	* c-exp.y (typebase): Extend the comment.
	(ident_tokens): Likewise.
---
 gdb/ChangeLog |  6 ++++++
 gdb/c-exp.y   | 17 +++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index bfc78415b22..447ac78eee1 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1224,7 +1224,17 @@ func_mod:	'(' ')'
 type	:	ptype
 	;
 
-typebase  /* Implements (approximately): (type-qualifier)* type-specifier */
+/* Implements (approximately): (type-qualifier)* type-specifier.
+
+   When type-specifier is only ever a single word, like 'float' then these
+   arrive as pre-built TYPENAME tokens thanks to the classify_name
+   function.  However, when a type-specifier can contain multiple words,
+   for example 'double' can appear as just 'double' or 'long double', and
+   similarly 'long' can appear as just 'long' or in 'long double', then
+   these type-specifiers are parsed into their own tokens in the function
+   lex_one_token and the ident_tokens array.  These separate tokens are all
+   recognised here.  */
+typebase
 	:	TYPENAME
 			{ $$ = $1.type; }
 	|	INT_KEYWORD
@@ -2323,7 +2333,10 @@ static const struct token tokentab2[] =
     {".*", DOT_STAR, BINOP_END, FLAG_CXX}
   };
 
-/* Identifier-like tokens.  */
+/* Identifier-like tokens.  Only type-specifiers than can appear in
+   multi-word type names (for example 'double' can appear in 'long
+   double') need to be listed here.  type-specifiers that are only ever
+   single word (like 'float') are handled by the classify_name function.  */
 static const struct token ident_tokens[] =
   {
     {"unsigned", UNSIGNED, OP_NULL, 0},
-- 
2.14.5

[PATCHv2 2/4] gdb: Resolve 49 reduce/reduce conflicts in c-exp.y

[Andrew Burgess]

This commit splits the ptr_operator rule in c-exp.y in two, resolving
49 reduce/reduce conflicts in the process.  There should be no
user visible change after this commit.

There are still 42 shift/reduce and 4 reduce/reduce conflicts
remaining in c-exp.y (using GNU bison 3.0.4).

gdb/ChangeLog:

	* c-exp.y (base_ptr_operator): New rule, with non-recursive
	content taken from...
	(ptr_operator): ...here, leaving just the recursion here.
---
 gdb/ChangeLog |  6 ++++++
 gdb/c-exp.y   | 18 ++++++++----------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 447ac78eee1..d2198b1fdf7 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1136,21 +1136,19 @@ const_or_volatile_or_space_identifier:
 	|
 	;
 
-ptr_operator:
-		ptr_operator '*'
+base_ptr_operator
+	:	'*'
 			{ insert_type (tp_pointer); }
-		const_or_volatile_or_space_identifier
-	|	'*'
-			{ insert_type (tp_pointer); }
-		const_or_volatile_or_space_identifier
+			const_or_volatile_or_space_identifier
 	|	'&'
 			{ insert_type (tp_reference); }
-	|	'&' ptr_operator
-			{ insert_type (tp_reference); }
 	|       ANDAND
 			{ insert_type (tp_rvalue_reference); }
-	|       ANDAND ptr_operator
-			{ insert_type (tp_rvalue_reference); }
+	;
+
+ptr_operator
+	:	base_ptr_operator
+	|	base_ptr_operator base_ptr_operator
 	;
 
 ptr_operator_ts: ptr_operator
-- 
2.14.5

Re: [PATCHv2 1/4] gdb: Extend the comments in c-exp.y

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> In an attempt to fix PR gdb/13368 this commit adds some comments to
Andrew> c-exp.y which hopefully makes the type parsing code a little clearer.
Andrew> There are no code changes here, so there should be no user visible
Andrew> changes after this commit.

Andrew> gdb/ChangeLog:

Andrew> 	PR gdb/13368
Andrew> 	* c-exp.y (typebase): Extend the comment.
Andrew> 	(ident_tokens): Likewise.

Thanks, this is ok.

Tom

Re: [PATCHv2 2/4] gdb: Resolve 49 reduce/reduce conflicts in c-exp.y

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> This commit splits the ptr_operator rule in c-exp.y in two, resolving
Andrew> 49 reduce/reduce conflicts in the process.  There should be no
Andrew> user visible change after this commit.

Andrew> There are still 42 shift/reduce and 4 reduce/reduce conflicts
Andrew> remaining in c-exp.y (using GNU bison 3.0.4).

Andrew> gdb/ChangeLog:

Andrew> 	* c-exp.y (base_ptr_operator): New rule, with non-recursive
Andrew> 	content taken from...
Andrew> 	(ptr_operator): ...here, leaving just the recursion here.

Andrew> -ptr_operator:
Andrew> -		ptr_operator '*'
Andrew> +base_ptr_operator
Andrew> +	:	'*'
Andrew>  			{ insert_type (tp_pointer); }
Andrew> -		const_or_volatile_or_space_identifier
Andrew> -	|	'*'
Andrew> -			{ insert_type (tp_pointer); }
Andrew> -		const_or_volatile_or_space_identifier
Andrew> +			const_or_volatile_or_space_identifier
Andrew>  	|	'&'
Andrew>  			{ insert_type (tp_reference); }
Andrew> -	|	'&' ptr_operator
Andrew> -			{ insert_type (tp_reference); }
Andrew>  	|       ANDAND
Andrew>  			{ insert_type (tp_rvalue_reference); }
Andrew> -	|       ANDAND ptr_operator
Andrew> -			{ insert_type (tp_rvalue_reference); }

I don't really understand the previous code here -- in particular why
the '*' case puts ptr_operator first, while the '&' and '&&' cases put
it second.

Does this patch change the order of calls to insert_type?  I find it
hard to reason about what is going on here, so some assurance that it is
all ok would be good.

According to my --coverage build, the "'&' ptr_operator" and "ANDAND ptr_operator"
rules are not exercised by the test suite.  So, if there is a change, it
wouldn't necessarily be noticed :(

Tom

Re: [PATCHv2 3/4] gdb: Add new parser rule for structure field names

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Introduces a new rule in c-exp.y for matching structure field names.
Andrew> This is a restructure in preparation for the next commit, this commit
Andrew> shouldn't result in any user visible changes.

Andrew> gdb/ChangeLog:

Andrew> 	* c-exp.y (field_name): New %token, and new rule.
Andrew> 	(exp): Replace uses of 'name' with 'field_name' where appropriate.

This is ok.  Thanks.

Tom

Re: [PATCHv2 4/4] gdb: Allow struct fields named double

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> gdb/ChangeLog:

Andrew> 	* c-exp.y (field_name): Allow DOUBLE_KEYWORD, INT_KEYWORD, LONG,
Andrew> 	SHORT, SIGNED_KEYWORD, and UNSIGNED tokens to act as a field
Andrew> 	names.
Andrew> 	(typename_stoken): New function.

Andrew> gdb/testsuite/ChangeLog:

Andrew> 	* gdb.dwarf2/dw2-unusual-field-names.c: New file.
Andrew> 	* gdb.dwarf2/dw2-unusual-field-names.exp: New file.

Thanks for doing this.  This is ok.  I wanted to mention that I think
it's a good idea to do this because the C language also serves as a
fallback either when language support isn't available or when one wants
to inspect things in a low-level way, and in this case it can be useful
to have some extensions to C.

Andrew> +foreach field_name { double float char byte long int short unsigned signed } {
Andrew> +    run_test $field_name
Andrew> +}

I'd like to verify that there aren't any duplicate test names resulting
from this approach.  If there aren't, this is fine; if there are, I
think run_test could be wrapped in a with_test_prefix.

thanks,
Tom

Re: [PATCHv2 0/4] gdb: Allow struct fields named double

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2018-12-18 22:40:09 +0000]:

> Simon, Tom,
> 
> Thanks for the feedback.
> 
> * Tom Tromey <tom@tromey.com> [2018-12-16 09:42:15 -0700]:
> 
> > >>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> > 
> > Simon> This looks reasonable to me, from the user point of view there is not reason
> > Simon> why x.double would be different from x.float.  With the same logic, we should
> > Simon> also allow x.int, x.short, etc.  But I'm fine with doing it on an as-needed basis.
> > 
> > See also https://sourceware.org/bugzilla/show_bug.cgi?id=13368
> 
> The new patch series includes a patch that extends some of the
> comments in order to hopefully address this bug.
> 
> > 
> > Simon> This LGTM, with one thing below you might want to change.  I'm far from an expert
> > Simon> in parsers though, so please give others ~1 week to comment, and push then if you
> > Simon> don't receive additional feedback.
> > 
> > I think the main possible issue is if this introduces a new parser conflict.
> > Taking a glance at the uses of "name" in the grammar, though, it seems
> > safe enough.
> 
> I took a better look at how the YACC step of the compile works, and it
> turns out that parser conflicts are not fatal to the build, and my
> change had introduced a new conflict.
> 
> The new patch series addresses this by focusing my change specifically
> on structure field names, so I want to be able to parse:
> 'object.double', but I don't expect to be able to parse an object
> named 'double'.  I also extended my patch to cover other types like
> 'int', 'short', etc.
> 
> As penance I've included a patch that tweaks how pointers are parsed
> that resolves 49 reduce/reduce conflicts.
> 
> After this series there's still 42 shift/reduce conflicts, and 4
> reduce/reduce conflicts in the C parser, but patch #4, the one I
> really care about, doesn't introduce any new conflicts.
> 
> Tested on X86-64 GNU/Linux.
> 

Tom,

Thanks for the feedback on the revised patch series.  I have gone
ahead and pushed 1, 3, and 4, dropping patch 2.

For patch #4 I confirmed there are no duplicate test names.

In patch #2, you are correct and that the type stack is parsed in a
slightly different order with the patch than before.  I don't know if
this is significant, but I certainly don't know that it's not.

I'm convinced that patch #2 could be made to work, but I don't have
time right now, so I'll add it to my ever-growing todo list.

Thanks,
Andrew