[patch] GNU vector unop support

[patch] GNU vector unop support

[Ken Werner]

[-- Attachment #1: Type: Text/Plain, Size: 374 bytes --]

Hi,

This patch implements some unary operations for GNU vectors.
The following operators have been verified:
- pre/post increment (++)
- pre/post decrement (--)
- unary plus (+)
- unary minus (-)
- bitwise not/complement (~)

It also prevents GDB from returning a lvalue in case of post 
increment/decrement.
Tested on i686-*-linux-gnu, no regressions.

Regards
Ken Werner

[-- Attachment #2: vec_unop.patch --]
[-- Type: text/x-patch, Size: 10337 bytes --]

ChangeLog:

2010-09-17  Ken Werner  <ken.werner@de.ibm.com>

	* eval.c (evaluate_subexp_standard) <UNOP_POSTINCREMENT,
	UNOP_POSTDECREMENT>: Copy arg1 to prevent returning a lvalue.
	* valarith.c (value_pos, value_neg, value_complement): Handle
	vector types.
	* valops.c (value_one): Likewise.

testsuite/ChangeLog:

2010-09-17  Ken Werner  <ken.werner@de.ibm.com>

	* gdb.base/gnu_vector.exp: Add unary operator tests.


Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.139
diff -p -u -r1.139 eval.c
--- gdb/eval.c	11 Aug 2010 16:48:26 -0000	1.139
+++ gdb/eval.c	16 Sep 2010 10:30:43 -0000
@@ -2739,7 +2739,14 @@ evaluate_subexp_standard (struct type *e
 	}
       else
 	{
-	  if (ptrmath_type_p (exp->language_defn, value_type (arg1)))
+	  type = value_type (arg1);
+	  arg3 = allocate_value (type);
+
+	  /* Copy the value to prevent to return a lvalue.  */
+	  memcpy (value_contents_raw (arg3), value_contents (arg1),
+		 TYPE_LENGTH (type));
+
+	  if (ptrmath_type_p (exp->language_defn, type))
 	    arg2 = value_ptradd (arg1, 1);
 	  else
 	    {
@@ -2751,7 +2758,7 @@ evaluate_subexp_standard (struct type *e
 	    }
 
 	  value_assign (arg1, arg2);
-	  return arg1;
+	  return arg3;
 	}
 
     case UNOP_POSTDECREMENT:
@@ -2764,7 +2771,14 @@ evaluate_subexp_standard (struct type *e
 	}
       else
 	{
-	  if (ptrmath_type_p (exp->language_defn, value_type (arg1)))
+	  type = value_type (arg1);
+	  arg3 = allocate_value (type);
+
+	  /* Copy the value to prevent to return a lvalue.  */
+	  memcpy (value_contents_raw (arg3), value_contents (arg1),
+		 TYPE_LENGTH (type));
+
+	  if (ptrmath_type_p (exp->language_defn, type))
 	    arg2 = value_ptradd (arg1, -1);
 	  else
 	    {
@@ -2776,7 +2790,7 @@ evaluate_subexp_standard (struct type *e
 	    }
 
 	  value_assign (arg1, arg2);
-	  return arg1;
+	  return arg3;
 	}
 
     case OP_THIS:
Index: gdb/valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.86
diff -p -u -r1.86 valarith.c
--- gdb/valarith.c	11 Aug 2010 16:48:26 -0000	1.86
+++ gdb/valarith.c	16 Sep 2010 10:30:43 -0000
@@ -1673,39 +1673,58 @@ value_less (struct value *arg1, struct v
 struct value *
 value_pos (struct value *arg1)
 {
+  struct value *val;
   struct type *type;
 
   arg1 = coerce_ref (arg1);
   type = check_typedef (value_type (arg1));
 
-  if (TYPE_CODE (type) == TYPE_CODE_FLT)
-    return value_from_double (type, value_as_double (arg1));
-  else if (TYPE_CODE (type) == TYPE_CODE_DECFLOAT)
-    return value_from_decfloat (type, value_contents (arg1));
-  else if (is_integral_type (type))
+  if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_VECTOR (type))
     {
-      return value_from_longest (type, value_as_long (arg1));
+      val = allocate_value (type);
+      memcpy (value_contents_raw (val), value_contents (arg1),
+	      TYPE_LENGTH (type));
     }
+  else if (TYPE_CODE (type) == TYPE_CODE_FLT)
+    val = value_from_double (type, value_as_double (arg1));
+  else if (TYPE_CODE (type) == TYPE_CODE_DECFLOAT)
+    val = value_from_decfloat (type, value_contents (arg1));
+  else if (is_integral_type (type))
+    val = value_from_longest (type, value_as_long (arg1));
   else
-    {
       error ("Argument to positive operation not a number.");
-      return 0;			/* For lint -- never reached */
-    }
+
+  return val;
 }
 
 struct value *
 value_neg (struct value *arg1)
 {
+  struct value *val;
   struct type *type;
 
   arg1 = coerce_ref (arg1);
   type = check_typedef (value_type (arg1));
 
-  if (TYPE_CODE (type) == TYPE_CODE_DECFLOAT)
+  if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_VECTOR (type))
+    {
+      struct type *eltype = check_typedef (TYPE_TARGET_TYPE (type));
+      int i, n = TYPE_LENGTH (type) / TYPE_LENGTH (eltype);
+      struct value *tmp;
+
+      val = allocate_value (type);
+      for (i = 0; i < n; i++)
+	{
+	  tmp = value_neg (value_subscript (arg1, i));
+	  memcpy (value_contents_writeable (val) + i * TYPE_LENGTH (eltype),
+		  value_contents_all (tmp), TYPE_LENGTH (eltype));
+	}
+    }
+  else if (TYPE_CODE (type) == TYPE_CODE_DECFLOAT)
     {
-      struct value *val = allocate_value (type);
       int len = TYPE_LENGTH (type);
       gdb_byte decbytes[16];  /* a decfloat is at most 128 bits long */
+      val = allocate_value (type);
 
       memcpy (decbytes, value_contents (arg1), len);
 
@@ -1715,33 +1734,53 @@ value_neg (struct value *arg1)
 	decbytes[0] = decbytes[0] | 0x80;
 
       memcpy (value_contents_raw (val), decbytes, len);
-      return val;
+
     }
   else if (TYPE_CODE (type) == TYPE_CODE_FLT)
-    return value_from_double (type, -value_as_double (arg1));
+    val = value_from_double (type, -value_as_double (arg1));
   else if (is_integral_type (type))
-    {
-      return value_from_longest (type, -value_as_long (arg1));
-    }
+    val = value_from_longest (type, -value_as_long (arg1));
   else
-    {
-      error (_("Argument to negate operation not a number."));
-      return 0;			/* For lint -- never reached */
-    }
+    error (_("Argument to negate operation not a number."));
+
+  return val;
 }
 
 struct value *
 value_complement (struct value *arg1)
 {
   struct type *type;
+  struct value *val;
 
   arg1 = coerce_ref (arg1);
   type = check_typedef (value_type (arg1));
 
-  if (!is_integral_type (type))
-    error (_("Argument to complement operation not an integer or boolean."));
-
-  return value_from_longest (type, ~value_as_long (arg1));
+  if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_VECTOR (type))
+    {
+      struct type *eltype = check_typedef (TYPE_TARGET_TYPE (type));
+      int i, n = TYPE_LENGTH (type) / TYPE_LENGTH (eltype);
+      struct value *tmp;
+
+      if (!is_integral_type (eltype))
+	error(_("\
+Argument to complement operation not an integer or boolean vector."));
+
+      val = allocate_value (type);
+      for (i = 0; i < n; i++)
+	{
+	  tmp = value_complement (value_subscript (arg1, i));
+	  memcpy (value_contents_writeable (val) + i * TYPE_LENGTH (eltype),
+		  value_contents_all (tmp), TYPE_LENGTH (eltype));
+	}
+    }
+  else
+    {
+      if (!is_integral_type (type))
+	error (_
+	       ("Argument to complement operation not an integer or boolean."));
+      val = value_from_longest (type, ~value_as_long (arg1));
+    }
+  return val;
 }
 \f
 /* The INDEX'th bit of SET value whose value_type is TYPE,
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.249
diff -p -u -r1.249 valops.c
--- gdb/valops.c	14 Jul 2010 14:54:58 -0000	1.249
+++ gdb/valops.c	16 Sep 2010 10:30:43 -0000
@@ -421,7 +421,8 @@ value_cast (struct type *type, struct va
     }
 
   if (current_language->c_style_arrays
-      && TYPE_CODE (type2) == TYPE_CODE_ARRAY)
+      && TYPE_CODE (type2) == TYPE_CODE_ARRAY
+      && ! TYPE_VECTOR (type2))
     arg2 = value_coerce_array (arg2);
 
   if (TYPE_CODE (type2) == TYPE_CODE_FUNC)
@@ -833,7 +834,20 @@ value_one (struct type *type, enum lval_
   struct type *type1 = check_typedef (type);
   struct value *val;
 
-  if (TYPE_CODE (type1) == TYPE_CODE_DECFLOAT)
+  if (TYPE_CODE (type1) == TYPE_CODE_ARRAY && TYPE_VECTOR (type1))
+    {
+      struct type *eltype = check_typedef (TYPE_TARGET_TYPE (type1));
+      int i, n = TYPE_LENGTH (type1) / TYPE_LENGTH (eltype);
+      struct value *tmp;
+      val = allocate_value (type);
+      for (i = 0; i < n; i++)
+	{
+	  tmp = value_one (eltype, lv);
+	  memcpy (value_contents_writeable (val) + i * TYPE_LENGTH (eltype),
+		  value_contents_all (tmp), TYPE_LENGTH (eltype));
+	}
+    }
+  else if (TYPE_CODE (type1) == TYPE_CODE_DECFLOAT)
     {
       enum bfd_endian byte_order = gdbarch_byte_order (get_type_arch (type));
       gdb_byte v[16];
Index: gdb/testsuite/gdb.base/gnu_vector.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/gnu_vector.exp,v
retrieving revision 1.1
diff -p -u -r1.1 gnu_vector.exp
--- gdb/testsuite/gdb.base/gnu_vector.exp	11 Aug 2010 16:48:26 -0000	1.1
+++ gdb/testsuite/gdb.base/gnu_vector.exp	16 Sep 2010 10:30:43 -0000
@@ -46,7 +46,7 @@ if { ![runto main] } {
     return -1
 }
 
-# Test binary operators on integer vector types
+# Test operators on integer vector types
 gdb_test "print i4a" "\\\$$decimal = \\{2, 4, 8, 16\\}"
 gdb_test "print i4b" "\\\$$decimal = \\{1, 2, 8, 4\\}"
 # Arithmetic operators
@@ -55,15 +55,23 @@ gdb_test "print i4a - i4b" "\\\$$decimal
 gdb_test "print i4a * i4b" "\\\$$decimal = \\{2, 8, 64, 64\\}"
 gdb_test "print i4a / i4b" "\\\$$decimal = \\{2, 2, 1, 4\\}"
 gdb_test "print i4a % i4b" "\\\$$decimal = \\{0, 0, 0, 0\\}"
+gdb_test "print i4a++" "\\\$$decimal = \\{2, 4, 8, 16\\}"
+gdb_test "print ++i4a" "\\\$$decimal = \\{4, 6, 10, 18\\}"
+gdb_test "print i4a--" "\\\$$decimal = \\{4, 6, 10, 18\\}"
+gdb_test "print --i4a" "\\\$$decimal = \\{2, 4, 8, 16\\}"
+gdb_test "print +i4a" "\\\$$decimal = \\{2, 4, 8, 16\\}"
+gdb_test "print -i4a" "\\\$$decimal = \\{-2, -4, -8, -16\\}"
+
 # Bitwise operators
 gdb_test "print i4a & i4b" "\\\$$decimal = \\{0, 0, 8, 0\\}"
 gdb_test "print i4a | i4b" "\\\$$decimal = \\{3, 6, 8, 20\\}"
 gdb_test "print i4a ^ i4b" "\\\$$decimal = \\{3, 6, 0, 20\\}"
+gdb_test "print ~i4a" "\\\$$decimal = \\{-3, -5, -9, -17\\}"
 # Shift operators
 gdb_test "print i4a << i4b" "\\\$$decimal = \\{4, 16, 2048, 256\\}"
 gdb_test "print i4a >> i4b" "\\\$$decimal = \\{1, 1, 0, 1\\}"
 
-# Test binary operators on floating point vector types
+# Test operators on floating point vector types
 gdb_test "print f4a" "\\\$$decimal = \\{2, 4, 8, 16\\}"
 gdb_test "print f4b" "\\\$$decimal = \\{1, 2, 8, 4\\}"
 # Arithmetic operators
@@ -71,6 +79,8 @@ gdb_test "print f4a + f4b" "\\\$$decimal
 gdb_test "print f4a - f4b" "\\\$$decimal = \\{1, 2, 0, 12\\}"
 gdb_test "print f4a * f4b" "\\\$$decimal = \\{2, 8, 64, 64\\}"
 gdb_test "print f4a / f4b" "\\\$$decimal = \\{2, 2, 1, 4\\}"
+gdb_test "print +f4a" "\\\$$decimal = \\{2, 4, 8, 16\\}"
+gdb_test "print -f4a" "\\\$$decimal = \\{-2, -4, -8, -16\\}"
 
 # Test error conditions
 gdb_test "print i4a + 1" "Vector operations are only supported among vectors"

Re: [patch] GNU vector unop support

[Ken Werner]

On Friday, September 17, 2010 10:33:35 am Ken Werner wrote:
> This patch implements some unary operations for GNU vectors.
> The following operators have been verified:
> - pre/post increment (++)
> - pre/post decrement (--)
> - unary plus (+)
> - unary minus (-)
> - bitwise not/complement (~)
> 
> It also prevents GDB from returning a lvalue in case of post
> increment/decrement.
> Tested on i686-*-linux-gnu, no regressions.

Ping.
-ken

[patch] fix pre-/post- in-/decrement

[Ken Werner]

[-- Attachment #1: Type: Text/Plain, Size: 659 bytes --]

On Thursday, September 30, 2010 8:56:34 pm Joel Brobecker wrote:
> Would you mind if we treated this as a separate patch, on top of
> the current patch? I'd also like to add a test that verifies this,
> hopefully not involving vector types.

The attached patch prevents the pre-/post- in-/decrement operators from 
returning  lvalues.  The following gdb command for example should not assign a 
value to the result of the post increment operation but throw an error:
  print i++ = 3;
I've added a few tests to the testsuite. Please let me know if there is a 
better place to put them.
Tested on i686-*-linux-gnu with no regressions. OK to apply?

Thanks
-ken

[-- Attachment #2: pre_post_in_decrement.patch --]
[-- Type: text/x-patch, Size: 3685 bytes --]

ChangeLog:

2010-10-01  Ken Werner  <ken.werner@de.ibm.com>

	* eval.c evaluate_subexp_standard) <UNOP_PREINCREMENT,
	UNOP_PREDECREMENT> Set VALUE_LVAL of the result to not_lval.
	<UNOP_POSTINCREMENT, UNOP_POSTDECREMENT>: Copy arg1 to prevent
	returning a lvalue.

testsuite/ChangeLog:

2010-10-01  Ken Werner  <ken.werner@de.ibm.com>

	* gdb.base/exprs.exp: Add tests for pre-/post- in-/decrement operators.


Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.139
diff -p -u -r1.139 eval.c
--- gdb/eval.c	11 Aug 2010 16:48:26 -0000	1.139
+++ gdb/eval.c	1 Oct 2010 17:31:16 -0000
@@ -2702,7 +2702,10 @@ evaluate_subexp_standard (struct type *e
 	      arg2 = value_binop (tmp, arg2, BINOP_ADD);
 	    }
 
-	  return value_assign (arg1, arg2);
+	  /* Prevent to return a lvalue.  */
+	  arg3 = value_assign (arg1, arg2);
+	  VALUE_LVAL (arg3) = not_lval;
+	  return arg3;
 	}
 
     case UNOP_PREDECREMENT:
@@ -2726,7 +2729,10 @@ evaluate_subexp_standard (struct type *e
 	      arg2 = value_binop (tmp, arg2, BINOP_SUB);
 	    }
 
-	  return value_assign (arg1, arg2);
+	  /* Prevent to return a lvalue.  */
+	  arg3 = value_assign (arg1, arg2);
+	  VALUE_LVAL (arg3) = not_lval;
+	  return arg3;
 	}
 
     case UNOP_POSTINCREMENT:
