From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x430.google.com (mail-wr1-x430.google.com [IPv6:2a00:1450:4864:20::430]) by sourceware.org (Postfix) with ESMTPS id EBF1C3858D37; Tue, 25 Aug 2020 12:26:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org EBF1C3858D37 Received: by mail-wr1-x430.google.com with SMTP id c15so12485802wrs.11; Tue, 25 Aug 2020 05:26:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=n6Hv5VOqou5P3ZNOXc8mxmBOCIbepI3kyl5Ze5ZO5x8=; b=YrYPMsIIdakoM7Hy24w05ChLgVdppjCxwt6RjXiRuYfzXEI68/TvBCqN5wgHhfOeDE E2Bwypyu+pzlIGmhRL5KXqfOBqoYf1ToNxh3Se/ucgFVlR3E3hJgDp/d/egor1g0Ns3n fT8QBXK8UjGhvPh8NzqkjwcWhxR4q4TxyDLKhVrRzWP98To4w7u2PkdKTvqA7gPWmGwA LMdEsz9qX+L/Lwso6KavN3sCab22XOPomTAVVMly/XoGnbxHRTUGpm7yP0hmqs/LFls6 ahYgUjYzexddOK6ZzMQn03gRj3Hi9qKOT54VAcHEIsHknedue6KBmSwfuPHCLJIDTEZO nwOg== X-Gm-Message-State: AOAM530YL9fTApiUqG0X2whmK/9OXQnyzmcCnSDy1WBPAQDyXmxjvhak Cm6qboooEHkVWRXaCTEqK36wrLif2Bk= X-Google-Smtp-Source: ABdhPJxVw6D33qyDdL9gR1D4EKQR2CDmSwouGQqZU5sFSKdEpuXhkpspKMcIeoHIAlL+tDkG3D+qBw== X-Received: by 2002:a5d:56c9:: with SMTP id m9mr10304094wrw.311.1598358359386; Tue, 25 Aug 2020 05:25:59 -0700 (PDT) Received: from ettard ([195.147.220.46]) by smtp.gmail.com with ESMTPSA id u3sm5305856wml.44.2020.08.25.05.25.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Aug 2020 05:25:58 -0700 (PDT) From: Gaius Mulley To: Andrew Burgess Cc: gdb@sourceware.org, gdb-patches@sourceware.org Subject: Re: Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays References: <878se3xxtr.fsf@ettard> <20200825092246.GS853475@embecosm.com> Date: Tue, 25 Aug 2020 13:25:57 +0100 In-Reply-To: <20200825092246.GS853475@embecosm.com> (Andrew Burgess's message of "Tue, 25 Aug 2020 10:22:46 +0100") Message-ID: <87pn7egbe2.fsf@ettard> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Aug 2020 12:26:03 -0000 Andrew Burgess writes: > * Gaius Mulley via Gdb-patches [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 >> >> * 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 > 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 . */ > + > + > +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 . > + > +# 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"