[patch] Small fix for assigning values to vectors

[patch] Small fix for assigning values to vectors

[Ken Werner]

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

Hi,

I noticed that with the current GDB it is possible to assign values to 
not_lval-vector-values. Consider the following sample code:
  /* gcc -g -maltivec -mabi=altivec altivec.c -o altivec */
  #include <altivec.h>
  int test () {
    return 1;
  }
  vector int vtest () {
    return (vector int) {0, 1, 2, 3};
  }
  int main () {
    return 0;
  }

Here is the corresponding gdb session:
  gdb ./altivec
  start
  [...]
  (gdb) p test()
  $1 = 1
  (gdb) p &test()
  Attempt to take address of value not located in memory.
  (gdb) set variable test() = 2
  Left operand of assignment is not an lvalue.
  (gdb)  
  (gdb) 
  (gdb) p vtest()
  $2 = {0, 1, 2, 3}
  (gdb) p &vtest()
  $3 = (__vector signed int *) 0x10020008
  (gdb) set variable vtest() = {3, 2, 1, 0}
  (gdb) p vtest()
  $4 = {0, 1, 2, 3}

As can be seen the GDB behaves incorrect for vector types. A quick look to the 
valops.c:value_assign function shows that value_coerce_to_target creates a 
value with lval set to lval_memory for array types (including vectors). The 
code was introduced with the following patch: http://sourceware.org/ml/gdb-
patches/2008-03/msg00079.html. I have to admit that I do not entirely 
understand why value_coerce_to_target is called here. The attached patch adds 
a check for vector types within value_coerce_to_target and extends the 
altivec-abi testcase. Tested on powerpc64-*-linux-gnu, no regressions. Any 
suggestions are welcome.

Regards
-ken

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

Changelog:

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

	* valops.c (value_must_coerce_to_target): Return 0 in case of TYPE_VECTOR.

testsuite/ChangeLog:

