From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21704 invoked by alias); 28 Feb 2014 17:37:10 -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 21692 invoked by uid 89); 28 Feb 2014 17:37:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 28 Feb 2014 17:37:07 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 9D54A116538; Fri, 28 Feb 2014 12:37:05 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id UxRIXAIO1G+g; Fri, 28 Feb 2014 12:37:05 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 5279F116530; Fri, 28 Feb 2014 12:37:05 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 78D77E03BD; Fri, 28 Feb 2014 09:37:04 -0800 (PST) Date: Fri, 28 Feb 2014 17:37:00 -0000 From: Joel Brobecker To: Sanimir Agovic Cc: tromey@redhat.com, keven.boell@intel.com, gdb-patches@sourceware.org Subject: Re: [PATCH v5 10/15] vla: evaluate operand of sizeof if its type is a vla Message-ID: <20140228173704.GC16479@adacore.com> References: <1391704056-25246-1-git-send-email-sanimir.agovic@intel.com> <1391704056-25246-11-git-send-email-sanimir.agovic@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1391704056-25246-11-git-send-email-sanimir.agovic@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-02/txt/msg00869.txt.bz2 On Thu, Feb 06, 2014 at 05:27:31PM +0100, Sanimir Agovic wrote: > The c99 standard in "6.5.3.4 The sizeof operator" states: > > If the type of the operand is a variable length array type, the operand > is evaluated;[...] > > This patch mirrors the following c99 semantic in gdb: > > 1| int vla[n][m]; > 2| int i = 1; > 3| sizeof(vla[i++][0]); // No sideffect > 4| assert (i == 1); > 5| sizeof(vla[i++]); // With sideffect > 6| assert (i == 2); 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. > 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. My comments below. > > > 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_type, > 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) += 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) Can you also update the function's introductory comment to document the new parameter? > { > /* FIXME: This should be size_t. */ > struct type *size_type = builtin_type (exp->gdbarch)->builtin_int; > @@ -3054,6 +3056,38 @@ evaluate_subexp_for_sizeof (struct expression *exp, int *pos) > return > value_from_longest (size_type, (LONGEST) TYPE_LENGTH (type)); > > + case BINOP_SUBSCRIPT: > + if (noside == EVAL_NORMAL) > + { 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... > + int oldpos = *pos; > + > + (*pos) += 1; > + val = evaluate_subexp (NULL_TYPE, exp, pos, EVAL_AVOID_SIDE_EFFECTS); > + *pos = oldpos; > + noside = EVAL_AVOID_SIDE_EFFECTS; 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. 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. > + > + type = check_typedef (value_type (val)); > + if (TYPE_CODE (type) == TYPE_CODE_ARRAY) > + { > + type = check_typedef (TYPE_TARGET_TYPE (type)); > + if (TYPE_CODE (type) == TYPE_CODE_ARRAY) > + { > + type = TYPE_INDEX_TYPE (type); > + if ((TYPE_RANGE_DATA (type)->flags & RANGE_EVALUATED) > + == RANGE_EVALUATED) > + { [1] (see reference to this above). > + val = evaluate_subexp (NULL_TYPE, exp, pos, EVAL_NORMAL); > + return value_from_longest > + (size_type, (LONGEST)TYPE_LENGTH (value_type (val))); > + } > + } > + } > + } > + > + val = evaluate_subexp (NULL_TYPE, exp, pos, EVAL_AVOID_SIDE_EFFECTS); > + return value_from_longest (size_type, > + (LONGEST)TYPE_LENGTH (value_type (val))); 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: /* Fall through. */ > + > default: > val = evaluate_subexp (NULL_TYPE, exp, pos, EVAL_AVOID_SIDE_EFFECTS); > 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_ADDR addr) > = create_range_type (NULL, > TYPE_TARGET_TYPE (range_type), > &low_bound, &high_bound); > + TYPE_RANGE_DATA (range_type)->flags |= RANGE_EVALUATED; > array_type = 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 = 1 /* High bound contains number of elements. */ > + RANGE_UPPER_BOUND_IS_COUNT = 1, /* High bound contains number of elements. */ > + RANGE_EVALUATED /* Bound was dynamic. */ 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? > }; > > /* 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/gdb.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 () Add "void" as param here, please. > +{ > + int n = SIZE; > + int i = 0; > + int j = 0; > + int vla2[SIZE][n]; > + int vla1[n]; > + > + for (i = 0; i < n; i++) > + vla1[i] = (i * 2) + n; > + > + for (i = 0; i < SIZE; i++) > + for (j = 0; j < n; j++) > + vla2[i][j] = (i + j) + n; > + > + > + i = 0; > + j = 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 > +# Based on gcc/testsuite/gcc.dg/vla-4.c; vla-15.c > + > +standard_testfile ".c" The ".c" shouldn't be necessary. Can you remove it? > +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+ = ${sizeof_int}" \ You don't need to match the $N part of the output, we've traditionally saved us the trouble, but simply using: " = ${sizeof_int}" Can you adjust the testcase throughout? > + "print sizeof (vla1\[i++\])" > +gdb_test "print i" "\\$\\d+ = 0" \ > + "print i - sizeof no side effects" > + > +gdb_test "print sizeof (++vla1\[0\])" "\\$\\d+ = ${sizeof_int}" \ > + "print sizeof (++vla1\[0\])" > +gdb_test "print vla1\[0\]" "\\$\\d+ = 10" \ > + "print vla1\[0\] - sizeof no side effects" > + > +gdb_test "ptype ++vla1\[0\]" "type = int" "ptype ++vla1\[0\]" > +gdb_test "print vla1\[0\]" "\\$\\d+ = 10" \ > + "print vla1\[0\] - ptype no side effects" > + > +gdb_test "whatis ++vla1\[0\]" "type = int" "whatis ++vla1\[0\]" > +gdb_test "print vla1\[0\]" "\\$\\d+ = 10" \ > + "print vla1\[0\] - whatis no side effects" > + > + > +gdb_test "print sizeof (vla2\[i++\])" "\\$\\d+ = ${sizeof_vla}" \ > + "print sizeof (vla2\[i++\])" > +gdb_test "print i" "\\$\\d+ = 1" \ > + "print i - sizeof with side effects (1)" > + > +gdb_test "print sizeof (vla2\[i++ + sizeof(j++)\])" "\\$\\d+ = ${sizeof_vla}" \ > + "print sizeof (vla2\[i++ + sizeof(j++)\])" > +gdb_test "print i" "\\$\\d+ = 2" \ > + "print i - sizeof with side effects (2)" > +gdb_test "print j" "\\$\\d+ = 0" \ > + "print j - sizeof with no side effects" > + > +gdb_test "ptype vla2\[i++\]" "type = int \\\[10\\\]" \ > + "ptype vla2\[i++\]" > +gdb_test "print i" "\\$\\d+ = 2" \ > + "print i - ptype with side effects (1)" > + > +gdb_test "ptype vla2\[i++ + sizeof(j++)\]" "type = int \\\[10\\\]" \ > + "ptype vla2\[i++ + sizeof(j++)\]" > +gdb_test "print i" "\\$\\d+ = 2" \ > + "print i - ptype with side effects (2)" > +gdb_test "print j" "\\$\\d+ = 0" \ > + "print j - ptype with no side effects" > + > +gdb_test "whatis vla2\[i++\]" "type = int \\\[10\\\]" \ > + "whatis vla2\[i++\]" > +gdb_test "print i" "\\$\\d+ = 2" \ > + "print i - whatis with side effects (1)" > + > +gdb_test "whatis vla2\[i++ + sizeof(j++)\]" "type = int \\\[10\\\]" \ > + "whatis vla2\[i++ + sizeof(j++)\]" > +gdb_test "print i" "\\$\\d+ = 2" \ > + "print i - whatis with side effects (2)" > +gdb_test "print j" "\\$\\d+ = 0" \ > + "print j - whatis with no side effects" > -- > 1.8.4.2 -- Joel