This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v5 10/15] vla: evaluate operand of sizeof if its type is a vla
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Sanimir Agovic <sanimir dot agovic at intel dot com>
- Cc: tromey at redhat dot com, keven dot boell at intel dot com, gdb-patches at sourceware dot org
- Date: Fri, 28 Feb 2014 09:37:04 -0800
- Subject: Re: [PATCH v5 10/15] vla: evaluate operand of sizeof if its type is a vla
- Authentication-results: sourceware.org; auth=none
- References: <1391704056-25246-1-git-send-email-sanimir dot agovic at intel dot com> <1391704056-25246-11-git-send-email-sanimir dot agovic at intel dot com>
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 <sanimir.agovic@intel.com>
> Keven Boell <keven.boell@intel.com>
>
> * eval.c (evaluate_subexp_for_sizeof): Add enum noside argument.
> (evaluate_subexp_standard): Pass noside argument.
> (evaluate_subexp_for_sizeof) <BINOP_SUBSCRIPT>: 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 <sanimir.agovic@intel.com>
> ---
> 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 <http://www.gnu.org/licenses/>. */
> +
> +#include <stddef.h>
> +#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 <http://www.gnu.org/licenses/>.
> +
> +# 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