From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31136 invoked by alias); 19 Mar 2014 12:55:07 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 31126 invoked by uid 89); 19 Mar 2014 12:55:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mga01.intel.com Received: from mga01.intel.com (HELO mga01.intel.com) (192.55.52.88) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Mar 2014 12:55:04 +0000 Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP; 19 Mar 2014 05:55:03 -0700 X-ExtLoop1: 1 Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by fmsmga002.fm.intel.com with ESMTP; 19 Mar 2014 05:55:00 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.87]) by IRSMSX103.ger.corp.intel.com ([169.254.3.27]) with mapi id 14.03.0123.003; Wed, 19 Mar 2014 12:54:40 +0000 From: "Agovic, Sanimir" To: 'Joel Brobecker' CC: "tromey@redhat.com" , "Boell, Keven" , "gdb-patches@sourceware.org" Subject: RE: [PATCH v5 10/15] vla: evaluate operand of sizeof if its type is a vla Date: Wed, 19 Mar 2014 12:55:00 -0000 Message-ID: <0377C58828D86C4588AEEC42FC3B85A7177464DE@IRSMSX105.ger.corp.intel.com> References: <1391704056-25246-1-git-send-email-sanimir.agovic@intel.com> <1391704056-25246-11-git-send-email-sanimir.agovic@intel.com> <20140228173704.GC16479@adacore.com> In-Reply-To: <20140228173704.GC16479@adacore.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2014-03/txt/msg00446.txt.bz2 Thanks for your review. > > 1| int vla[n][m]; > > 2| int i =3D 1; > > 3| sizeof(vla[i++][0]); // No sideffect > > 4| assert (i =3D=3D 1); > > 5| sizeof(vla[i++]); // With sideffect > > 6| assert (i =3D=3D 2); >=20 > Not knowing C that well, I don't understand why the first one > does not have any side effect, while the second does. Can you > explain? This will also help explain the implementation, as > I don't understand the logic yet. >=20 If the operand passed to sizeof evaluates to a variable length array type t= hen C99 requires that the size of the operand is evaluated at runtime (otherwise at= compile time). 1| int vla[n][m]; sizeof(vla[0]) // Operand type is a variable length type, size is eval= uated at runtime sizeof(vla[0][0]) // Operand type is an int, size is evaluated at compile= time=20 If the expression contains a side effects it is evaluated too sizeof(vla[i++]) // i gets modified as the operand is evaluated at runti= me sizeof(vla[i++][0]) // no modifications, operand type evaluates to int The C99 chapter about sizeof is "6.5.3.4 The sizeof operator". I addressed all of your issues below. -Sanimir > > 2014-02-05 Sanimir Agovic > > Keven Boell > > > > * eval.c (evaluate_subexp_for_sizeof): Add enum noside argument. > > (evaluate_subexp_standard): Pass noside argument. > > (evaluate_subexp_for_sizeof) : Handle subscript case > > if noside equals EVAL_NORMAL. If the subscript yields a vla type > > re-evaluate subscript operation with EVAL_NORMAL to enable sideffects. > > * gdbtypes.c (resolve_dynamic_bounds): Mark bound as evaluated. > > * gdbtypes.h (enum range_flags): Add RANGE_EVALUATED case. > > > > testsuite/gdb.base/ > > > > * vla-sideeffect.c: New file. > > * vla-sideeffect.exp: New file. >=20 > My comments below. >=20 > > > > > > Signed-off-by: Sanimir Agovic > > --- > > gdb/eval.c | 40 ++++++++++++-- > > gdb/gdbtypes.c | 1 + > > gdb/gdbtypes.h | 3 +- > > gdb/testsuite/gdb.base/vla-sideeffect.c | 42 +++++++++++++++ > > gdb/testsuite/gdb.base/vla-sideeffect.exp | 88 +++++++++++++++++++++++= ++++++++ > > 5 files changed, 170 insertions(+), 4 deletions(-) > > create mode 100644 gdb/testsuite/gdb.base/vla-sideeffect.c > > create mode 100644 gdb/testsuite/gdb.base/vla-sideeffect.exp > > > > diff --git a/gdb/eval.c b/gdb/eval.c > > index b3e45ca..7a34b95 100644 > > --- a/gdb/eval.c > > +++ b/gdb/eval.c > > @@ -51,7 +51,8 @@ extern int overload_resolution; > > > > /* Prototypes for local functions. */ > > > > -static struct value *evaluate_subexp_for_sizeof (struct expression *, = int *); > > +static struct value *evaluate_subexp_for_sizeof (struct expression *, = int *, > > + enum noside); > > > > static struct value *evaluate_subexp_for_address (struct expression *, > > int *, enum noside); > > @@ -2563,7 +2564,7 @@ evaluate_subexp_standard (struct type *expect_typ= e, > > evaluate_subexp (NULL_TYPE, exp, pos, EVAL_SKIP); > > goto nosideret; > > } > > - return evaluate_subexp_for_sizeof (exp, pos); > > + return evaluate_subexp_for_sizeof (exp, pos, noside); > > > > case UNOP_CAST: > > (*pos) +=3D 2; > > @@ -3000,7 +3001,8 @@ evaluate_subexp_with_coercion (struct expression = *exp, > > Advance *POS over the subexpression. */ > > > > static struct value * > > -evaluate_subexp_for_sizeof (struct expression *exp, int *pos) > > +evaluate_subexp_for_sizeof (struct expression *exp, int *pos, > > + enum noside noside) >=20 > Can you also update the function's introductory comment to document > the new parameter? >=20 Done > > { > > /* FIXME: This should be size_t. */ > > struct type *size_type =3D builtin_type (exp->gdbarch)->builtin_int; > > @@ -3054,6 +3056,38 @@ evaluate_subexp_for_sizeof (struct expression *e= xp, int *pos) > > return > > value_from_longest (size_type, (LONGEST) TYPE_LENGTH (type)); > > > > + case BINOP_SUBSCRIPT: > > + if (noside =3D=3D EVAL_NORMAL) > > + { >=20 > This deserves a comment explaining why you do what you in EVAL_NORMAL > mode. Probably something along the lines of the answer to the question > I asked above. Especially the bits I marked as [1] below... >=20 >=20 I will try, please have a look at the upcoming v6. > > + int oldpos =3D *pos; > > + > > + (*pos) +=3D 1; > > + val =3D evaluate_subexp (NULL_TYPE, exp, pos, EVAL_AVOID_SIDE_EFFEC= TS); > > + *pos =3D oldpos; > > + noside =3D EVAL_AVOID_SIDE_EFFECTS; >=20 > It would be easier, I think, to have a temporary like you do, and pass > that temporary as the position to evaluate_subexp. You'd then not > have to restore POS afterwards. >=20 > Also, I don't think you need to set noside to EVAL_AVOID_SIDE_EFFECTS > since you're not using it other than in your initial mode check. >=20 Both done. > > + > > + type =3D check_typedef (value_type (val)); > > + if (TYPE_CODE (type) =3D=3D TYPE_CODE_ARRAY) > > + { > > + type =3D check_typedef (TYPE_TARGET_TYPE (type)); > > + if (TYPE_CODE (type) =3D=3D TYPE_CODE_ARRAY) > > + { > > + type =3D TYPE_INDEX_TYPE (type); > > + if ((TYPE_RANGE_DATA (type)->flags & RANGE_EVALUATED) > > + =3D=3D RANGE_EVALUATED) > > + { >=20 > [1] (see reference to this above). >=20 > > + val =3D evaluate_subexp (NULL_TYPE, exp, pos, EVAL_NORMAL); > > + return value_from_longest > > + (size_type, (LONGEST)TYPE_LENGTH (value_type (val))); > > + } > > + } > > + } > > + } > > + > > + val =3D evaluate_subexp (NULL_TYPE, exp, pos, EVAL_AVOID_SIDE_EF= FECTS); > > + return value_from_longest (size_type, > > + (LONGEST)TYPE_LENGTH (value_type (val))); >=20 > Formatting: Space after "(LONGEST)". But I suggest intead falling > through to the default: case below. Just add a command mentioning > that you are doing it intentionally. Eg: >=20 > /* Fall through. */ >=20 Done. >=20 > > + > > default: > > val =3D evaluate_subexp (NULL_TYPE, exp, pos, EVAL_AVOID_SIDE_EF= FECTS); > > return value_from_longest (size_type, > > diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c > > index 83a2c75..67aa439 100644 > > --- a/gdb/gdbtypes.c > > +++ b/gdb/gdbtypes.c > > @@ -1697,6 +1697,7 @@ resolve_dynamic_bounds (struct type *type, CORE_A= DDR addr) > > =3D create_range_type (NULL, > > TYPE_TARGET_TYPE (range_type), > > &low_bound, &high_bound); > > + TYPE_RANGE_DATA (range_type)->flags |=3D RANGE_EVALUATED; > > array_type =3D create_array_type (copy_type (type), > > array_type, > > range_type); > > diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h > > index 100e3f4..4e92d4a 100644 > > --- a/gdb/gdbtypes.h > > +++ b/gdb/gdbtypes.h > > @@ -396,7 +396,8 @@ struct dynamic_prop > > > > enum range_flags > > { > > - RANGE_UPPER_BOUND_IS_COUNT =3D 1 /* High bound contains number of el= ements. */ > > + RANGE_UPPER_BOUND_IS_COUNT =3D 1, /* High bound contains number of e= lements. */ > > + RANGE_EVALUATED /* Bound was dynamic. */ >=20 > Same as in one of the previous patches. I think a bit component would > be better.. I also would like the comment to be a little more > elaborated: What does it mean when the flag is set vs unset? >=20 Done. > > }; > > > > /* Determine which field of the union main_type.fields[x].loc is used.= */ > > diff --git a/gdb/testsuite/gdb.base/vla-sideeffect.c b/gdb/testsuite/gd= b.base/vla- > sideeffect.c > > new file mode 100644 > > index 0000000..29ee99b > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/vla-sideeffect.c > > @@ -0,0 +1,42 @@ > > +/* This testcase is part of GDB, the GNU debugger. > > + > > + Copyright 2014 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 . */ > > + > > +#include > > +#define SIZE 10 > > + > > +int > > +main () >=20 > Add "void" as param here, please. >=20 Done. > > +{ > > + int n =3D SIZE; > > + int i =3D 0; > > + int j =3D 0; > > + int vla2[SIZE][n]; > > + int vla1[n]; > > + > > + for (i =3D 0; i < n; i++) > > + vla1[i] =3D (i * 2) + n; > > + > > + for (i =3D 0; i < SIZE; i++) > > + for (j =3D 0; j < n; j++) > > + vla2[i][j] =3D (i + j) + n; > > + > > + > > + i =3D 0; > > + j =3D 0; > > + > > + return 0; /* vla-filled */ > > +} > > diff --git a/gdb/testsuite/gdb.base/vla-sideeffect.exp b/gdb/testsuite/= gdb.base/vla- > sideeffect.exp > > new file mode 100644 > > index 0000000..0ea9bc2 > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/vla-sideeffect.exp > > @@ -0,0 +1,88 @@ > > +# Copyright 2014 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 . > > + > > +# Tests sideffect of sizeof evaluation. > ^^^^^^^^^ > side-effects >=20 Done. > > +# Based on gcc/testsuite/gcc.dg/vla-4.c; vla-15.c > > + > > +standard_testfile ".c" >=20 > The ".c" shouldn't be necessary. Can you remove it? >=20 Done. > > +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } { > > + return -1 > > +} > > + > > +if ![runto_main] { > > + return -1 > > +} > > + > > +gdb_breakpoint [gdb_get_line_number "vla-filled"] > > +gdb_continue_to_breakpoint "vla-filled" > > + > > +# Check side effects for sizeof argument. > > +set sizeof_int [get_sizeof "int" 4] > > +set sizeof_vla [ expr "10" * "$sizeof_int" ] > > + > > +gdb_test "print sizeof (vla1\[i++\])" "\\$\\d+ =3D ${sizeof_int}" \ >=20 > You don't need to match the $N part of the output, we've traditionally > saved us the trouble, but simply using: " =3D ${sizeof_int}" >=20 > Can you adjust the testcase throughout? >=20 Done. > > + "print sizeof (vla1\[i++\])" > > +gdb_test "print i" "\\$\\d+ =3D 0" \ > > + "print i - sizeof no side effects" > > + > > +gdb_test "print sizeof (++vla1\[0\])" "\\$\\d+ =3D ${sizeof_int}" \ > > + "print sizeof (++vla1\[0\])" > > +gdb_test "print vla1\[0\]" "\\$\\d+ =3D 10" \ > > + "print vla1\[0\] - sizeof no side effects" > > + > > +gdb_test "ptype ++vla1\[0\]" "type =3D int" "ptype ++vla1\[0\]" > > +gdb_test "print vla1\[0\]" "\\$\\d+ =3D 10" \ > > + "print vla1\[0\] - ptype no side effects" > > + > > +gdb_test "whatis ++vla1\[0\]" "type =3D int" "whatis ++vla1\[0\]" > > +gdb_test "print vla1\[0\]" "\\$\\d+ =3D 10" \ > > + "print vla1\[0\] - whatis no side effects" > > + > > + > > +gdb_test "print sizeof (vla2\[i++\])" "\\$\\d+ =3D ${sizeof_vla}" \ > > + "print sizeof (vla2\[i++\])" > > +gdb_test "print i" "\\$\\d+ =3D 1" \ > > + "print i - sizeof with side effects (1)" > > + > > +gdb_test "print sizeof (vla2\[i++ + sizeof(j++)\])" "\\$\\d+ =3D ${siz= eof_vla}" \ > > + "print sizeof (vla2\[i++ + sizeof(j++)\])" > > +gdb_test "print i" "\\$\\d+ =3D 2" \ > > + "print i - sizeof with side effects (2)" > > +gdb_test "print j" "\\$\\d+ =3D 0" \ > > + "print j - sizeof with no side effects" > > + > > +gdb_test "ptype vla2\[i++\]" "type =3D int \\\[10\\\]" \ > > + "ptype vla2\[i++\]" > > +gdb_test "print i" "\\$\\d+ =3D 2" \ > > + "print i - ptype with side effects (1)" > > + > > +gdb_test "ptype vla2\[i++ + sizeof(j++)\]" "type =3D int \\\[10\\\]" \ > > + "ptype vla2\[i++ + sizeof(j++)\]" > > +gdb_test "print i" "\\$\\d+ =3D 2" \ > > + "print i - ptype with side effects (2)" > > +gdb_test "print j" "\\$\\d+ =3D 0" \ > > + "print j - ptype with no side effects" > > + > > +gdb_test "whatis vla2\[i++\]" "type =3D int \\\[10\\\]" \ > > + "whatis vla2\[i++\]" > > +gdb_test "print i" "\\$\\d+ =3D 2" \ > > + "print i - whatis with side effects (1)" > > + > > +gdb_test "whatis vla2\[i++ + sizeof(j++)\]" "type =3D int \\\[10\\\]" \ > > + "whatis vla2\[i++ + sizeof(j++)\]" > > +gdb_test "print i" "\\$\\d+ =3D 2" \ > > + "print i - whatis with side effects (2)" > > +gdb_test "print j" "\\$\\d+ =3D 0" \ > > + "print j - whatis with no side effects" > > -- > > 1.8.4.2 >=20 > -- > Joel Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen, Deutschland Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk Registergericht: Muenchen HRB 47456 Ust.-IdNr./VAT Registration No.: DE129385895 Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052