@@ -2739,7 +2745,14 @@ evaluate_subexp_standard (struct type *e
 	}
       else
 	{
-	  if (ptrmath_type_p (exp->language_defn, value_type (arg1)))
+	  type = value_type (arg1);
+	  arg3 = allocate_value (type);
+
+	  /* Copy the value to prevent to return a lvalue.  */
+	  memcpy (value_contents_raw (arg3), value_contents (arg1),
+		  TYPE_LENGTH (type));
+
+	  if (ptrmath_type_p (exp->language_defn, type))
 	    arg2 = value_ptradd (arg1, 1);
 	  else
 	    {
@@ -2751,7 +2764,7 @@ evaluate_subexp_standard (struct type *e
 	    }
 
 	  value_assign (arg1, arg2);
-	  return arg1;
+	  return arg3;
 	}
 
     case UNOP_POSTDECREMENT:
@@ -2764,7 +2777,14 @@ evaluate_subexp_standard (struct type *e
 	}
       else
 	{
-	  if (ptrmath_type_p (exp->language_defn, value_type (arg1)))
+	  type = value_type (arg1);
+	  arg3 = allocate_value (type);
+
+	  /* Copy the value to prevent to return a lvalue.  */
+	  memcpy (value_contents_raw (arg3), value_contents (arg1),
+		  TYPE_LENGTH (type));
+
+	  if (ptrmath_type_p (exp->language_defn, type))
 	    arg2 = value_ptradd (arg1, -1);
 	  else
 	    {
@@ -2776,7 +2796,7 @@ evaluate_subexp_standard (struct type *e
 	    }
 
 	  value_assign (arg1, arg2);
-	  return arg1;
+	  return arg3;
 	}
 
     case OP_THIS:
Index: gdb/testsuite/gdb.base/exprs.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/exprs.exp,v
retrieving revision 1.19
diff -p -u -r1.19 exprs.exp
--- gdb/testsuite/gdb.base/exprs.exp	10 Jun 2010 19:48:19 -0000	1.19
+++ gdb/testsuite/gdb.base/exprs.exp	1 Oct 2010 17:31:16 -0000
@@ -253,3 +253,14 @@ gdb_test "set output-radix 8" ".*"
 test_expr "print red" "\\$\[0-9\]* = red"
 test_expr "print/d red" "\\$\[0-9\]* = 0"
 gdb_test "set output-radix 10" ".*"
+
+# Pre-/post in-/decrement tests.
+gdb_test "set variable v_int = 1" ""
+gdb_test "print v_int++" "\\$\[0-9\]* = 1"
+gdb_test "print ++v_int" "\\$\[0-9\]* = 3"
+gdb_test "print v_int--" "\\$\[0-9\]* = 3"
+gdb_test "print --v_int" "\\$\[0-9\]* = 1"
+gdb_test "print v_int++ = 5" "Left operand of assignment is not an lvalue."
+gdb_test "print ++v_int = 5" "Left operand of assignment is not an lvalue."
+gdb_test "print v_int-- = 5" "Left operand of assignment is not an lvalue."
+gdb_test "print --v_int = 5" "Left operand of assignment is not an lvalue."

Re: [patch] fix pre-/post- in-/decrement

[Ulrich Weigand]

Ken Werner wrote:

> -	  return value_assign (arg1, arg2);
> +	  /* Prevent to return a lvalue.  */
> +	  arg3 = value_assign (arg1, arg2);
> +	  VALUE_LVAL (arg3) = not_lval;
> +	  return arg3;

We want to get away from changing core properties like lval
in values after the fact ...   In any case, hard-coding the
lval to non_lval without any further change can cause problems,
e.g. if the value is lazy.

I think there is a more general issue underlying this particular
change.  You're right that the result of a preincrement should
not be an lvalue.  But the same is true for results of assignment
operators in general.

Note that value_assign is used only to implement such operators
(simple assignment, compound assignment, pre/postfix operators).
Since *all* of them return non-lvalues, it might make sense to
simply change value_assign to return a non-lvalue in the first
place ...


> +	  type = value_type (arg1);
> +	  arg3 = allocate_value (type);
> +
> +	  /* Copy the value to prevent to return a lvalue.  */
> +	  memcpy (value_contents_raw (arg3), value_contents (arg1),
> +		  TYPE_LENGTH (type));

I'd prefer to encapsulate this in a function, e.g. value_non_lval (...)
or a similar name, which returns a version of the value that is non_lval.
This could then address a couple of additional issues:
- if the value is already non_lval, no need to create an extra copy
- it might be better to copy the full contents / enclosing type
  for C++ objects

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] fix pre-/post- in-/decrement

[Ken Werner]

[-- Attachment #1: Type: Text/Plain, Size: 2259 bytes --]

On Monday, October 04, 2010 3:01:22 pm Ulrich Weigand wrote:
> Ken Werner wrote:
> > -	  return value_assign (arg1, arg2);
> > +	  /* Prevent to return a lvalue.  */
> > +	  arg3 = value_assign (arg1, arg2);
> > +	  VALUE_LVAL (arg3) = not_lval;
> > +	  return arg3;
> 
> We want to get away from changing core properties like lval
> in values after the fact ...   In any case, hard-coding the
> lval to non_lval without any further change can cause problems,
> e.g. if the value is lazy.

Thank you for looking into the patch and clarifying things.

> I think there is a more general issue underlying this particular
> change.  You're right that the result of a preincrement should
> not be an lvalue.  But the same is true for results of assignment
> operators in general.

Interesting - fixed. : )

> Note that value_assign is used only to implement such operators
> (simple assignment, compound assignment, pre/postfix operators).
> Since *all* of them return non-lvalues, it might make sense to
> simply change value_assign to return a non-lvalue in the first
> place ...

Sounds good because value_assign would match the C semantic then. 
Unfortunately the varobj.c:varobj_set_value function (through the 
gdb_value_assign wrapper) seems to rely on the return value of value_assing 
beeing a lvalue. Therefore I decided to adjust the places where value_assign 
is called.

> > +	  type = value_type (arg1);
> > +	  arg3 = allocate_value (type);
> > +
> > +	  /* Copy the value to prevent to return a lvalue.  */
> > +	  memcpy (value_contents_raw (arg3), value_contents (arg1),
> > +		  TYPE_LENGTH (type));
> 
> I'd prefer to encapsulate this in a function, e.g. value_non_lval (...)
> or a similar name, which returns a version of the value that is non_lval.
> This could then address a couple of additional issues:
> - if the value is already non_lval, no need to create an extra copy
> - it might be better to copy the full contents / enclosing type
>   for C++ objects

The attached patch introduces a new function called value_non_lval that 
returns a non-lval version of the given value. This function is called prior 
the simple assignment, compound assignment and pre/postfix routines return. Any 
comments are appreciated.

Regards
Ken Werner

[-- Attachment #2: pre_post_in_decrement.patch --]
[-- Type: text/x-patch, Size: 5347 bytes --]

ChangeLog:

2010-10-04  Ken Werner  <ken.werner@de.ibm.com>

	* value.h (value_non_lval): Declare.
	* value.c (value_non_lval): New function.
	* eval.c (evaluate_subexp_standard) <BINOP_ASSIGN, BINOP_ASSIGN_MODIFY,
	UNOP_PREINCREMENT, UNOP_PREDECREMENT, UNOP_POSTINCREMENT,
	UNOP_POSTDECREMENT>: Call value_non_lval to ensure to return a
	non-lvalue.

testsuite/ChangeLog:

2010-10-04  Ken Werner  <ken.werner@de.ibm.com>

	* gdb.base/exprs.exp: Add tests for pre-/post- in-/decrement operators.


Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.139
diff -p -u -r1.139 eval.c
--- gdb/eval.c	11 Aug 2010 16:48:26 -0000	1.139
+++ gdb/eval.c	4 Oct 2010 18:11:09 -0000
@@ -2010,7 +2010,7 @@ evaluate_subexp_standard (struct type *e
       if (binop_user_defined_p (op, arg1, arg2))
 	return value_x_binop (arg1, arg2, op, OP_NULL, noside);
       else
-	return value_assign (arg1, arg2);
+	return value_non_lval (value_assign (arg1, arg2));
 
     case BINOP_ASSIGN_MODIFY:
       (*pos) += 2;
@@ -2043,7 +2043,7 @@ evaluate_subexp_standard (struct type *e
 
 	  arg2 = value_binop (tmp, arg2, op);
 	}
-      return value_assign (arg1, arg2);
+      return value_non_lval (value_assign (arg1, arg2));
 
     case BINOP_ADD:
       arg1 = evaluate_subexp_with_coercion (exp, pos, noside);
@@ -2702,7 +2702,7 @@ evaluate_subexp_standard (struct type *e
 	      arg2 = value_binop (tmp, arg2, BINOP_ADD);
 	    }
 
-	  return value_assign (arg1, arg2);
+	  return value_non_lval (value_assign (arg1, arg2));
 	}
 
     case UNOP_PREDECREMENT:
@@ -2726,7 +2726,7 @@ evaluate_subexp_standard (struct type *e
 	      arg2 = value_binop (tmp, arg2, BINOP_SUB);
 	    }
 
-	  return value_assign (arg1, arg2);
+	  return value_non_lval (value_assign (arg1, arg2));
 	}
 
     case UNOP_POSTINCREMENT:
@@ -2739,6 +2739,8 @@ evaluate_subexp_standard (struct type *e
 	}
       else
 	{
+	  arg3 = value_non_lval (arg1);
+	  
 	  if (ptrmath_type_p (exp->language_defn, value_type (arg1)))
 	    arg2 = value_ptradd (arg1, 1);
 	  else
@@ -2751,7 +2753,7 @@ evaluate_subexp_standard (struct type *e
 	    }
 
 	  value_assign (arg1, arg2);
-	  return arg1;
+	  return arg3;
 	}
 
     case UNOP_POSTDECREMENT:
@@ -2764,6 +2766,8 @@ evaluate_subexp_standard (struct type *e
 	}
       else
 	{
+	  arg3 = value_non_lval (arg1);
+	  
 	  if (ptrmath_type_p (exp->language_defn, value_type (arg1)))
 	    arg2 = value_ptradd (arg1, -1);
 	  else
@@ -2776,7 +2780,7 @@ evaluate_subexp_standard (struct type *e
 	    }
 
 	  value_assign (arg1, arg2);
-	  return arg1;
+	  return arg3;
 	}
 
     case OP_THIS:
Index: gdb/value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.113
diff -p -u -r1.113 value.c
--- gdb/value.c	30 Sep 2010 18:58:07 -0000	1.113
+++ gdb/value.c	4 Oct 2010 18:11:09 -0000
@@ -826,6 +826,26 @@ value_copy (struct value *arg)
   return val;
 }
 
