Re: [PATCH v5 10/15] vla: evaluate operand of sizeof if its type is a vla

[Joel Brobecker <brobecker at adacore 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