From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29373 invoked by alias); 10 Jul 2010 13:04:11 -0000 Received: (qmail 29340 invoked by uid 22791); 10 Jul 2010 13:04:09 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mtagate4.de.ibm.com (HELO mtagate4.de.ibm.com) (195.212.17.164) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 10 Jul 2010 13:04:02 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate4.de.ibm.com (8.13.1/8.13.1) with ESMTP id o6AD3xWs029718 for ; Sat, 10 Jul 2010 13:03:59 GMT Received: from d12av04.megacenter.de.ibm.com (d12av04.megacenter.de.ibm.com [9.149.165.229]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o6AD3xHp1814634 for ; Sat, 10 Jul 2010 15:03:59 +0200 Received: from d12av04.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av04.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id o6AD3wSG015852 for ; Sat, 10 Jul 2010 15:03:59 +0200 Received: from leonard.localnet (ICON-9-164-180-158.megacenter.de.ibm.com [9.164.180.158]) by d12av04.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with ESMTP id o6AD3s1h015748 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 10 Jul 2010 15:03:58 +0200 From: Ken Werner To: "Ulrich Weigand" , Daniel Jacobowitz Subject: Re: [patch] Small fix for assigning values to vectors Date: Sat, 10 Jul 2010 13:04:00 -0000 User-Agent: KMail/1.13.2 (Linux/2.6.32-23-generic-pae; KDE/4.4.2; i686; ; ) References: <201007091312.o69DC1TX013390@d12av02.megacenter.de.ibm.com> In-Reply-To: <201007091312.o69DC1TX013390@d12av02.megacenter.de.ibm.com> Cc: gdb-patches@sourceware.org MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_6+GOMYooyfmWG5Y" Message-Id: <201007101503.54109.ken@linux.vnet.ibm.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-07/txt/msg00195.txt.bz2 --Boundary-00=_6+GOMYooyfmWG5Y Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-length: 3974 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 --Boundary-00=_6+GOMYooyfmWG5Y Content-Type: text/x-patch; charset="UTF-8"; name="vec_lval.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="vec_lval.patch" Content-length: 2720 Changelog: 2010-07-09 Ken Werner * 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 * 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 \ --Boundary-00=_6+GOMYooyfmWG5Y--