* Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays @ 2020-08-25 2:29 Gaius Mulley 2020-08-25 9:22 ` Andrew Burgess 2020-08-25 9:29 ` Tom de Vries 0 siblings, 2 replies; 6+ messages in thread From: Gaius Mulley @ 2020-08-25 2:29 UTC (permalink / raw) To: gdb-patches, gdb, Tom de Vries [-- Attachment #1: Type: text/plain, Size: 2479 bytes --] 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). Patch ===== 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(); } - 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 + { + 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); + } ; 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++; } ; and simple testcase =================== [-- Attachment #2: multidimension test case for Modula-2 --] [-- Type: application/gzip, Size: 1081 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays 2020-08-25 2:29 Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays Gaius Mulley @ 2020-08-25 9:22 ` Andrew Burgess 2020-08-25 12:25 ` Gaius Mulley 2020-08-25 9:29 ` Tom de Vries 1 sibling, 1 reply; 6+ messages in thread From: Andrew Burgess @ 2020-08-25 9:22 UTC (permalink / raw) To: Gaius Mulley; +Cc: gdb-patches, gdb, Tom de Vries * 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" ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays 2020-08-25 9:22 ` Andrew Burgess @ 2020-08-25 12:25 ` Gaius Mulley 0 siblings, 0 replies; 6+ messages in thread From: Gaius Mulley @ 2020-08-25 12:25 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb, gdb-patches 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" ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays 2020-08-25 2:29 Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays Gaius Mulley 2020-08-25 9:22 ` Andrew Burgess @ 2020-08-25 9:29 ` Tom de Vries 2020-08-25 9:36 ` Andrew Burgess 1 sibling, 1 reply; 6+ messages in thread From: Tom de Vries @ 2020-08-25 9:29 UTC (permalink / raw) To: Gaius Mulley, gdb-patches, gdb [-- Attachment #1: Type: text/plain, Size: 1178 bytes --] On 8/25/20 4:29 AM, Gaius Mulley wrote: > > > 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? > Hi Gaius, I ran into some trouble applying the patch with the patch program, so I applied the patch manually, and then diffed the result of that with the original patch and fixed whitespace problems in the original patch until I got duplicates, to ensure that the application process was sane. I verified that when building with the patch, the warning "rule useless in parser due to conflicts" is gone, and no new warnings are introduced. I ran the test-case and verified that it failed without and passed with the patch. I've also fixed the ChangeLog entry a bit (multidim.y -> multidim.exp) and split it up for gdb/ChangeLog and gdb/testsuite/Changelog, as well as added a PR number. I've gone ahead an applied it (with you as author, obviously). See https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=07758bdfa9e5a762f2ec0deeb51b11d6ad5fe376 . Attached here for reference. Thanks, - Tom [-- Attachment #2: 0001-Fix-for-Bug-26372-Modula-2-Parsing-of-multi-subscript-arrays.patch --] [-- Type: text/x-patch, Size: 5559 bytes --] Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays 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. gdb/ChangeLog: 2020-08-25 Gaius Mulley <gaiusmod2@gmail.com> PR m2/26372 * m2-exp.y: Rewrite array subscript rules to support multidimension array access. (ArgumentList) replaces non_empty_arglist. gdb/testsuite/ChangeLog: 2020-08-25 Gaius Mulley <gaiusmod2@gmail.com> PR m2/26372 * testsuite/gdb.modula2/multidim.exp: New file. * testsuite/gdb.modula2/multidim.c: New file. --- gdb/m2-exp.y | 45 +++++++++++++++------------------- gdb/testsuite/gdb.modula2/multidim.c | 39 +++++++++++++++++++++++++++++ gdb/testsuite/gdb.modula2/multidim.exp | 34 +++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 25 deletions(-) diff --git a/gdb/m2-exp.y b/gdb/m2-exp.y index 70a3d9c483..dba331f405 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(); } - 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 + { + 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); + } ; 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++; } ; diff --git a/gdb/testsuite/gdb.modula2/multidim.c b/gdb/testsuite/gdb.modula2/multidim.c new file mode 100644 index 0000000000..b0ce848868 --- /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 0000000000..df7054892a --- /dev/null +++ b/gdb/testsuite/gdb.modula2/multidim.exp @@ -0,0 +1,34 @@ +# 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 foo" + 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" ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays 2020-08-25 9:29 ` Tom de Vries @ 2020-08-25 9:36 ` Andrew Burgess 2020-08-25 12:33 ` Gaius Mulley 0 siblings, 1 reply; 6+ messages in thread From: Andrew Burgess @ 2020-08-25 9:36 UTC (permalink / raw) To: Tom de Vries; +Cc: Gaius Mulley, gdb-patches, gdb * Tom de Vries <tdevries@suse.de> [2020-08-25 11:29:18 +0200]: > On 8/25/20 4:29 AM, Gaius Mulley wrote: > > > > > > 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? > > > > Hi Gaius, > > I ran into some trouble applying the patch with the patch program, so I > applied the patch manually, and then diffed the result of that with the > original patch and fixed whitespace problems in the original patch until > I got duplicates, to ensure that the application process was sane. > > I verified that when building with the patch, the warning "rule useless > in parser due to conflicts" is gone, and no new warnings are introduced. > > I ran the test-case and verified that it failed without and passed with > the patch. > > I've also fixed the ChangeLog entry a bit (multidim.y -> multidim.exp) > and split it up for gdb/ChangeLog and gdb/testsuite/Changelog, as well > as added a PR number. > > I've gone ahead an applied it (with you as author, obviously). See > https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=07758bdfa9e5a762f2ec0deeb51b11d6ad5fe376 > . > > Attached here for reference. Given the issues I raised with this patch in [1] I reverted this commit and pushed my suggested patch instead. Again maintaining the original author. I hope this does not offend anyone. Please let me know if this was the wrong course of action. Thanks, Andrew [1] https://sourceware.org/pipermail/gdb-patches/2020-August/171456.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays 2020-08-25 9:36 ` Andrew Burgess @ 2020-08-25 12:33 ` Gaius Mulley 0 siblings, 0 replies; 6+ messages in thread From: Gaius Mulley @ 2020-08-25 12:33 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom de Vries, gdb, gdb-patches Andrew Burgess <andrew.burgess@embecosm.com> writes: > * Tom de Vries <tdevries@suse.de> [2020-08-25 11:29:18 +0200]: > >> On 8/25/20 4:29 AM, Gaius Mulley wrote: >> > >> > >> > 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? >> > >> >> Hi Gaius, >> >> I ran into some trouble applying the patch with the patch program, so I >> applied the patch manually, and then diffed the result of that with the >> original patch and fixed whitespace problems in the original patch until >> I got duplicates, to ensure that the application process was sane. >> >> I verified that when building with the patch, the warning "rule useless >> in parser due to conflicts" is gone, and no new warnings are introduced. >> >> I ran the test-case and verified that it failed without and passed with >> the patch. >> >> I've also fixed the ChangeLog entry a bit (multidim.y -> multidim.exp) >> and split it up for gdb/ChangeLog and gdb/testsuite/Changelog, as well >> as added a PR number. >> >> I've gone ahead an applied it (with you as author, obviously). See >> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=07758bdfa9e5a762f2ec0deeb51b11d6ad5fe376 >> . >> >> Attached here for reference. > > Given the issues I raised with this patch in [1] I reverted this > commit and pushed my suggested patch instead. Again maintaining the > original author. > > I hope this does not offend anyone. Please let me know if this was > the wrong course of action. > > Thanks, > Andrew > > [1] https://sourceware.org/pipermail/gdb-patches/2020-August/171456.html Hi Andrew and Tom, many thanks for reviewing and testing the patch, improving the patch and applying it. All looks great now, regards, Gaius ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-25 12:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-25 2:29 Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays Gaius Mulley 2020-08-25 9:22 ` Andrew Burgess 2020-08-25 12:25 ` Gaius Mulley 2020-08-25 9:29 ` Tom de Vries 2020-08-25 9:36 ` Andrew Burgess 2020-08-25 12:33 ` Gaius Mulley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).