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

["Agovic, Sanimir" <sanimir dot agovic at intel dot com>]

Thanks for your review.

> >  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.
> 
If the operand passed to sizeof evaluates to a variable length array type then 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 evaluated at runtime
sizeof(vla[0][0])   // Operand type is an int, size is evaluated at compile time 

If the expression contains a side effects it is evaluated too

sizeof(vla[i++])    // i gets modified as the operand is evaluated at runtime
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  <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?
> 
Done

> >  {
> >    /* 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...
> 
> 
I will try, please have a look at the upcoming v6.

> > +	  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.
> 
Both done.

> > +
> > +	  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.  */
> 
Done.

> 
> > +
> >      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?
> 
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/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.
> 
Done.
> > +{
> > +  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
> 
Done.

> > +# 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?
> 
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+ = ${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?
> 
Done.

> > +         "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
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