* 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 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: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 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).