* [patch] Small fix for assigning values to vectors @ 2010-07-06 13:58 Ken Werner 2010-07-07 17:50 ` Ulrich Weigand 0 siblings, 1 reply; 12+ messages in thread From: Ken Werner @ 2010-07-06 13:58 UTC (permalink / raw) To: gdb-patches [-- 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 \ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-06 13:58 [patch] Small fix for assigning values to vectors Ken Werner @ 2010-07-07 17:50 ` Ulrich Weigand 2010-07-07 18:26 ` Daniel Jacobowitz 0 siblings, 1 reply; 12+ messages in thread From: Ulrich Weigand @ 2010-07-07 17:50 UTC (permalink / raw) To: Ken Werner, dan; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-07 17:50 ` Ulrich Weigand @ 2010-07-07 18:26 ` Daniel Jacobowitz 2010-07-09 10:39 ` Ken Werner 0 siblings, 1 reply; 12+ messages in thread From: Daniel Jacobowitz @ 2010-07-07 18:26 UTC (permalink / raw) To: Ulrich Weigand; +Cc: Ken Werner, gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-07 18:26 ` Daniel Jacobowitz @ 2010-07-09 10:39 ` Ken Werner 2010-07-09 13:03 ` Daniel Jacobowitz 2010-07-09 13:12 ` Ulrich Weigand 0 siblings, 2 replies; 12+ messages in thread From: Ken Werner @ 2010-07-09 10:39 UTC (permalink / raw) To: Daniel Jacobowitz, Ulrich Weigand; +Cc: gdb-patches [-- 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 \ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-09 10:39 ` Ken Werner @ 2010-07-09 13:03 ` Daniel Jacobowitz 2010-07-09 13:16 ` Ulrich Weigand 2010-07-09 13:12 ` Ulrich Weigand 1 sibling, 1 reply; 12+ messages in thread From: Daniel Jacobowitz @ 2010-07-09 13:03 UTC (permalink / raw) To: Ken Werner; +Cc: Ulrich Weigand, gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-09 13:03 ` Daniel Jacobowitz @ 2010-07-09 13:16 ` Ulrich Weigand 0 siblings, 0 replies; 12+ messages in thread From: Ulrich Weigand @ 2010-07-09 13:16 UTC (permalink / raw) To: Daniel Jacobowitz; +Cc: Ken Werner, gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-09 10:39 ` Ken Werner 2010-07-09 13:03 ` Daniel Jacobowitz @ 2010-07-09 13:12 ` Ulrich Weigand 2010-07-10 13:04 ` Ken Werner 1 sibling, 1 reply; 12+ messages in thread From: Ulrich Weigand @ 2010-07-09 13:12 UTC (permalink / raw) To: Ken Werner; +Cc: Daniel Jacobowitz, gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-09 13:12 ` Ulrich Weigand @ 2010-07-10 13:04 ` Ken Werner 2010-07-10 15:39 ` Daniel Jacobowitz 0 siblings, 1 reply; 12+ messages in thread From: Ken Werner @ 2010-07-10 13:04 UTC (permalink / raw) To: Ulrich Weigand, Daniel Jacobowitz; +Cc: gdb-patches [-- 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 \ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-10 13:04 ` Ken Werner @ 2010-07-10 15:39 ` Daniel Jacobowitz 2010-07-10 17:05 ` Ken Werner 0 siblings, 1 reply; 12+ messages in thread From: Daniel Jacobowitz @ 2010-07-10 15:39 UTC (permalink / raw) To: Ken Werner; +Cc: Ulrich Weigand, gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-10 15:39 ` Daniel Jacobowitz @ 2010-07-10 17:05 ` Ken Werner 2010-07-12 14:55 ` Ulrich Weigand 0 siblings, 1 reply; 12+ messages in thread From: Ken Werner @ 2010-07-10 17:05 UTC (permalink / raw) To: Daniel Jacobowitz; +Cc: Ulrich Weigand, gdb-patches [-- 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 \ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-10 17:05 ` Ken Werner @ 2010-07-12 14:55 ` Ulrich Weigand 2010-07-14 14:55 ` Ken Werner 0 siblings, 1 reply; 12+ messages in thread From: Ulrich Weigand @ 2010-07-12 14:55 UTC (permalink / raw) To: Ken Werner; +Cc: Daniel Jacobowitz, Ulrich Weigand, gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Small fix for assigning values to vectors 2010-07-12 14:55 ` Ulrich Weigand @ 2010-07-14 14:55 ` Ken Werner 0 siblings, 0 replies; 12+ messages in thread From: Ken Werner @ 2010-07-14 14:55 UTC (permalink / raw) To: Ulrich Weigand, gdb-patches; +Cc: Daniel Jacobowitz 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2010-07-14 14:55 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-07-06 13:58 [patch] Small fix for assigning values to vectors Ken Werner 2010-07-07 17:50 ` Ulrich Weigand 2010-07-07 18:26 ` Daniel Jacobowitz 2010-07-09 10:39 ` Ken Werner 2010-07-09 13:03 ` Daniel Jacobowitz 2010-07-09 13:16 ` Ulrich Weigand 2010-07-09 13:12 ` Ulrich Weigand 2010-07-10 13:04 ` Ken Werner 2010-07-10 15:39 ` Daniel Jacobowitz 2010-07-10 17:05 ` Ken Werner 2010-07-12 14:55 ` Ulrich Weigand 2010-07-14 14:55 ` Ken Werner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).