Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
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 \

  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