Re: [patch] Small fix for assigning values to vectors

[Ken Werner <ken at linux dot vnet dot ibm dot com>]

On Friday, July 09, 2010 03:12:01 pm Ulrich Weigand wrote:
> Ken Werner wrote:
> > Ok, the attached patch removes coerce_array call as well. Tested on
> > powerpc64- *-linux-gnu and i686-*-linux-gnu, no regressions.
> 
> Hmm, actually I was thinking of the value_coerce_to_target call on the
> *destination*, not the coerce_array on the source.  However, it is in fact
> interesting to see that the coerce_array call seems to have no effects on
> the testsuite either ...
> 
> The current code reads:
> 
>   type = value_type (toval);
>   if (VALUE_LVAL (toval) != lval_internalvar)
>     {
>       toval = value_coerce_to_target (toval);
>       fromval = value_cast (type, fromval);
>     }
>   else
>     {
>       /* Coerce arrays and functions to pointers, except for arrays
>          which only live in GDB's storage.  */
>       if (!value_must_coerce_to_target (fromval))
>         fromval = coerce_array (fromval);
>     }
> 
> which distinguishes between assignments to a GDB internal variable and
> some other assignment destination.
> 
> 1.) For usual destinations (except GDB internal variables), the destination
>     has a specified type, and we need to cast FROMVAL to that destination
>     type before assignment.
> 
> 2.) In addition, for usual destinations, if the destination currently
> resides in GDB memory only, it is forced to target memory.  This is the
> call to value_coerce_to_target in the "if" part.
> 
> 3.) Finally, if we assign to a GDB internal variable, the variable has no
>     type in and of itself, but assumes the type of whatever is assigned to
>     it; therefore we do not need to type-cast the source.  However, the
> code will (sometimes) convert an array to a pointer.
> 
> Now, I think (1) is clearly required.
> 
> As to (2), this is what my original mail was refering to.  If the
> destination is not an internal variable, but something that temporarily
> resides in GDB memory (e.g. as the result of GDB constructing a string
> literal), assigning to it would normally fail because the destination is
> not an lvalue.  However, due to that value_coerce_to_target call, the old
> contents of the destination will be copied to some random location in
> target memory, and immediately afterwards overwritten by the new contents.
>  (And then the pointer to that target memory will most likely be forgotten
> anyway.)   This seems not really useful to me, therefore I'd suggested to
> remove that call.
> 
> Now as to (3), which you just removed, this also seems questionable.  The
> effect of the coerce_array call is that if you assign an array to an
> internal variable, the full array contents are not copied into the GDB
> representation of the internal variable; instead, only a pointer is stored.
> However, this seems to conflict with another goal of internal variables:
> see e.g. this comment in set_internalvar:
>       /* Force the value to be fetched from the target now, to avoid
> problems later when this internalvar is referenced and the target is gone
> or has changed.  */
> The code in value_assign directly conflicts with the code in
> set_internalvar in that respect.  It seems to me that it does indeed make
> sense to fetch internal variable contents completely, to allow to preserve
> them once the target goes away.  (If the user want to specifically store a
> pointer in the internal variable for some reason, they're still free to
> explicitly use the & operator anyway.)
> 
> So in summary, it's good that you verified (3) can go away with no
> testsuite regressions.  Could you in addition check whether (2) can *also*
> go away?
> 
> Bye,
> Ulrich

Hello,
thanks for the analysis and sorry for the confusion. I accidentally removed 
the else body.

But while we are at it. Are there any objections on copying the contents to 
the destination instead of creating a pointer?

The attached patch removes both calls with no regressions on powerpc64-*-
linux-gnu and i686-*-linux-gnu.

Regards
-ken

Changelog:

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

	* valops.c (value_assign): Do not call tovalue_coerce_to_target and
	coerce_array.
	(value_must_coerce_to_target): Return 0 in case of TYPE_VECTOR.

testsuite/ChangeLog:

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

	* gdb.arch/altivec-abi.exp: New tests.


Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.248
diff -p -u -r1.248 valops.c
--- gdb/valops.c	28 Jun 2010 20:35:52 -0000	1.248
+++ gdb/valops.c	10 Jul 2010 11:36:42 -0000
@@ -1079,17 +1079,7 @@ value_assign (struct value *toval, struc
 
   type = value_type (toval);
   if (VALUE_LVAL (toval) != lval_internalvar)
-    {
-      toval = value_coerce_to_target (toval);
-      fromval = value_cast (type, fromval);
-    }
-  else
-    {
-      /* Coerce arrays and functions to pointers, except for arrays
-	 which only live in GDB's storage.  */
-      if (!value_must_coerce_to_target (fromval))
-	fromval = coerce_array (fromval);
-    }
+    fromval = value_cast (type, fromval);
 
   CHECK_TYPEDEF (type);
 
@@ -1427,6 +1417,7 @@ value_must_coerce_to_target (struct valu
   switch (TYPE_CODE (valtype))
     {
     case TYPE_CODE_ARRAY:
+      return TYPE_VECTOR (valtype) ? 0 : 1;
     case TYPE_CODE_STRING:
       return 1;
     default:
Index: gdb/testsuite/gdb.arch/altivec-abi.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.arch/altivec-abi.exp,v
retrieving revision 1.19
diff -p -u -r1.19 altivec-abi.exp
--- gdb/testsuite/gdb.arch/altivec-abi.exp	2 Jul 2010 18:02:19 -0000	1.19
+++ gdb/testsuite/gdb.arch/altivec-abi.exp	10 Jul 2010 11:36:42 -0000
@@ -98,6 +98,16 @@ proc altivec_abi_tests { extra_flags for
     gdb_test "p vec_func(vshort_d,vushort_d,vint_d,vuint_d,vchar_d,vuchar_d,vfloat_d,x_d,y_d,a_d,b_d,c_d,intv_on_stack_d)" \
 	".\[0-9\]+ = .0, 0, 0, 0." "call inferior function with vectors (2)"
 
+    # Attempt to take address of the return value of vec_func.
+    gdb_test "p &vec_func(vshort,vushort,vint,vuint,vchar,vuchar,vfloat,x,y,a,b,c,intv_on_stack)" \
+	"Attempt to take address of value not located in memory." \
+	"Attempt to take address of the return value of vec_func"
+
+    # Attempt to assing a value to the return value of vec_func.
+    gdb_test "set variable vec_func(vshort,vushort,vint,vuint,vchar,vuchar,vfloat,x,y,a,b,c,intv_on_stack) = {0,1,2,3}" \
+	"Left operand of assignment is not an lvalue." \
+	"Attempt to assing a value to the return value of vec_func"
+
     # Let's step into the function, to see if the args are printed correctly.
     gdb_test "step" \
 	$pattern1 \