2010-07-06  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 -u -p -r1.248 valops.c
--- gdb/valops.c	28 Jun 2010 20:35:52 -0000	1.248
+++ gdb/valops.c	6 Jul 2010 12:47:00 -0000
@@ -1424,6 +1424,9 @@ value_must_coerce_to_target (struct valu
 
   valtype = check_typedef (value_type (val));
 
+  if (TYPE_VECTOR (valtype))
+    return 0;
+
   switch (TYPE_CODE (valtype))
     {
     case TYPE_CODE_ARRAY:
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 -u -p -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	6 Jul 2010 12:47:01 -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 \

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

[Ulrich Weigand]

Ken Werner wrote:

> As can be seen the GDB behaves incorrect for vector types. A quick look to the 
> valops.c:value_assign function shows that value_coerce_to_target creates a 
> value with lval set to lval_memory for array types (including vectors). The 
> code was introduced with the following patch: http://sourceware.org/ml/gdb-
> patches/2008-03/msg00079.html. I have to admit that I do not entirely 
> understand why value_coerce_to_target is called here.

Dan, do you recall why you added a value_coerce_to_target for the *destination*
of an assignment?  It seems this shouldn't really be necessary ...

> @@ -1424,6 +1424,9 @@ value_must_coerce_to_target (struct valu
>  
>    valtype = check_typedef (value_type (val));
>  
> +  if (TYPE_VECTOR (valtype))
> +    return 0;
> +
>    switch (TYPE_CODE (valtype))
>      {
>      case TYPE_CODE_ARRAY:

Ken, this doesn't look quite right: the TYPE_VECTOR flag is defined only for
TYPE_CODE_ARRAY types; you should never look at TYPE_VECTOR for any other type.

Otherwise, this looks reasonable to me ...

Bye,
Ulrich

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

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

[Daniel Jacobowitz]

On Wed, Jul 07, 2010 at 07:50:14PM +0200, Ulrich Weigand wrote:
> Ken Werner wrote:
> 
> > As can be seen the GDB behaves incorrect for vector types. A quick look to the 
> > valops.c:value_assign function shows that value_coerce_to_target creates a 
> > value with lval set to lval_memory for array types (including vectors). The 
> > code was introduced with the following patch: http://sourceware.org/ml/gdb-
> > patches/2008-03/msg00079.html. I have to admit that I do not entirely 
> > understand why value_coerce_to_target is called here.
> 
> Dan, do you recall why you added a value_coerce_to_target for the *destination*
> of an assignment?  It seems this shouldn't really be necessary ...

I'm not sure now.  We could try dropping it.

> > @@ -1424,6 +1424,9 @@ value_must_coerce_to_target (struct valu
> >  
> >    valtype = check_typedef (value_type (val));
> >  
> > +  if (TYPE_VECTOR (valtype))
> > +    return 0;
> > +
> >    switch (TYPE_CODE (valtype))
> >      {
> >      case TYPE_CODE_ARRAY:
> 
> Ken, this doesn't look quite right: the TYPE_VECTOR flag is defined only for
> TYPE_CODE_ARRAY types; you should never look at TYPE_VECTOR for any other type.
> 
> Otherwise, this looks reasonable to me ...

It does seem reasonable that a vector does not have to live in target
memory.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ken Werner]

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

On Wednesday, July 07, 2010 08:26:10 pm Daniel Jacobowitz wrote:
> On Wed, Jul 07, 2010 at 07:50:14PM +0200, Ulrich Weigand wrote:
> > Ken Werner wrote:
> > > As can be seen the GDB behaves incorrect for vector types. A quick look
> > > to the valops.c:value_assign function shows that
> > > value_coerce_to_target creates a value with lval set to lval_memory
> > > for array types (including vectors). The code was introduced with the
> > > following patch: http://sourceware.org/ml/gdb-
> > > patches/2008-03/msg00079.html. I have to admit that I do not entirely
> > > understand why value_coerce_to_target is called here.
> > 
> > Dan, do you recall why you added a value_coerce_to_target for the
> > *destination* of an assignment?  It seems this shouldn't really be
> > necessary ...
> 
> I'm not sure now.  We could try dropping it.

Ok, the attached patch removes coerce_array call as well. Tested on powerpc64-
*-linux-gnu and i686-*-linux-gnu, no regressions.

> > > @@ -1424,6 +1424,9 @@ value_must_coerce_to_target (struct valu
> > > 
> > >    valtype = check_typedef (value_type (val));
> > > 
> > > +  if (TYPE_VECTOR (valtype))
> > > +    return 0;
> > > +
> > > 
> > >    switch (TYPE_CODE (valtype))
> > >    
> > >      {
> > 
> > >      case TYPE_CODE_ARRAY:
> > Ken, this doesn't look quite right: the TYPE_VECTOR flag is defined only
> > for TYPE_CODE_ARRAY types; you should never look at TYPE_VECTOR for any
> > other type.

Fixed, thanks.

> > Otherwise, this looks reasonable to me ...
> 
> It does seem reasonable that a vector does not have to live in target
> memory.

Regards
-ken

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

Changelog:

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

	* valops.c (value_assign): Remove coerce_array call.
	(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	9 Jul 2010 08:12:16 -0000
@@ -1083,13 +1083,6 @@ value_assign (struct value *toval, struc
       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);
-    }
 
   CHECK_TYPEDEF (type);
 
@@ -1427,6 +1420,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 -u -p -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	6 Jul 2010 12:47:01 -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 \

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

[Daniel Jacobowitz]

On Fri, Jul 09, 2010 at 12:38:57PM +0200, Ken Werner wrote:
> Ok, the attached patch removes coerce_array call as well. Tested on powerpc64-
> *-linux-gnu and i686-*-linux-gnu, no regressions.

I'm confused.

> @@ -1083,13 +1083,6 @@ value_assign (struct value *toval, struc
>        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);
> -    }
>  
>    CHECK_TYPEDEF (type);
>  

I thought we were discussing toval, at the top of the context block;
that's the destination.  It seems to me like we do need to coerce
fromval here.  If this works, maybe somewhere else is coercing it
anyway?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

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

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

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> I thought we were discussing toval, at the top of the context block;
> that's the destination.  It seems to me like we do need to coerce
> fromval here.  If this works, maybe somewhere else is coercing it
> anyway?

Oops, sorry, crossed emails.  If we don't coerce from fromval when
assigning to an internal variable, it simply means that the internal
variable acquires array type and the full array contents, instead of
acquiring pointer type.  But I don't see why this would be a problem;
in fact it may well preferable.  See my other mail for more details ...

Bye,
Ulrich

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

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

[Ken Werner]

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

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

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

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 \

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

[Daniel Jacobowitz]

On Sat, Jul 10, 2010 at 03:03:53PM +0200, Ken Werner wrote:
> But while we are at it. Are there any objections on copying the contents to 
> the destination instead of creating a pointer?

I think we're going a bit too far now, and maybe we need to figure out
what the semantics of internalvars are supposed to be...

With this, IIUC, "set $internalvar = program_array" is going to read
the whole array.  Previously, it would decay to a pointer as in C.


-- 
Daniel Jacobowitz
CodeSourcery

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

[Ken Werner]

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

On Saturday, July 10, 2010 05:39:09 pm Daniel Jacobowitz wrote:
> On Sat, Jul 10, 2010 at 03:03:53PM +0200, Ken Werner wrote:
> > But while we are at it. Are there any objections on copying the contents
> > to the destination instead of creating a pointer?
> 
> I think we're going a bit too far now, and maybe we need to figure out
> what the semantics of internalvars are supposed to be...
> 
> With this, IIUC, "set $internalvar = program_array" is going to read
> the whole array.  Previously, it would decay to a pointer as in C.

I agree. As I don't have a strong opinion either way on this particular case 
I'd leave that decision to more experienced GDB developers.

Here is the patch I intended to post where only the call to 
value_coerce_to_target is omitted and the value_must_coerce_to_target routine 
returns zero in case of a vector. Tested on powerpc64-*-linux-gnu with no 
regressions.

Regards.
-ken

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

Changelog:

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

	* valops.c (value_assign): Do not call to value_coerce_to_target.
	(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 10:16:36 -0000
@@ -1079,10 +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);
-    }
+    fromval = value_cast (type, fromval);
   else
     {
       /* Coerce arrays and functions to pointers, except for arrays
@@ -1427,6 +1424,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 -u -p -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	6 Jul 2010 12:47:01 -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 \

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

[Ulrich Weigand]

Ken Werner wrote:

> On Saturday, July 10, 2010 05:39:09 pm Daniel Jacobowitz wrote:
> > On Sat, Jul 10, 2010 at 03:03:53PM +0200, Ken Werner wrote:
> > > But while we are at it. Are there any objections on copying the contents
> > > to the destination instead of creating a pointer?
> > 
> > I think we're going a bit too far now, and maybe we need to figure out
> > what the semantics of internalvars are supposed to be...
> > 
> > With this, IIUC, "set $internalvar = program_array" is going to read
> > the whole array.  Previously, it would decay to a pointer as in C.
> 
> I agree. As I don't have a strong opinion either way on this particular case 
> I'd leave that decision to more experienced GDB developers.

Yes, this is a separate issue; let's leave the behaviour as is for now.
 
> Here is the patch I intended to post where only the call to 
> value_coerce_to_target is omitted and the value_must_coerce_to_target routine 
> returns zero in case of a vector. Tested on powerpc64-*-linux-gnu with no 
> regressions.

> Changelog:
> 
> 2010-07-09  Ken Werner  <ken.werner@de.ibm.com>
> 
> 	* valops.c (value_assign): Do not call to value_coerce_to_target.
> 	(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.

This is OK.

I think you should get Write After Approval access to the GDB CVS so that you
can commit your approved patches yourself.  Please apply using this form:
http://sourceware.org/cgi-bin/pdw/ps_form.cgi

Once you get access, please add yourself to the MAINTAINERS file under
the Write After Approval section, and then commit your patch.

Bye,
Ulrich

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

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

[Ken Werner]

On Monday, July 12, 2010 04:55:36 pm Ulrich Weigand wrote:
> > Here is the patch I intended to post where only the call to
> > value_coerce_to_target is omitted and the value_must_coerce_to_target
> > routine returns zero in case of a vector. Tested on
> > powerpc64-*-linux-gnu with no regressions.
> > 
> > Changelog:
> > 
> > 2010-07-09  Ken Werner  <ken.werner@de.ibm.com>
> > 
> > 	* valops.c (value_assign): Do not call to value_coerce_to_target.
> > 	(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.
> 
> This is OK.
> 
> I think you should get Write After Approval access to the GDB CVS so that
> you can commit your approved patches yourself.  Please apply using this
> form: http://sourceware.org/cgi-bin/pdw/ps_form.cgi
> 
> Once you get access, please add yourself to the MAINTAINERS file under
> the Write After Approval section, and then commit your patch.

Thank you. I've checked both patches in now.

Regards,
-ken