Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays
Gaius Mulley
gaius.southwales@gmail.com
Tue Aug 25 12:25:57 GMT 2020
Andrew Burgess <andrew.burgess@embecosm.com> writes:
> * 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.
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 <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"
More information about the Gdb-patches
mailing list