From: Ken Werner <ken@linux.vnet.ibm.com>
To: "Ulrich Weigand" <uweigand@de.ibm.com>,
Daniel Jacobowitz <dan@codesourcery.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [patch] Small fix for assigning values to vectors
Date: Sat, 10 Jul 2010 13:04:00 -0000 [thread overview]
Message-ID: <201007101503.54109.ken@linux.vnet.ibm.com> (raw)
In-Reply-To: <201007091312.o69DC1TX013390@d12av02.megacenter.de.ibm.com>
[-- 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 \
next prev parent reply other threads:[~2010-07-10 13:04 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-07-06 13:58 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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201007101503.54109.ken@linux.vnet.ibm.com \
--to=ken@linux.vnet.ibm.com \
--cc=dan@codesourcery.com \
--cc=gdb-patches@sourceware.org \
--cc=uweigand@de.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox