Re: Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Gaius Mulley via Gdb-patches <gdb-patches@sourceware.org> [2020-08-25 03:29:20 +0100]:

> 
> 
> Hi Tom,
> 
> here is a bugfix for Pr 26372 [Modula-2] Parsing of multi-subscript arrays.
> Also included is a dejagnu testcase.  No extra regressions are caused on
> Debian GNU/Linux Buster amd64, is this ok to apply?
> 
> regards,
> Gaius
> 
> 
> gdb/ChangeLog entry
> ===================
> 
> 2020-08-25  Gaius Mulley  <gaiusmod2@gmail.com>
> 
> 	* m2-exp.y: Rewrite array subscript rules to support multidimension
> 	array access.  (ArgumentList) replaces non_empty_arglist.
> 	* testsuite/gdb.modula2/multidim.y:  (New file).
> 	* testsuite/gdb.modula2/multidim.c:  (New file).

The testsuite has its own ChangeLog file so the entries for
testsuite/... should all be placed there.  You wrote 'multidim.y'
instead of 'multidim.exp', and the style '(New file)' is out of line
with our existing ChangeLog style.

Additionally, the ChangeLog entry should include the bug number for
the bug being fixed.

> 
> Patch
> =====

You should consider using 'git format-patch' for create patches for
the mailing list.  If you can get it set up you can even use 'git
send-email' to make submitting patches really easy.

> 
> diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y
> index 70a3d9c483..ea5c83e60a 100644
> --- a/gdb/m2-exp.y
> +++ b/gdb/m2-exp.y
> @@ -293,21 +293,18 @@ set	:	'{' arglist '}'
>  	;
> 
> 
> -/* Modula-2 array subscript notation [a,b,c...] */
> -exp     :       exp '['
> -                        /* This function just saves the number of arguments
> -			   that follow in the list.  It is *not* specific to
> -			   function types */
> -                        { pstate->start_arglist(); }

Removing the calls to start_arglist and end_arglist is a bad idea.
There is only a single pstate, with a single "current" arglist.  These
calls allow GDB to support nested arglists.

So, with these calls in place we can do this:

  (gdb) print array[ array[1,2], array[3,4] ]

without these calls this will all get very messed up.

> -                non_empty_arglist ']'  %prec DOT
> -                        { write_exp_elt_opcode (pstate, MULTI_SUBSCRIPT);
> -			  write_exp_elt_longcst (pstate,
> -						 pstate->end_arglist());
> -			  write_exp_elt_opcode (pstate, MULTI_SUBSCRIPT); }
> -        ;
> -
> -exp	:	exp '[' exp ']'
> -			{ write_exp_elt_opcode (pstate, BINOP_SUBSCRIPT); }
> +/* Modula-2 array subscript notation [a,b,c...].  */
> +exp     :       exp '[' ArgumentList ']' %prec DOT

GDB uses snake case, not camel case for new code.  Given the comment
attached to ArgumentList below I would suggest a suitable name would
be non_empty_arglist.

> +                {
> +                    if (pstate->arglist_len > 1)
> +		      {
> +			write_exp_elt_opcode (pstate, MULTI_SUBSCRIPT);
> +			write_exp_elt_longcst (pstate, pstate->arglist_len);
> +			write_exp_elt_opcode (pstate, MULTI_SUBSCRIPT);
> +		      }
> +		    else
> +		      write_exp_elt_opcode (pstate, BINOP_SUBSCRIPT);
> +		}

So when I saw this my first though was, I wonder why MULTI_SUBSCRIPT
doesn't support the 1 subscript case.  Looking at other users, and the
handling of MULTI_SUBSCRIPT I think it actually does.  So this code
can be simplified.

Given we know the arglist will have 1 or more entries we can just
assert that an make use of MULTI_SUBSCRIPT in all cases.

But adding back the call to end_arglist for the reasons above.

Once you do this, and restore the formatting you'll notice all we've
done is remove one of the 'exp' patterns.

>          ;
> 
>  exp	:	exp '('
> @@ -321,24 +318,22 @@ exp	:	exp '('
>  			  write_exp_elt_opcode (pstate, OP_FUNCALL); }
>  	;
> 
> -arglist	:
> -	;
> -
> -arglist	:	exp
> +/* Non empty argument list.  */
> +ArgumentList:
> +	exp
>  		{ pstate->arglist_len = 1; }
> +|	ArgumentList ',' exp
> +		{ pstate->arglist_len++; }
>  ;
> 
> -arglist	:	arglist ',' exp   %prec ABOVE_COMMA
> -			{ pstate->arglist_len++; }
> +arglist	:
>  	;
> 
> -non_empty_arglist
> -        :       exp
> +arglist	:	exp
>  			{ pstate->arglist_len = 1; }
>  	;
> 
> -non_empty_arglist
> -        :       non_empty_arglist ',' exp %prec ABOVE_COMMA
> +arglist	:	arglist ',' exp   %prec ABOVE_COMMA
>  			{ pstate->arglist_len++; }
>  	;

This feels like unnecessary churn, you rename 'non_empty_arglist' to
'ArgumentList' which I don't like, and you reorder things, but I'd
rather just leave this code unchanged.

