From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x344.google.com (mail-wm1-x344.google.com [IPv6:2a00:1450:4864:20::344]) by sourceware.org (Postfix) with ESMTPS id 04891385780F for ; Tue, 25 Aug 2020 09:22:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 04891385780F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x344.google.com with SMTP id t2so1705218wma.0 for ; Tue, 25 Aug 2020 02:22:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=IJCJyzRLBlZYXqrwAmh1JgTN6Xl3Yj4ZTTKN4A1G2G0=; b=A1kJiSyX+eo1udMW3mJmZa2E8jjv/PAqz71jAgxStq8qCHREPa9Tm8EO8Ed60EVzBM jYOrb675wWqE1FxRO+ucA6EyPMCa6bg6p8IbpWYoCvOB3kpzc2VenOPGL7j3GMUGZuL6 FLj4m+92fxdKmqMCfjT2YRcuDoSO3Ghvd7kh7ok7jIxrYWhO+z48YiTLsyWBRYxA8SLh 9ycPbpbQ5ddIG0RldYB49fBiwfhUBqRvuViykKVWubO5SMQDeQNhM6x4clYv3oOA6w7U U3RCzGzxxDJvweBJWpZOrjJ0fq016N5giAtZyjkvDw+23oofQ6h8vI9EjRTkQ7S3E0Cj h7Gw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=IJCJyzRLBlZYXqrwAmh1JgTN6Xl3Yj4ZTTKN4A1G2G0=; b=kDMaYCsMNfNS3EWrBDe94eXFRX0sBQjuvdqMWNZFikNt0c3K4xQDVzZflZKPr2U3ks imhJWqebsp83bxAebFVOJV5aU3EzoggRWf1U3KtrQG9hmYsmmeC3yDiw8APrq+5GMexT 37SAe7AfVlNrzWGJkllU5x3awhP2yaFS/Q3CaWfX6BebY328FZAhjAClYocEB0ZOxF/W 17P4ul2g50h+fMbUYDcosHqj7R2SOXxFBhNPyBjT4sp7vcy7ED4T2Sle6zB3P4ZttsPy ySBtBGO5gV6vcLYkaP0LU87UTwWOxycgtUkBPJwiTO04Y16K5oSe9Zjj+71sGMC0rswn q8iQ== X-Gm-Message-State: AOAM533gO6/I9NTKMSIGfB8bm4mKMz1sU+BSiTQMREnxBmjHT+QP6t0M zfKk5slrv9b98Kxixzn8xGnzYg== X-Google-Smtp-Source: ABdhPJxUt95CW3DSoTDuHlo/29XK4fgcWuWuarCVYPcMaCIPshf3J51Uki+75+cXyqpjOjnemuKXvw== X-Received: by 2002:a05:600c:2246:: with SMTP id a6mr1157424wmm.71.1598347367811; Tue, 25 Aug 2020 02:22:47 -0700 (PDT) Received: from localhost (host109-148-134-218.range109-148.btcentralplus.com. [109.148.134.218]) by smtp.gmail.com with ESMTPSA id z6sm4668980wml.41.2020.08.25.02.22.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Aug 2020 02:22:47 -0700 (PDT) Date: Tue, 25 Aug 2020 10:22:46 +0100 From: Andrew Burgess To: Gaius Mulley Cc: gdb-patches@sourceware.org, gdb@sourceware.org, Tom de Vries Subject: Re: Patch: Fix for Bug 26372 [Modula-2] Parsing of multi-subscript arrays Message-ID: <20200825092246.GS853475@embecosm.com> References: <878se3xxtr.fsf@ettard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <878se3xxtr.fsf@ettard> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 10:04:22 up 37 days, 18:18, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-8.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_ABUSEAT, 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 09:22:51 -0000 * 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. > > 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 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"