+/* Return a version of ARG that is non-lvalue.  */
+
+struct value *
+value_non_lval (struct value *arg)
+{
+  if (VALUE_LVAL (arg) != not_lval)
+    {
+      struct type *enc_type = value_enclosing_type (arg);
+      struct value *val = allocate_value (enc_type);
+  
+      memcpy (value_contents_all_raw (val), value_contents_all (arg),
+	      TYPE_LENGTH (enc_type));
+      val->type = arg->type;
+      set_value_embedded_offset (val, value_embedded_offset (arg));
+      set_value_pointed_to_offset (val, value_pointed_to_offset (arg));
+      return val;
+    }
+   return arg;
+}
+
 void
 set_value_component_location (struct value *component,
 			      const struct value *whole)
Index: gdb/value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.161
diff -p -u -r1.161 value.h
--- gdb/value.h	7 Jul 2010 16:15:18 -0000	1.161
+++ gdb/value.h	4 Oct 2010 18:11:09 -0000
@@ -710,6 +710,8 @@ extern void preserve_values (struct objf
 
 extern struct value *value_copy (struct value *);
 
+extern struct value *value_non_lval (struct value *);
+
 extern void preserve_one_value (struct value *, struct objfile *, htab_t);
 
 /* From valops.c */
Index: gdb/testsuite/gdb.base/exprs.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/exprs.exp,v
retrieving revision 1.19
diff -p -u -r1.19 exprs.exp
--- gdb/testsuite/gdb.base/exprs.exp	10 Jun 2010 19:48:19 -0000	1.19
+++ gdb/testsuite/gdb.base/exprs.exp	4 Oct 2010 18:11:09 -0000
@@ -253,3 +253,14 @@ gdb_test "set output-radix 8" ".*"
 test_expr "print red" "\\$\[0-9\]* = red"
 test_expr "print/d red" "\\$\[0-9\]* = 0"
 gdb_test "set output-radix 10" ".*"
+
+# Pre-/post in-/decrement tests.
+gdb_test "set variable v_int = 1" ""
+gdb_test "print v_int++" "\\$\[0-9\]* = 1"
+gdb_test "print ++v_int" "\\$\[0-9\]* = 3"
+gdb_test "print v_int--" "\\$\[0-9\]* = 3"
+gdb_test "print --v_int" "\\$\[0-9\]* = 1"
+gdb_test "print v_int++ = 5" "Left operand of assignment is not an lvalue."
+gdb_test "print ++v_int = 5" "Left operand of assignment is not an lvalue."
+gdb_test "print v_int-- = 5" "Left operand of assignment is not an lvalue."
+gdb_test "print --v_int = 5" "Left operand of assignment is not an lvalue."

Re: [patch] fix pre-/post- in-/decrement

[Daniel Jacobowitz]

On Mon, Oct 04, 2010 at 09:46:50PM +0200, Ken Werner wrote:
> The attached patch introduces a new function called value_non_lval that 
> returns a non-lval version of the given value. This function is called prior 
> the simple assignment, compound assignment and pre/postfix routines return. Any 
> comments are appreciated.

I think this will have an undesired effect.

Try this - it's easiest to see what's going on with set debug target
1, or else by using gdbserver and set debug remote 1 (the latter is
easier for me to read).  I've edited out some reads relating to the
stack frame.

(gdb) set $p = (int *) $sp
(gdb) p *$p
Sending packet: $m7fffffffdff0,4#2e...Packet received: 01000000
$4 = 1
(gdb) set *$p = 2
Sending packet: $X7fffffffdff0,4:\002\000\000\000#55...Packet received: OK
(gdb) print *$p = 1
Sending packet: $X7fffffffdff0,4:\001\000\000\000#54...Packet received: OK
Sending packet: $m7fffffffdff0,4#2e...Packet received: 01000000
$5 = 1
(gdb)

I suspect that your patch will be called to handle the value_assign
for the set command, and it will result in an unnecessary read from
the target.  You want the resulting value to be unwritable, but it can
still be lazy; you don't need the value.

Of course, those two things are not orthogonal in GDB's current
representation.  So I don't know how to do this.  But don't break the
current behavior, please - it's important both for performance and for
embedded correctness.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] GNU vector unop support

[Ken Werner]

