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

Andrew Burgess <[email protected]> writes:

> * Gaius Mulley via Gdb-patches <[email protected]> [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  <[email protected]>
>> 
>> 	* 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.

Hi Andrew,

many thanks for the steer - very useful and will short cut my old workflow.

>> 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.

yes thanks for spotting this issue.

>> -                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.

yes this is really great - thanks for the analysis and also
improving/applying the patch!

regards,
Gaius


>
> Thanks,
> Andrew
>
> ----
>
> commit 807521c52d6c4bfa0dec7b69a779ae25696652d6
> Author: Andrew Burgess <[email protected]>
> 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"