> 
> 
> 
> and simple testcase
> ===================

Should be included in the patch.  Using 'git format-patch' will take
care of this for you.

There was a small issue when you wrote:

> if ![runto here] then {
>     perror "couldn't run to breakpoint foo"
>     continue
> }

The string passed to 'perror' is not correct.

I took the liberty of putting together the patch below.  This
addresses the issues I raised above, and includes an extra test case
that covers the nested array index use example.

If you're happy with this revision then I'll go ahead and merge this.

Thanks,
Andrew

----

commit 807521c52d6c4bfa0dec7b69a779ae25696652d6
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue Aug 25 09:39:27 2020 +0100

    gdb/modula-2: parsing of multi-subscript arrays
    
    Fix bug PR m2/26372, GDB's inability to parse multi-dimensional
    modula-2 arrays.
    
    We previously had two rules for handling the parsing of array
    sub-scripts.  I have reproduced them here with the actual handler
    blocks removed to make the bug clearer:
    
      exp     :    exp '[' non_empty_arglist ']'
              ;
    
      exp     :    exp '[' exp ']'
              ;
    
      non_empty_arglist
              :       exp
              ;
    
      non_empty_arglist
              :       non_empty_arglist ',' exp
              ;
    
    This is ambiguous as the pattern "exp '[' exp" could match either of
    the 'exp' rules.  Currently it just so happens that the parser picks
    the second 'exp' rule which means we can only handle a single array
    index.
    
    As the handler code for the first 'exp' pattern will correctly handle
    and number of array indexes then lets just remove the second pattern.
    
    gdb/ChangeLog:
    
            PR m2/26372
            * m2-exp.y (exp): Improve comment for non_empty_arglist case, add
            an assert.  Remove single element array indexing pattern as the
            MULTI_SUBSCRIPT support will handle this case too.
    
    gdb/testsuite/ChangeLog:
    
            PR m2/26372
            * gdb.modula2/multidim.c: New file.
            * gdb.modula2/multidim.exp: New file.

diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y
index 70a3d9c483a..c79c1f25828 100644
--- a/gdb/m2-exp.y
+++ b/gdb/m2-exp.y
@@ -293,21 +293,20 @@ set	:	'{' arglist '}'
 	;
 
 
-/* Modula-2 array subscript notation [a,b,c...] */
+/* Modula-2 array subscript notation [a,b,c...].  */
 exp     :       exp '['
                         /* This function just saves the number of arguments
 			   that follow in the list.  It is *not* specific to
 			   function types */
                         { pstate->start_arglist(); }
                 non_empty_arglist ']'  %prec DOT
-                        { write_exp_elt_opcode (pstate, MULTI_SUBSCRIPT);
+			{
+			  gdb_assert (pstate->arglist_len > 0);
+			  write_exp_elt_opcode (pstate, MULTI_SUBSCRIPT);
 			  write_exp_elt_longcst (pstate,
 						 pstate->end_arglist());
-			  write_exp_elt_opcode (pstate, MULTI_SUBSCRIPT); }
-        ;
-
-exp	:	exp '[' exp ']'
-			{ write_exp_elt_opcode (pstate, BINOP_SUBSCRIPT); }
+			  write_exp_elt_opcode (pstate, MULTI_SUBSCRIPT);
+			}
 	;
 
 exp	:	exp '('
diff --git a/gdb/testsuite/gdb.modula2/multidim.c b/gdb/testsuite/gdb.modula2/multidim.c
new file mode 100644
index 00000000000..b0ce8488681
--- /dev/null
+++ b/gdb/testsuite/gdb.modula2/multidim.c
@@ -0,0 +1,39 @@
+/* This test script is part of GDB, the GNU debugger.
+
+   Copyright 2020 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/>.  */
+
+
+static int a[10][20];
+
+static void
+here (void)
+{
+}
+
+int
+main ()
+{
+  int i, j;
+  int count = 0;
+
+  for (i = 0; i < 10; i++)
+    for (j = 0; j < 20; j++)
+      {
+	a[i][j] = count;
+	count += 1;
+      }
+  here ();
+}
diff --git a/gdb/testsuite/gdb.modula2/multidim.exp b/gdb/testsuite/gdb.modula2/multidim.exp
new file mode 100644
index 00000000000..ccccaa25981
--- /dev/null
+++ b/gdb/testsuite/gdb.modula2/multidim.exp
@@ -0,0 +1,37 @@
+# Copyright 2020 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/>.
+
+# This file is part of the gdb testsuite.  It contains tests for printing
+# the elements of an unbounded array using the Modula-2 language mode of
+# gdb.
+
+standard_testfile multidim.c
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
+	 {debug quiet}]} {
+    return -1
+}
+
+if ![runto here] then {
+    perror "couldn't run to breakpoint 'here'"
+    continue
+}
+
+gdb_test "set lang modula-2" ".*does not match.*" "switch to modula-2"
+
+gdb_test "print a\[1,2\]" ".*= 22.*" "second row third column"
+gdb_test "print a\[2,1\]" ".*= 41.*" "fifth row second column"
+gdb_test "print a\[a\[0,1\],a\[0,2\]\]" ".*= 22.*" \
+    "second row third column again"