[-- Attachment #1: Type: Text/Plain, Size: 3407 bytes --]

On Thursday, September 30, 2010 8:56:34 pm Joel Brobecker wrote:
> > 	* eval.c (evaluate_subexp_standard) <UNOP_POSTINCREMENT,
> > 	UNOP_POSTDECREMENT>: Copy arg1 to prevent returning a lvalue.
> 
> Would you mind if we treated this as a separate patch, on top of
> the current patch? I'd also like to add a test that verifies this,
> hopefully not involving vector types.

Ok, good point - here we go:
http://sourceware.org/ml/gdb-patches/2010-10/msg00012.html

> You also seem to do cleanups at the same time, which is OK. But
> I'd rather they be submitted separately. It's not all that bad,
> but it does tend to pollute a bit the meat of the change you are
> trying to make.  Fortunately, tools such as git make it really easy
> to manage (I know Dan an Pedro use something different, but I just
> can't remember the name of that tool).

Thanks for the advice. The new patch changes only what I think is absolutely 
necessary in order to implement the wanted functionality.

> > +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_VECTOR (type))
> > 
> >      {
> > 
> > -      return value_from_longest (type, value_as_long (arg1));
> > +      val = allocate_value (type);
> > +      memcpy (value_contents_raw (val), value_contents (arg1),
> > +	      TYPE_LENGTH (type));
> > 
> >      }
> 
> I'd rather you put the check for vector types at the end of the
> if/elsif sequence. I think that vector types are going to be less
> common than integers, floats et al.  This is true for all the changes
> you made.

Sounds reasonable. Fixed.

> One question: Is it possible to have a non-array vector type?
> In other words, can we just check for TYPE_VECTOR (type) instead
> of TYPE_CODE (type) and TYPE_VECTOR (type)?

No, not that I'm aware of. Even a GNU Vector with a single element only is an 
array underneath. My understanding is that querying the flag_vector is only 
legal if the type is an array. Historically vectors have been treated as 
arrays and there were only a few cases within GDB where arrays and vector were 
handled differently. There was no need to introduce a new type code. The more I 
do with vectors within GDB the more I think it would be nice to have such a 
distinct type code.

> Please move the assignment to `val' one line below (empty line after
> the declarations, and no need for an empty line between the two
> statements).

Ok.

> Unnecessary empty line?

Yep. Thanks.

> > @@ -421,7 +421,8 @@ value_cast (struct type *type, struct va
> > 
> >      }
> >    
> >    if (current_language->c_style_arrays
> > 
> > -      && TYPE_CODE (type2) == TYPE_CODE_ARRAY)
> > +      && TYPE_CODE (type2) == TYPE_CODE_ARRAY
> > +      && ! TYPE_VECTOR (type2))
> > 
> >      arg2 = value_coerce_array (arg2);
> 
> You forgot to mention this change in you ChangeLog, and I don't
> understand why it's necessary? Can you elaborate? (and add a test
> if appropriate)

Good catch. I thought that value_cast should not call value_coerce_array for 
vectors. You probably don't want a value which is a pointer to the first 
element in that case because vectors aren't arrays. Since this has nothing 
particularly to do with what the patch intended to improve I removed this 
chunk.

> Empty line after the declarations.
Fixed.

This new patch depends on the lval-fix posted here:
http://sourceware.org/ml/gdb-patches/2010-10/msg00029.html
Thanks for looking into it.

Regards
Ken Werner

[-- Attachment #2: vec_unop.patch --]
[-- Type: text/x-patch, Size: 6293 bytes --]

ChangeLog:

2010-10-04  Ken Werner  <ken.werner@de.ibm.com>

	* valarith.c (value_pos, value_neg, value_complement): Handle
	vector types.
	* valops.c (value_one): Likewise.

testsuite/ChangeLog:

2010-10-04  Ken Werner  <ken.werner@de.ibm.com>

	* gdb.base/gnu_vector.exp: Add unary operator tests.


Index: gdb/valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.86
diff -p -u -r1.86 valarith.c
--- gdb/valarith.c	11 Aug 2010 16:48:26 -0000	1.86
+++ gdb/valarith.c	4 Oct 2010 20:48:52 -0000
@@ -1686,6 +1686,14 @@ value_pos (struct value *arg1)
     {
       return value_from_longest (type, value_as_long (arg1));
     }
+  else if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_VECTOR (type))
+    {
+      struct value *val = allocate_value (type);
+
+      memcpy (value_contents_raw (val), value_contents (arg1),
+              TYPE_LENGTH (type));
+      return val;
+    }
   else
     {
       error ("Argument to positive operation not a number.");
@@ -1723,6 +1731,20 @@ value_neg (struct value *arg1)
     {
       return value_from_longest (type, -value_as_long (arg1));
     }
+  else if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_VECTOR (type))
+    {
+      struct value *tmp, *val = allocate_value (type);
+      struct type *eltype = check_typedef (TYPE_TARGET_TYPE (type));
+      int i, n = TYPE_LENGTH (type) / TYPE_LENGTH (eltype);
+
+      for (i = 0; i < n; i++)
+	{
+	  tmp = value_neg (value_subscript (arg1, i));
+	  memcpy (value_contents_writeable (val) + i * TYPE_LENGTH (eltype),
+		  value_contents_all (tmp), TYPE_LENGTH (eltype));
+	}
+      return val;
+    }
   else
     {
       error (_("Argument to negate operation not a number."));
@@ -1734,14 +1756,31 @@ struct value *
 value_complement (struct value *arg1)
 {
   struct type *type;
+  struct value *val;
 
   arg1 = coerce_ref (arg1);
   type = check_typedef (value_type (arg1));
 
-  if (!is_integral_type (type))
-    error (_("Argument to complement operation not an integer or boolean."));
+  if (is_integral_type (type))
+    val = value_from_longest (type, ~value_as_long (arg1));
+  else if (TYPE_CODE (type) == TYPE_CODE_ARRAY && TYPE_VECTOR (type))
+    {
+      struct value *tmp;
+      struct type *eltype = check_typedef (TYPE_TARGET_TYPE (type));
+      int i, n = TYPE_LENGTH (type) / TYPE_LENGTH (eltype);
+
+      val = allocate_value (type);
+      for (i = 0; i < n; i++)
+        {
+          tmp = value_complement (value_subscript (arg1, i));
+          memcpy (value_contents_writeable (val) + i * TYPE_LENGTH (eltype),
+                  value_contents_all (tmp), TYPE_LENGTH (eltype));
+        }
+    }
+  else
+    error (_("Argument to complement operation not an integer, boolean."));
 
-  return value_from_longest (type, ~value_as_long (arg1));
+  return val;
 }
 \f
 /* The INDEX'th bit of SET value whose value_type is TYPE,
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.251
diff -p -u -r1.251 valops.c
--- gdb/valops.c	24 Sep 2010 14:47:53 -0000	1.251
+++ gdb/valops.c	4 Oct 2010 20:48:52 -0000
@@ -849,6 +849,20 @@ value_one (struct type *type, enum lval_
     {
       val = value_from_longest (type, (LONGEST) 1);
     }
+  else if (TYPE_CODE (type1) == TYPE_CODE_ARRAY && TYPE_VECTOR (type1))
+    {
+      struct type *eltype = check_typedef (TYPE_TARGET_TYPE (type1));
+      int i, n = TYPE_LENGTH (type1) / TYPE_LENGTH (eltype);
+      struct value *tmp;
+
+      val = allocate_value (type);
+      for (i = 0; i < n; i++)
+	{
+	  tmp = value_one (eltype, lv);
+	  memcpy (value_contents_writeable (val) + i * TYPE_LENGTH (eltype),
+		  value_contents_all (tmp), TYPE_LENGTH (eltype));
+	}
+    }
   else
     {
       error (_("Not a numeric type."));
Index: gdb/testsuite/gdb.base/gnu_vector.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/gnu_vector.exp,v
retrieving revision 1.1
diff -p -u -r1.1 gnu_vector.exp
--- gdb/testsuite/gdb.base/gnu_vector.exp	11 Aug 2010 16:48:26 -0000	1.1
+++ gdb/testsuite/gdb.base/gnu_vector.exp	4 Oct 2010 20:48:53 -0000
@@ -46,7 +46,7 @@ if { ![runto main] } {
     return -1
 }
 
-# Test binary operators on integer vector types
+# Test operators on integer vector types
 gdb_test "print i4a" "\\\$$decimal = \\{2, 4, 8, 16\\}"
 gdb_test "print i4b" "\\\$$decimal = \\{1, 2, 8, 4\\}"
 # Arithmetic operators
@@ -55,15 +55,23 @@ gdb_test "print i4a - i4b" "\\\$$decimal
 gdb_test "print i4a * i4b" "\\\$$decimal = \\{2, 8, 64, 64\\}"
 gdb_test "print i4a / i4b" "\\\$$decimal = \\{2, 2, 1, 4\\}"
 gdb_test "print i4a % i4b" "\\\$$decimal = \\{0, 0, 0, 0\\}"
+gdb_test "print i4a++" "\\\$$decimal = \\{2, 4, 8, 16\\}"
+gdb_test "print ++i4a" "\\\$$decimal = \\{4, 6, 10, 18\\}"
+gdb_test "print i4a--" "\\\$$decimal = \\{4, 6, 10, 18\\}"
+gdb_test "print --i4a" "\\\$$decimal = \\{2, 4, 8, 16\\}"
+gdb_test "print +i4a" "\\\$$decimal = \\{2, 4, 8, 16\\}"
+gdb_test "print -i4a" "\\\$$decimal = \\{-2, -4, -8, -16\\}"
+
 # Bitwise operators
 gdb_test "print i4a & i4b" "\\\$$decimal = \\{0, 0, 8, 0\\}"
 gdb_test "print i4a | i4b" "\\\$$decimal = \\{3, 6, 8, 20\\}"
 gdb_test "print i4a ^ i4b" "\\\$$decimal = \\{3, 6, 0, 20\\}"
+gdb_test "print ~i4a" "\\\$$decimal = \\{-3, -5, -9, -17\\}"
 # Shift operators
 gdb_test "print i4a << i4b" "\\\$$decimal = \\{4, 16, 2048, 256\\}"
 gdb_test "print i4a >> i4b" "\\\$$decimal = \\{1, 1, 0, 1\\}"
 
-# Test binary operators on floating point vector types
+# Test operators on floating point vector types
 gdb_test "print f4a" "\\\$$decimal = \\{2, 4, 8, 16\\}"
 gdb_test "print f4b" "\\\$$decimal = \\{1, 2, 8, 4\\}"
 # Arithmetic operators
@@ -71,6 +79,8 @@ gdb_test "print f4a + f4b" "\\\$$decimal
 gdb_test "print f4a - f4b" "\\\$$decimal = \\{1, 2, 0, 12\\}"
 gdb_test "print f4a * f4b" "\\\$$decimal = \\{2, 8, 64, 64\\}"
 gdb_test "print f4a / f4b" "\\\$$decimal = \\{2, 2, 1, 4\\}"
+gdb_test "print +f4a" "\\\$$decimal = \\{2, 4, 8, 16\\}"
+gdb_test "print -f4a" "\\\$$decimal = \\{-2, -4, -8, -16\\}"
 
 # Test error conditions
 gdb_test "print i4a + 1" "Vector operations are only supported among vectors"

Re: [patch] fix pre-/post- in-/decrement

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> Try this - it's easiest to see what's going on with set debug target
> 1, or else by using gdbserver and set debug remote 1 (the latter is
> easier for me to read).  I've edited out some reads relating to the
> stack frame.
> 
> (gdb) set $p = (int *) $sp
> (gdb) p *$p
> Sending packet: $m7fffffffdff0,4#2e...Packet received: 01000000
> $4 = 1
> (gdb) set *$p = 2
> Sending packet: $X7fffffffdff0,4:\002\000\000\000#55...Packet received: OK
> (gdb) print *$p = 1
> Sending packet: $X7fffffffdff0,4:\001\000\000\000#54...Packet received: OK
> Sending packet: $m7fffffffdff0,4#2e...Packet received: 01000000
> $5 = 1
> (gdb)
> 
> I suspect that your patch will be called to handle the value_assign
> for the set command, and it will result in an unnecessary read from
> the target.  You want the resulting value to be unwritable, but it can
> still be lazy; you don't need the value.
> 
> Of course, those two things are not orthogonal in GDB's current
> representation.  So I don't know how to do this.  But don't break the
> current behavior, please - it's important both for performance and for
> embedded correctness.

It would appear that even the current behavior, as shown in your trace,
already contains an unnecessary load.  There should be no need to perform
a memory read to evaluate "print *$p = 1".

In fact, value_assign contains code that apparently tries to avoid just
that, but it seems this no longer works as expected with lazy values:

  val = value_copy (toval);
  memcpy (value_contents_raw (val), value_contents (fromval),
          TYPE_LENGTH (type));
  deprecated_set_value_type (val, type);
  val = value_change_enclosing_type (val,
                                     value_enclosing_type (fromval));
  set_value_embedded_offset (val, value_embedded_offset (fromval));
  set_value_pointed_to_offset (val, value_pointed_to_offset (fromval));


Note that if "toval" was lazy, "val" is likewise lazy -- but then, its
contents are filled in, and are subsequently completely ignored because
the value is lazy.

The rest of this sequence seems dubious as well.  The type of "toval"
(and hence "val") will already be "type" -- modulo typedefs, but I
don't see why you'd want an assignment operator to remove typedefs.

Changing the enclosing type and embedded offset seems just wrong: the
constructed "val" contents will have only "toval"'s enclosing contents,
not "fromval"'s.  (In addition, if the enclosing types are actually
different, the value_change_enclosing_type will just completely wipe
out "val"'s contents again ...)

The pointed_to_offset call looks OK.  (I'm not sure whether the whole
pointed_to_offset logic is needed at all today, but I guess that's a
different topic ...)


I think the way to fix this problem would be to add a call to
  set_value_lazy (val, 0);
at this point -- we have filled in the contents, and thus the value
indeed is no longer lazy if it ever was.

This would -even without Ken's patch fix- the unnecessary read shown above,
and would have the additional effect that Ken's patch will introduce no
further reads from the target, as all values returned from value_assign
will already be non-lazy.

Thoughts?


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] fix pre-/post- in-/decrement

[Daniel Jacobowitz]

On Mon, Oct 04, 2010 at 11:57:47PM +0200, Ulrich Weigand wrote:
> It would appear that even the current behavior, as shown in your trace,
> already contains an unnecessary load.  There should be no need to perform
> a memory read to evaluate "print *$p = 1".

In fact, we rely on this unintuitive behavior.  If you write to any
kind of memory other than RAM, then it's not possible for GDB to
predict what value was actually written.  Suppose you write a value to
ROM with "print"; GDB should show the old value, to reflect that the
variable was not modified.

I can't think of a good way to test this other than by matching debug
output...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix pre-/post- in-/decrement

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Mon, Oct 04, 2010 at 11:57:47PM +0200, Ulrich Weigand wrote:
> > It would appear that even the current behavior, as shown in your trace,
> > already contains an unnecessary load.  There should be no need to perform
> > a memory read to evaluate "print *$p = 1".
> 
> In fact, we rely on this unintuitive behavior.  If you write to any
> kind of memory other than RAM, then it's not possible for GDB to
> predict what value was actually written.  Suppose you write a value to
> ROM with "print"; GDB should show the old value, to reflect that the
> variable was not modified.

Well, this behavior clearly seems an accident in the current code;
you get this only if the target of the assignment happened to be
a lazy value before value_assign.  For example, while you do get
the extra read after:
   print *$p = 1
you get *no* extra read after:
   print *$p += 1

This seems inconsistent, at the very least.

In any case, I'm wondering a bit why you prefer this behavior; this
seems to have quite unexpected consequences to me:

- If you execute "set *$p = *$q = 0" and the write to *$q fails,
  do you really expect *$p to be set to the old value of *$q
  instead of to 0?

- If *$p is a memory-mapped register where reading and writing have
  different effects, should assigning to *$p really trigger *both*
  a write and a read cycle, even though a C assignment wouldn't?

- In the case you refer to where writing to ROM fails, shouldn't
  we actually get an error thrown anyway?  Writing to an unmapped
  address does that as well ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] fix pre-/post- in-/decrement

[Daniel Jacobowitz]

On Tue, Oct 05, 2010 at 01:24:51AM +0200, Ulrich Weigand wrote:
> Well, this behavior clearly seems an accident in the current code;
> you get this only if the target of the assignment happened to be
> a lazy value before value_assign.  For example, while you do get
> the extra read after:
>    print *$p = 1
> you get *no* extra read after:
>    print *$p += 1
> 
> This seems inconsistent, at the very least.

Yes, yes it is.  I was not aware of this.  I think Vladimir has also
fixed some bugs in this area for varobjs; I don't know if that was
posted yet.

> In any case, I'm wondering a bit why you prefer this behavior; this
> seems to have quite unexpected consequences to me:
> 
> - If you execute "set *$p = *$q = 0" and the write to *$q fails,
>   do you really expect *$p to be set to the old value of *$q
>   instead of to 0?

Yes, I would expect that.  To me, this is roughly "the debugger treats
all pointers as volatile".

> - If *$p is a memory-mapped register where reading and writing have
>   different effects, should assigning to *$p really trigger *both*
>   a write and a read cycle, even though a C assignment wouldn't?

If you say "print", yes.  If you don't want this behavior - often I
don't - then use set.

> - In the case you refer to where writing to ROM fails, shouldn't
>   we actually get an error thrown anyway?  Writing to an unmapped
>   address does that as well ...

Well, possibly.  It depends on the system.  It's very useful to be
able to use GDB to generate bus cycles.  And, for instance, I
sometimes use GDB to poke at a flash device; it's programmable, but
not in the straightforward write-to-memory way that RAM is.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix pre-/post- in-/decrement

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Tue, Oct 05, 2010 at 01:24:51AM +0200, Ulrich Weigand wrote:
> > - If you execute "set *$p = *$q = 0" and the write to *$q fails,
> >   do you really expect *$p to be set to the old value of *$q
> >   instead of to 0?
> 
> Yes, I would expect that.  To me, this is roughly "the debugger treats
> all pointers as volatile".

The thing is, I had interpreted the C standard to read that even if
pointers p and q *are* volatile, a statement like "*p = *q = 0" would
still just trigger two writes, and no reads.

Current GCC implementation agrees with this reading:

This code:

int f (volatile int *p)
{
  return *p = 0;
}

void g (volatile int *p, int *result)
{
  *result = *p = 0;
}

compiles to (using gcc-head -O2 on i386):

f:
        movl    4(%esp), %eax
        movl    $0, (%eax)
        xorl    %eax, %eax
        ret

g:
        movl    4(%esp), %eax
        movl    $0, (%eax)
        movl    8(%esp), %eax
        movl    $0, (%eax)
        ret


However, it seems this was not always completely clear, and until very
recently, the implementation in GCC was not quite consistent either:

E.g. using Ubuntu 4.4.3 we get:

f:
        movl    4(%esp), %eax
        movl    $0, (%eax)
        xorl    %eax, %eax      <<= retval is hard-coded to 0
        ret
g:
        movl    4(%esp), %eax
        movl    $0, (%eax)
        movl    (%eax), %edx    <<= extra read from *p
        movl    8(%esp), %eax
        movl    %edx, (%eax)
        ret

This behavior was discussed at length on the GCC mailing list starting at:
http://gcc.gnu.org/ml/gcc-patches/2010-06/msg02001.html
resulting in the behavior as now described at:
http://gcc.gnu.org/onlinedocs/gcc/Volatiles.html#Volatiles

"Assignments are also expressions and have an rvalue. However when assigning
to a scalar volatile, the volatile object is not reread, regardless of whether
the assignment expression's rvalue is used or not. If the assignment's rvalue
is used, the value is that assigned to the volatile object. [...]  If you need
to read the volatile object after an assignment has occurred, you must use a
separate expression with an intervening sequence point."

The current behavior is consistent with what C++ requires, and what most
other C compilers implement as well.

To reduce the potential for confusion, it seems to me GDB ought to mirror
that behavior as well ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] fix pre-/post- in-/decrement

[Daniel Jacobowitz]

On Tue, Oct 05, 2010 at 03:28:19PM +0200, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> > On Tue, Oct 05, 2010 at 01:24:51AM +0200, Ulrich Weigand wrote:
> > > - If you execute "set *$p = *$q = 0" and the write to *$q fails,
> > >   do you really expect *$p to be set to the old value of *$q
> > >   instead of to 0?
> > 
> > Yes, I would expect that.  To me, this is roughly "the debugger treats
> > all pointers as volatile".
> 
> The thing is, I had interpreted the C standard to read that even if
> pointers p and q *are* volatile, a statement like "*p = *q = 0" would
> still just trigger two writes, and no reads.

You're right, I remember Nathan's recent arguments about this on
gcc-patches.

> "Assignments are also expressions and have an rvalue. However when assigning
> to a scalar volatile, the volatile object is not reread, regardless of whether
> the assignment expression's rvalue is used or not. If the assignment's rvalue
> is used, the value is that assigned to the volatile object. [...]  If you need
> to read the volatile object after an assignment has occurred, you must use a
> separate expression with an intervening sequence point."

> To reduce the potential for confusion, it seems to me GDB ought to mirror
> that behavior as well ...

Hmm.

It seems to me that this is a disruptive change for us, because the "a
= b" case is more likely to be used than anything fancy (a += b, a = b
= c).  If we change it, we should document the change.

Vladimir, would this interact with your recent varobj changes?  We
worked out exactly which reads we wanted, but I don't remember if
value_assign is involved.

-- 
Daniel Jacobowitz
CodeSourcery

[rfc] Fix value_assign return value (Re: [patch] fix pre-/post- in-/decrement)

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Tue, Oct 05, 2010 at 03:28:19PM +0200, Ulrich Weigand wrote:
> > "Assignments are also expressions and have an rvalue. However when assigning
> > to a scalar volatile, the volatile object is not reread, regardless of whether
> > the assignment expression's rvalue is used or not. If the assignment's rvalue
> > is used, the value is that assigned to the volatile object. [...]  If you need
> > to read the volatile object after an assignment has occurred, you must use a
> > separate expression with an intervening sequence point."
> 
> > To reduce the potential for confusion, it seems to me GDB ought to mirror
> > that behavior as well ...
> 
> Hmm.
> 
> It seems to me that this is a disruptive change for us, because the "a
> = b" case is more likely to be used than anything fancy (a += b, a = b
> = c).  If we change it, we should document the change.

Sure.  The patch below implements fixes to the various problems w.r.t.
the return value of value_assign that I noticed:

- Make sure the returned value is non-lazy (this is what we discussed above).
  This includes a proposed NEWS entry -- feel free to suggest improved
  wordings.  I don't think we need to update the actual documentation,
  which already reads (in 15.4.1.1):  "The value of an assignment expression
  is the value assigned."

- In the case of assignments to a value of C++ class type, the code sets
  the enclosing type / embedded offset of the result value to reflect
  FROMVAL.  This is simply wrong; assignment to an object does not change
  its dynamic type.  The patch removes this, but keeps changing the
  enclosing type / pointed-to offset in the case of C++ object *pointers*.

- In the vase of assignment to internal variables, the existing code was
  simply incorrect (it returned a copy of "from", which means subsequent
  assignments no longer change the internal variable).  The patch fixes
  this by simply creating a new value for the updated internalvar using
  value_of_internalvar.

Tested on i386-linux with no regressions.

Any comments are welcome ...

Bye,
Ulrich

ChangeLog:

	* valops.c (value_assign): Returned value is never lazy.  If a
	C++ class type is returned, fix incorrect enclosing type / embedded
	offset.  If internal variable is returned, allocate new internalvar
	value using value_of_internalvar.

	* NEWS: Document changes in behavior of "print x = 0" and similar
	expressions.


Index: gdb/NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.405
diff -u -p -r1.405 NEWS
--- gdb/NEWS	13 Sep 2010 20:37:34 -0000	1.405
+++ gdb/NEWS	6 Oct 2010 18:41:26 -0000
@@ -23,6 +23,12 @@
      feature requires proper debuginfo support from the compiler; it
      was added to GCC 4.5.
 
+* GDB now follows GCC's rules on accessing volatile objects when
+  reading or writing target state during expression evaluation.
+  One notable difference to prior behavior is that "print x = 0"
+  no longer generates a read of x; the value of the assignment is
+  now always taken directly from the value being assigned.
+
 * GDB now has some support for using labels in the program's source in
   linespecs.  For instance, you can use "advance label" to continue
   execution to a label.
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.251
diff -u -p -r1.251 valops.c
--- gdb/valops.c	24 Sep 2010 14:47:53 -0000	1.251
+++ gdb/valops.c	6 Oct 2010 18:41:26 -0000
@@ -1099,13 +1099,8 @@ value_assign (struct value *toval, struc
     {
     case lval_internalvar:
       set_internalvar (VALUE_INTERNALVAR (toval), fromval);
-      val = value_copy (fromval);
-      val = value_change_enclosing_type (val, 
-					 value_enclosing_type (fromval));
-      set_value_embedded_offset (val, value_embedded_offset (fromval));
-      set_value_pointed_to_offset (val, 
-				   value_pointed_to_offset (fromval));
-      return val;
+      return value_of_internalvar (get_type_arch (type),
+				   VALUE_INTERNALVAR (toval));
 
     case lval_internalvar_component:
       set_internalvar_component (VALUE_INTERNALVAR (toval),
@@ -1291,14 +1286,23 @@ value_assign (struct value *toval, struc
       fromval = value_from_longest (type, fieldval);
     }
 
+  /* The return value is a copy of TOVAL so it shares its location
+     information, but its contents are updated from FROMVAL.  This
+     implies the returned value is not lazy, even if TOVAL was.  */
   val = value_copy (toval);
+  set_value_lazy (val, 0);
   memcpy (value_contents_raw (val), value_contents (fromval),
 	  TYPE_LENGTH (type));
-  deprecated_set_value_type (val, type);
-  val = value_change_enclosing_type (val, 
-				     value_enclosing_type (fromval));
-  set_value_embedded_offset (val, value_embedded_offset (fromval));
-  set_value_pointed_to_offset (val, value_pointed_to_offset (fromval));
+
+  /* We copy over the enclosing type and pointed-to offset from FROMVAL
+     in the case of pointer types.  For object types, the enclosing type
+     and embedded offset must *not* be copied: the target object refered
+     to by TOVAL retains its original dynamic type after assignment.  */
+  if (TYPE_CODE (type) == TYPE_CODE_PTR)
+    {
+      val = value_change_enclosing_type (val, value_enclosing_type (fromval));
+      set_value_pointed_to_offset (val, value_pointed_to_offset (fromval));
+    }
 
   return val;
 }

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [patch] fix pre-/post- in-/decrement

[Vladimir Prus]

On Tuesday, October 05, 2010 17:42:01 you wrote:
> On Tue, Oct 05, 2010 at 03:28:19PM +0200, Ulrich Weigand wrote:
> > Daniel Jacobowitz wrote:
> > > On Tue, Oct 05, 2010 at 01:24:51AM +0200, Ulrich Weigand wrote:
> > > > - If you execute "set *$p = *$q = 0" and the write to *$q fails,
> > > > 
> > > >   do you really expect *$p to be set to the old value of *$q
> > > >   instead of to 0?
> > > 
> > > Yes, I would expect that.  To me, this is roughly "the debugger treats
> > > all pointers as volatile".
> > 
> > The thing is, I had interpreted the C standard to read that even if
> > pointers p and q *are* volatile, a statement like "*p = *q = 0" would
> > still just trigger two writes, and no reads.
> 
> You're right, I remember Nathan's recent arguments about this on
> gcc-patches.
> 
> > "Assignments are also expressions and have an rvalue. However when
> > assigning to a scalar volatile, the volatile object is not reread,
> > regardless of whether the assignment expression's rvalue is used or not.
> > If the assignment's rvalue is used, the value is that assigned to the
> > volatile object. [...]  If you need to read the volatile object after an
> > assignment has occurred, you must use a separate expression with an
> > intervening sequence point."
> > 
> > To reduce the potential for confusion, it seems to me GDB ought to mirror
> > that behavior as well ...
> 
> Hmm.
> 
> It seems to me that this is a disruptive change for us, because the "a
> = b" case is more likely to be used than anything fancy (a += b, a = b
> = c).  If we change it, we should document the change.
> 
> Vladimir, would this interact with your recent varobj changes?  We
> worked out exactly which reads we wanted, but I don't remember if
> value_assign is involved.

I don't think value_assign was involved in my changes -- instead, I have
modified varobj_set_value to do a read-back of the value being assigned
to. 

I am not entirely sure whether on CLI, one wants:

	print $io.whatever = 10

to print '10' or the value in $io.whatever. Maybe, following C rules has
some sense here. On the other hand, in GUI there's no assignment
expression, so C standard is probably not a useful analogue.
So, I guess this patch is compatible with our changes.

- Volodya

Re: [patch] GNU vector unop support

[Joel Brobecker]

> > One question: Is it possible to have a non-array vector type?
> > In other words, can we just check for TYPE_VECTOR (type) instead
> > of TYPE_CODE (type) and TYPE_VECTOR (type)?
> 
> No, not that I'm aware of. Even a GNU Vector with a single element
> only is an array underneath. My understanding is that querying the
> flag_vector is only legal if the type is an array.

I feel that this makes the check for the TYPE_CODE a little superfluous.
But at the same time, the code handling vectors then assumes that the
underlying type is an array.

> ChangeLog:
> 
> 2010-10-04  Ken Werner  <ken.werner@de.ibm.com>
> 
> 	* valarith.c (value_pos, value_neg, value_complement): Handle
> 	vector types.
> 	* valops.c (value_one): Likewise.
> 
> testsuite/ChangeLog:
> 
> 2010-10-04  Ken Werner  <ken.werner@de.ibm.com>
> 
> 	* gdb.base/gnu_vector.exp: Add unary operator tests.

This is OK.

If it was just me, I'd probably wrap the checks for vector types
inside a function (such as type_is_vector or vector_type_p), with
a comment explaining the reason why we check for the TYPE_CODE
despite the fact that, normally, all types with the `vector' flag
should be arrays.  (this is just a soft suggestion, and to be treated
as a followup patch, if not ignored)

-- 
Joel

Re: [patch] fix pre-/post- in-/decrement

[Ken Werner]

[-- Attachment #1: Type: Text/Plain, Size: 843 bytes --]

On Monday, October 04, 2010 9:46:50 pm Ken Werner wrote:
> The attached patch introduces a new function called value_non_lval that
> returns a non-lval version of the given value. This function is called
> prior the simple assignment, compound assignment and pre/postfix routines
> return. Any comments are appreciated.

I overlooked that in case of C++ the various assignment, pre-increment and  
pre-decrement operators return lvalues while they return non-lvalues for C.
I've updated the patch to respect this. 
The patch also depends on non-lazy return values:
http://sourceware.org/ml/gdb-patches/2010-10/msg00082.html since calls of 
value_non_lval may trigger unnecessary reads from the target as mentioned 
here:
http://sourceware.org/ml/gdb-patches/2010-10/msg00030.html .
Tested on i686-*-linux-gnu with no regressions.

Regards
Ken

[-- Attachment #2: pre_post_in_decrement.patch --]
[-- Type: text/x-patch, Size: 5860 bytes --]

ChangeLog:

2010-10-07  Ken Werner  <ken.werner@de.ibm.com>

	* value.h (value_non_lval): Declare.
	* value.c (value_non_lval): New function.
	* eval.c (evaluate_subexp_standard) <BINOP_ASSIGN, BINOP_ASSIGN_MODIFY,
	UNOP_PREINCREMENT, UNOP_PREDECREMENT>: Call value_non_lval to ensure to
	return a non-lvalue if the source language is not C++.
	UNOP_POSTINCREMENT, UNOP_POSTDECREMENT>: Call value_non_lval to ensure
	to return a non-lvalue.

testsuite/ChangeLog:

2010-10-07  Ken Werner  <ken.werner@de.ibm.com>

	* gdb.base/exprs.exp: Add tests for pre-/post- in-/decrement operators.


Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.139
diff -p -u -r1.139 eval.c
--- gdb/eval.c	11 Aug 2010 16:48:26 -0000	1.139
+++ gdb/eval.c	7 Oct 2010 09:22:40 -0000
@@ -2009,8 +2009,10 @@ evaluate_subexp_standard (struct type *e
 	return arg1;
       if (binop_user_defined_p (op, arg1, arg2))
 	return value_x_binop (arg1, arg2, op, OP_NULL, noside);
-      else
+      else if (exp->language_defn->la_language == language_cplus)
 	return value_assign (arg1, arg2);
+      else
+	return value_non_lval (value_assign (arg1, arg2));
 
     case BINOP_ASSIGN_MODIFY:
       (*pos) += 2;
@@ -2043,7 +2045,10 @@ evaluate_subexp_standard (struct type *e
 
 	  arg2 = value_binop (tmp, arg2, op);
 	}
-      return value_assign (arg1, arg2);
+      if (exp->language_defn->la_language == language_cplus)
+	return value_assign (arg1, arg2);
+      else
+	return value_non_lval (value_assign (arg1, arg2));
 
     case BINOP_ADD:
       arg1 = evaluate_subexp_with_coercion (exp, pos, noside);
@@ -2702,7 +2707,10 @@ evaluate_subexp_standard (struct type *e
 	      arg2 = value_binop (tmp, arg2, BINOP_ADD);
 	    }
 
-	  return value_assign (arg1, arg2);
+	  if (exp->language_defn->la_language == language_cplus)
+	    return value_assign (arg1, arg2);
+	  else
+	    return value_non_lval (value_assign (arg1, arg2));
 	}
 
     case UNOP_PREDECREMENT:
@@ -2726,7 +2734,10 @@ evaluate_subexp_standard (struct type *e
 	      arg2 = value_binop (tmp, arg2, BINOP_SUB);
 	    }
 
-	  return value_assign (arg1, arg2);
+	  if (exp->language_defn->la_language == language_cplus)
+	    return value_assign (arg1, arg2);
+	  else
+	    return value_non_lval (value_assign (arg1, arg2));
 	}
 
     case UNOP_POSTINCREMENT:
@@ -2739,6 +2750,8 @@ evaluate_subexp_standard (struct type *e
 	}
       else
 	{
+	  arg3 = value_non_lval (arg1);
+	  
 	  if (ptrmath_type_p (exp->language_defn, value_type (arg1)))
 	    arg2 = value_ptradd (arg1, 1);
 	  else
@@ -2751,7 +2764,7 @@ evaluate_subexp_standard (struct type *e
 	    }
 
 	  value_assign (arg1, arg2);
-	  return arg1;
+	  return arg3;
 	}
 
     case UNOP_POSTDECREMENT:
@@ -2764,6 +2777,8 @@ evaluate_subexp_standard (struct type *e
 	}
       else
 	{
+	  arg3 = value_non_lval (arg1);
+	  
 	  if (ptrmath_type_p (exp->language_defn, value_type (arg1)))
 	    arg2 = value_ptradd (arg1, -1);
 	  else
@@ -2776,7 +2791,7 @@ evaluate_subexp_standard (struct type *e
 	    }
 
 	  value_assign (arg1, arg2);
-	  return arg1;
+	  return arg3;
 	}
 
     case OP_THIS:
Index: gdb/value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.113
diff -p -u -r1.113 value.c
--- gdb/value.c	30 Sep 2010 18:58:07 -0000	1.113
+++ gdb/value.c	7 Oct 2010 09:22:40 -0000
@@ -826,6 +826,26 @@ value_copy (struct value *arg)
   return val;
 }
 
+/* Return a version of ARG that is non-lvalue.  */
+
+struct value *
+value_non_lval (struct value *arg)
+{
+  if (VALUE_LVAL (arg) != not_lval)
+    {
+      struct type *enc_type = value_enclosing_type (arg);
+      struct value *val = allocate_value (enc_type);
+  
+      memcpy (value_contents_all_raw (val), value_contents_all (arg),
+	      TYPE_LENGTH (enc_type));
+      val->type = arg->type;
+      set_value_embedded_offset (val, value_embedded_offset (arg));
+      set_value_pointed_to_offset (val, value_pointed_to_offset (arg));
+      return val;
+    }
+   return arg;
+}
+
 void
 set_value_component_location (struct value *component,
 			      const struct value *whole)
Index: gdb/value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.161
diff -p -u -r1.161 value.h
--- gdb/value.h	7 Jul 2010 16:15:18 -0000	1.161
+++ gdb/value.h	7 Oct 2010 09:22:41 -0000
@@ -710,6 +710,8 @@ extern void preserve_values (struct objf
 
 extern struct value *value_copy (struct value *);
 
+extern struct value *value_non_lval (struct value *);
+
 extern void preserve_one_value (struct value *, struct objfile *, htab_t);
 
 /* From valops.c */
Index: gdb/testsuite/gdb.base/exprs.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/exprs.exp,v
retrieving revision 1.19
diff -p -u -r1.19 exprs.exp
--- gdb/testsuite/gdb.base/exprs.exp	10 Jun 2010 19:48:19 -0000	1.19
+++ gdb/testsuite/gdb.base/exprs.exp	7 Oct 2010 09:22:41 -0000
@@ -253,3 +253,14 @@ gdb_test "set output-radix 8" ".*"
 test_expr "print red" "\\$\[0-9\]* = red"
 test_expr "print/d red" "\\$\[0-9\]* = 0"
 gdb_test "set output-radix 10" ".*"
+
+# Pre-/post in-/decrement tests.
+gdb_test "set variable v_int = 1" ""
+gdb_test "print v_int++" "\\$\[0-9\]* = 1"
+gdb_test "print ++v_int" "\\$\[0-9\]* = 3"
+gdb_test "print v_int--" "\\$\[0-9\]* = 3"
+gdb_test "print --v_int" "\\$\[0-9\]* = 1"
+gdb_test "print v_int++ = 5" "Left operand of assignment is not an lvalue."
+gdb_test "print ++v_int = 5" "Left operand of assignment is not an lvalue."
+gdb_test "print v_int-- = 5" "Left operand of assignment is not an lvalue."
+gdb_test "print --v_int = 5" "Left operand of assignment is not an lvalue."

Re: [patch] GNU vector unop support

[Ken Werner]

On Thursday, October 07, 2010 1:26:59 am Joel Brobecker wrote:
> > > One question: Is it possible to have a non-array vector type?
> > > In other words, can we just check for TYPE_VECTOR (type) instead
> > > of TYPE_CODE (type) and TYPE_VECTOR (type)?
> > 
> > No, not that I'm aware of. Even a GNU Vector with a single element
> > only is an array underneath. My understanding is that querying the
> > flag_vector is only legal if the type is an array.
> 
> I feel that this makes the check for the TYPE_CODE a little superfluous.
> But at the same time, the code handling vectors then assumes that the
> underlying type is an array.

Ok, I see your point. : ) It seems my statement was incorrect. I missed that 
various backends (e.g. i386-tdep.c:i386_mmx_type or rs6000-
tdep.c:rs6000_builtin_type_vec64) create vectors with TYPE_CODE_UNION for 
their vector registers. Therefore checking the vector flag only may not always 
work as expected.

> > ChangeLog:
> > 
> > 2010-10-04  Ken Werner  <ken.werner@de.ibm.com>
> > 
> > 	* valarith.c (value_pos, value_neg, value_complement): Handle
> > 	vector types.
> > 	* valops.c (value_one): Likewise.
> > 
> > testsuite/ChangeLog:
> > 
> > 2010-10-04  Ken Werner  <ken.werner@de.ibm.com>
> > 
> > 	* gdb.base/gnu_vector.exp: Add unary operator tests.
> 
> This is OK.

Thanks. I'd prefer to wait with the commit until the lval-issue gets resolved.

> If it was just me, I'd probably wrap the checks for vector types
> inside a function (such as type_is_vector or vector_type_p), with
> a comment explaining the reason why we check for the TYPE_CODE
> despite the fact that, normally, all types with the `vector' flag
> should be arrays.  (this is just a soft suggestion, and to be treated
> as a followup patch, if not ignored)

It would clearly improve the readability of the source code. Giving vectors a 
distinct type code may also be an option.

Regards
Ken

Re: [patch] fix pre-/post- in-/decrement

[Tom Tromey]

>>>>> "Ken" == Ken Werner <ken@linux.vnet.ibm.com> writes:

Ken> I overlooked that in case of C++ the various assignment,
Ken> pre-increment and pre-decrement operators return lvalues while they
Ken> return non-lvalues for C.

I still had this patch in my queue, but I wasn't sure if it was
obsoleted by something else...

While I don't really mind having language checks in the expression
evaluator, it seems like this particular error condition is something
that could be detected in the C parser.

Also, I think the C++ rule is more complicated.  I did not look through
the standard to find it, but g++ at least gives an error for a simple
scalar "x++ = 5".

Tom

Re: [patch] fix pre-/post- in-/decrement

[Andreas Schwab]

Tom Tromey <tromey@redhat.com> writes:

> Also, I think the C++ rule is more complicated.  I did not look through
> the standard to find it, but g++ at least gives an error for a simple
> scalar "x++ = 5".

Postfix ++ does not return an lvalue.  Try prefix ++.

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: [patch] fix pre-/post- in-/decrement

[Ken Werner]

On Wednesday, October 13, 2010 1:00:04 am Tom Tromey wrote:
> >>>>> "Ken" == Ken Werner <ken@linux.vnet.ibm.com> writes:
> Ken> I overlooked that in case of C++ the various assignment,
> Ken> pre-increment and pre-decrement operators return lvalues while they
> Ken> return non-lvalues for C.
> 
> I still had this patch in my queue, but I wasn't sure if it was
> obsoleted by something else...

First, thank you for looking at this. In my opinion the patch is still 
relevant. The goal is to add unop support for vector types. The initial patch 
contained (beside its main purpose) code to prevent post in-/decrement 
operators from returning lvalues. Joel suggested this should be treated as a 
separate patch. This is how this patch was born. It helps GDB to be more 
accurate when it comes to the result types of unary operations.

> While I don't really mind having language checks in the expression
> evaluator, it seems like this particular error condition is something
> that could be detected in the C parser.

Hmm interesting. I guess you are referring to c-exp.y as this generates the 
expression parser for both languages C and C++. I don't see how to control the 
type of the result of an operator there. I'm still quite new to the GDB 
parsing internals and would appreciate any insights.

> Also, I think the C++ rule is more complicated.  I did not look through
> the standard to find it, but g++ at least gives an error for a simple
> scalar "x++ = 5".

Yes, this behaviour seems correct to me and the patch ensures that a non-
lvalue is returned in case of UNOP_POSTINCREMENT and UNOP_POSTDECREMENT as C 
and C++ do not differ here. For BINOP_ASSIGN, BINOP_ASSIGN_MODIFY, 
UNOP_PREINCREMENT and UNOP_PREDECREMENT C returns non-lvalues while C++ 
returns lvalues. This is why the patch checks the language and calls 
value_non_lval for languages other than C++. I think the relevant sections of 
the C++ standard are these:

5.3.2 Increment and decrement [expr.pre.incr]:
"The result is the updated operand; it is an lvalue, [..]"

5.2.6 Increment and decrement [expr.post.incr]
"The result is an rvalue."

5.17 Assignment and compound assignment operators [expr.ass]
"[...] return an lvalue with the type and value of the left operand after the 
assignment has taken
place."

So, this patch doesn't conflict with the g++ behaviour you are seeing.

Regards
Ken Werner

Re: [patch] fix pre-/post- in-/decrement

[Daniel Jacobowitz]

On Wed, Oct 13, 2010 at 11:23:39AM +0200, Ken Werner wrote:
> On Wednesday, October 13, 2010 1:00:04 am Tom Tromey wrote:
> > While I don't really mind having language checks in the expression
> > evaluator, it seems like this particular error condition is something
> > that could be detected in the C parser.
> 
> Hmm interesting. I guess you are referring to c-exp.y as this generates the 
> expression parser for both languages C and C++. I don't see how to control the 
> type of the result of an operator there. I'm still quite new to the GDB 
> parsing internals and would appreciate any insights.

You could add an expression operator to remove lval-ness, for
instance.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix pre-/post- in-/decrement

[Tom Tromey]

>>>>> "Ken" == Ken Werner <ken@linux.vnet.ibm.com> writes:

Tom> While I don't really mind having language checks in the expression
Tom> evaluator, it seems like this particular error condition is something
Tom> that could be detected in the C parser.

Ken> Hmm interesting. I guess you are referring to c-exp.y as this
Ken> generates the expression parser for both languages C and C++.

Yeah.

Ken> I don't see how to control the type of the result of an operator
Ken> there. I'm still quite new to the GDB parsing internals and would
Ken> appreciate any insights.

It is probably a bit of a pain, since the IR generated by the parser is
a bit unusual (as compilers go).

However, it seems to me that it would be much friendlier for users to
report this as a parse error rather than a runtime error.

One option would be to write a C/C++ implementation of the language_defn
la_post_parser method, which would look at the expression to see if this
constraint is violated.

Another option would be to try to implement it in the grammar.

Tom> Also, I think the C++ rule is more complicated.  I did not look through
Tom> the standard to find it, but g++ at least gives an error for a simple
Tom> scalar "x++ = 5".

Ken> Yes, this behaviour seems correct to me and the patch ensures that
Ken> a non- lvalue is returned in case of UNOP_POSTINCREMENT and
Ken> UNOP_POSTDECREMENT as C and C++ do not differ here.

Thanks for the correction and the links to the standard.

Tom

Re: [patch] fix pre-/post- in-/decrement

[Ken Werner]

[-- Attachment #1: Type: Text/Plain, Size: 1227 bytes --]

On Wednesday, October 13, 2010 9:00:28 pm Tom Tromey wrote:
> Ken> I don't see how to control the type of the result of an operator
> Ken> there. I'm still quite new to the GDB parsing internals and would
> Ken> appreciate any insights.
> 
> It is probably a bit of a pain, since the IR generated by the parser is
> a bit unusual (as compilers go).
> 
> However, it seems to me that it would be much friendlier for users to
> report this as a parse error rather than a runtime error.
> 
> One option would be to write a C/C++ implementation of the language_defn
> la_post_parser method, which would look at the expression to see if this
> constraint is violated.
> 
> Another option would be to try to implement it in the grammar.

Ok - I see. Thanks for your suggestions. This seems to be something bigger and 
could be implemented as part of a future patch :). I think for now the non-
language dependent parts of the patch would be sufficient.
The attached patch only contains the fix for the post in-/decrement operators 
as this is what the vec_unop patch (http://sourceware.org/ml/gdb-
patches/2010-10/msg00031.html) prevents from going upstream.
Tested on i686-*-linux-gnu with no regressions.
OK to apply?

Regards
Ken

[-- Attachment #2: post_in_decrement.patch --]
[-- Type: text/x-patch, Size: 3990 bytes --]

ChangeLog:

2010-10-18  Ken Werner  <ken.werner@de.ibm.com>

	* value.h (value_non_lval): Declare.
	* value.c (value_non_lval): New function.
	* eval.c (evaluate_subexp_standard) <UNOP_POSTINCREMENT,
	UNOP_POSTDECREMENT>: Call value_non_lval to ensure to return a
	non-lvalue.

testsuite/ChangeLog:

2010-10-18  Ken Werner  <ken.werner@de.ibm.com>

	* gdb.base/exprs.exp: Add tests for pre-/post- in-/decrement operators.

Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.139
diff -p -u -r1.139 eval.c
--- gdb/eval.c	11 Aug 2010 16:48:26 -0000	1.139
+++ gdb/eval.c	18 Oct 2010 17:43:52 -0000
@@ -2739,6 +2739,8 @@ evaluate_subexp_standard (struct type *e
 	}
       else
 	{
+	  arg3 = value_non_lval (arg1);
+
 	  if (ptrmath_type_p (exp->language_defn, value_type (arg1)))
 	    arg2 = value_ptradd (arg1, 1);
 	  else
@@ -2751,7 +2753,7 @@ evaluate_subexp_standard (struct type *e
 	    }
 
 	  value_assign (arg1, arg2);
-	  return arg1;
+	  return arg3;
 	}
 
     case UNOP_POSTDECREMENT:
@@ -2764,6 +2766,8 @@ evaluate_subexp_standard (struct type *e
 	}
       else
 	{
+	  arg3 = value_non_lval (arg1);
+
 	  if (ptrmath_type_p (exp->language_defn, value_type (arg1)))
 	    arg2 = value_ptradd (arg1, -1);
 	  else
@@ -2776,7 +2780,7 @@ evaluate_subexp_standard (struct type *e
 	    }
 
 	  value_assign (arg1, arg2);
-	  return arg1;
+	  return arg3;
 	}
 
     case OP_THIS:
Index: gdb/value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.113
diff -p -u -r1.113 value.c
--- gdb/value.c	30 Sep 2010 18:58:07 -0000	1.113
+++ gdb/value.c	18 Oct 2010 17:43:52 -0000
@@ -826,6 +826,26 @@ value_copy (struct value *arg)
   return val;
 }
 
+/* Return a version of ARG that is non-lvalue.  */
+
+struct value *
+value_non_lval (struct value *arg)
+{
+  if (VALUE_LVAL (arg) != not_lval)
+    {
+      struct type *enc_type = value_enclosing_type (arg);
+      struct value *val = allocate_value (enc_type);
+
+      memcpy (value_contents_all_raw (val), value_contents_all (arg),
+	      TYPE_LENGTH (enc_type));
+      val->type = arg->type;
+      set_value_embedded_offset (val, value_embedded_offset (arg));
+      set_value_pointed_to_offset (val, value_pointed_to_offset (arg));
+      return val;
+    }
+   return arg;
+}
+
 void
 set_value_component_location (struct value *component,
 			      const struct value *whole)
Index: gdb/value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.162
diff -p -u -r1.162 value.h
--- gdb/value.h	15 Oct 2010 18:54:13 -0000	1.162
+++ gdb/value.h	18 Oct 2010 17:43:52 -0000
@@ -711,6 +711,8 @@ extern void preserve_values (struct objf
 
 extern struct value *value_copy (struct value *);
 
+extern struct value *value_non_lval (struct value *);
+
 extern void preserve_one_value (struct value *, struct objfile *, htab_t);
 
 /* From valops.c */
Index: gdb/testsuite/gdb.base/exprs.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/exprs.exp,v
retrieving revision 1.19
diff -p -u -r1.19 exprs.exp
--- gdb/testsuite/gdb.base/exprs.exp	10 Jun 2010 19:48:19 -0000	1.19
+++ gdb/testsuite/gdb.base/exprs.exp	18 Oct 2010 17:43:52 -0000
@@ -253,3 +253,12 @@ gdb_test "set output-radix 8" ".*"
 test_expr "print red" "\\$\[0-9\]* = red"
 test_expr "print/d red" "\\$\[0-9\]* = 0"
 gdb_test "set output-radix 10" ".*"
+
+# Pre-/post in-/decrement tests.
+gdb_test "set variable v_int = 1" ""
+gdb_test "print v_int++" "\\$\[0-9\]* = 1"
+gdb_test "print ++v_int" "\\$\[0-9\]* = 3"
+gdb_test "print v_int--" "\\$\[0-9\]* = 3"
+gdb_test "print --v_int" "\\$\[0-9\]* = 1"
+gdb_test "print v_int++ = 5" "Left operand of assignment is not an lvalue."
+gdb_test "print v_int-- = 5" "Left operand of assignment is not an lvalue."

Re: [rfc] Fix value_assign return value (Re: [patch] fix pre-/post- in-/decrement)

[Daniel Jacobowitz]

On Wed, Oct 06, 2010 at 08:59:18PM +0200, Ulrich Weigand wrote:
> 	* valops.c (value_assign): Returned value is never lazy.  If a
> 	C++ class type is returned, fix incorrect enclosing type / embedded
> 	offset.  If internal variable is returned, allocate new internalvar
> 	value using value_of_internalvar.
> 
> 	* NEWS: Document changes in behavior of "print x = 0" and similar
> 	expressions.

Looks good to me.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] fix pre-/post- in-/decrement

[Ken Werner]

On Tuesday, October 19, 2010 9:38:34 am Ken Werner wrote:
> On Wednesday, October 13, 2010 9:00:28 pm Tom Tromey wrote:
> > Ken> I don't see how to control the type of the result of an operator
> > Ken> there. I'm still quite new to the GDB parsing internals and would
> > Ken> appreciate any insights.
> > 
> > It is probably a bit of a pain, since the IR generated by the parser is
> > a bit unusual (as compilers go).
> > 
> > However, it seems to me that it would be much friendlier for users to
> > report this as a parse error rather than a runtime error.
> > 
> > One option would be to write a C/C++ implementation of the language_defn
> > la_post_parser method, which would look at the expression to see if this
> > constraint is violated.
> > 
> > Another option would be to try to implement it in the grammar.
> 
> Ok - I see. Thanks for your suggestions. This seems to be something bigger
> and could be implemented as part of a future patch :). I think for now the
> non- language dependent parts of the patch would be sufficient.
> The attached patch only contains the fix for the post in-/decrement
> operators as this is what the vec_unop patch
> (http://sourceware.org/ml/gdb-
> patches/2010-10/msg00031.html) prevents from going upstream.
> Tested on i686-*-linux-gnu with no regressions.
> OK to apply?

Ping. : )

Regards
Ken

Re: [patch] fix pre-/post- in-/decrement

[Tom Tromey]

>>>>> "Ken" == Ken Werner <ken@linux.vnet.ibm.com> writes:

Ken> 2010-10-18  Ken Werner  <ken.werner@de.ibm.com>
Ken> 	* value.h (value_non_lval): Declare.
Ken> 	* value.c (value_non_lval): New function.
Ken> 	* eval.c (evaluate_subexp_standard) <UNOP_POSTINCREMENT,
UNOP_POSTDECREMENT> : Call value_non_lval to ensure to return a
Ken> 	non-lvalue.

This is ok.  Thanks.

Tom

Re: [patch] fix pre-/post- in-/decrement

[Ken Werner]

On Tuesday, November 02, 2010 6:09:02 pm Tom Tromey wrote:
> >>>>> "Ken" == Ken Werner <ken@linux.vnet.ibm.com> writes:
> Ken> 2010-10-18  Ken Werner  <ken.werner@de.ibm.com>
> Ken> 	* value.h (value_non_lval): Declare.
> Ken> 	* value.c (value_non_lval): New function.
> Ken> 	* eval.c (evaluate_subexp_standard) <UNOP_POSTINCREMENT,
> UNOP_POSTDECREMENT> : Call value_non_lval to ensure to return a
> Ken> 	non-lvalue.
> 
> This is ok.  Thanks.

Patch checked in:
http://sourceware.org/ml/gdb-cvs/2010-11/msg00012.html

Regards
Ken

Re: [patch] GNU vector unop support

[Ken Werner]

On Thursday, October 07, 2010 1:26:59 am Joel Brobecker wrote:
> > ChangeLog:
> > 
> > 2010-10-04  Ken Werner  <ken.werner@de.ibm.com>
> > 
> > 	* valarith.c (value_pos, value_neg, value_complement): Handle
> > 	vector types.
> > 	* valops.c (value_one): Likewise.
> > 
> > testsuite/ChangeLog:
> > 
> > 2010-10-04  Ken Werner  <ken.werner@de.ibm.com>
> > 
> > 	* gdb.base/gnu_vector.exp: Add unary operator tests.
> 
> This is OK.

I've checked the patch in now:
http://sourceware.org/ml/gdb-cvs/2010-11/msg00013.html

Thanks
Ken

Re: [rfc] Fix value_assign return value (Re: [patch] fix pre-/post- in-/decrement)

[Ulrich Weigand]

Dan Jacobowitz wrote:
> On Wed, Oct 06, 2010 at 08:59:18PM +0200, Ulrich Weigand wrote:
> > 	* valops.c (value_assign): Returned value is never lazy.  If a
> > 	C++ class type is returned, fix incorrect enclosing type / embedded
> > 	offset.  If internal variable is returned, allocate new internalvar
> > 	value using value_of_internalvar.
> > 
> > 	* NEWS: Document changes in behavior of "print x = 0" and similar
> > 	expressions.
> 
> Looks good to me.

OK, thanks for the review!  I've now updated the patch for recent changes
(see below), retested on i386-linux, and checked it in.

Bye,
Ulrich


ChangeLog:

	* valops.c (value_assign): Returned value is never lazy.  If a
	C++ class type is returned, fix incorrect enclosing type / embedded
	offset.  If internal variable is returned, allocate new internalvar
	value using value_of_internalvar.

	* NEWS: Document changes in behavior of "print x = 0" and similar
	expressions.


Index: gdb/NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.411
diff -u -p -r1.411 NEWS
--- gdb/NEWS	5 Nov 2010 16:55:37 -0000	1.411
+++ gdb/NEWS	25 Nov 2010 16:11:03 -0000
@@ -46,6 +46,12 @@
      feature requires proper debuginfo support from the compiler; it
      was added to GCC 4.5.
 
+* GDB now follows GCC's rules on accessing volatile objects when
+  reading or writing target state during expression evaluation.
+  One notable difference to prior behavior is that "print x = 0"
+  no longer generates a read of x; the value of the assignment is
+  now always taken directly from the value being assigned.
+
 * GDB now has some support for using labels in the program's source in
   linespecs.  For instance, you can use "advance label" to continue
   execution to a label.
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.257
diff -u -p -r1.257 valops.c
--- gdb/valops.c	10 Nov 2010 17:47:23 -0000	1.257
+++ gdb/valops.c	25 Nov 2010 16:11:04 -0000
@@ -1138,11 +1138,8 @@ value_assign (struct value *toval, struc
     {
     case lval_internalvar:
       set_internalvar (VALUE_INTERNALVAR (toval), fromval);
-      val = value_copy (fromval);
-      set_value_enclosing_type (val, value_enclosing_type (fromval));
-      set_value_embedded_offset (val, value_embedded_offset (fromval));
-      set_value_pointed_to_offset (val, value_pointed_to_offset (fromval));
-      return val;
+      return value_of_internalvar (get_type_arch (type),
+				   VALUE_INTERNALVAR (toval));
 
     case lval_internalvar_component:
       set_internalvar_component (VALUE_INTERNALVAR (toval),
@@ -1328,13 +1325,23 @@ value_assign (struct value *toval, struc
       fromval = value_from_longest (type, fieldval);
     }
 
+  /* The return value is a copy of TOVAL so it shares its location
+     information, but its contents are updated from FROMVAL.  This
+     implies the returned value is not lazy, even if TOVAL was.  */
   val = value_copy (toval);
+  set_value_lazy (val, 0);
   memcpy (value_contents_raw (val), value_contents (fromval),
 	  TYPE_LENGTH (type));
-  deprecated_set_value_type (val, type);
-  set_value_enclosing_type (val, value_enclosing_type (fromval));
-  set_value_embedded_offset (val, value_embedded_offset (fromval));
-  set_value_pointed_to_offset (val, value_pointed_to_offset (fromval));
+
+  /* We copy over the enclosing type and pointed-to offset from FROMVAL
+     in the case of pointer types.  For object types, the enclosing type
+     and embedded offset must *not* be copied: the target object refered
+     to by TOVAL retains its original dynamic type after assignment.  */
+  if (TYPE_CODE (type) == TYPE_CODE_PTR)
+    {
+      set_value_enclosing_type (val, value_enclosing_type (fromval));
+      set_value_pointed_to_offset (val, value_pointed_to_offset (fromval));
+    }
 
   return val;
 }

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com