[rfc] Fix value_assign return value (Re: [patch] fix pre-/post- in-/decrement)

["Ulrich Weigand" <uweigand@de.ibm.com>]

Daniel Jacobowitz wrote:
> On Tue, Oct 05, 2010 at 03:28:19PM +0200, Ulrich Weigand wrote:
> > "Assignments are also expressions and have an rvalue. However when assigning
> > to a scalar volatile, the volatile object is not reread, regardless of whether
> > the assignment expression's rvalue is used or not. If the assignment's rvalue
> > is used, the value is that assigned to the volatile object. [...]  If you need
> > to read the volatile object after an assignment has occurred, you must use a
> > separate expression with an intervening sequence point."
> 
> > To reduce the potential for confusion, it seems to me GDB ought to mirror
> > that behavior as well ...
> 
> Hmm.
> 
> It seems to me that this is a disruptive change for us, because the "a
> = b" case is more likely to be used than anything fancy (a += b, a = b
> = c).  If we change it, we should document the change.

Sure.  The patch below implements fixes to the various problems w.r.t.
the return value of value_assign that I noticed:

- Make sure the returned value is non-lazy (this is what we discussed above).
  This includes a proposed NEWS entry -- feel free to suggest improved
  wordings.  I don't think we need to update the actual documentation,
  which already reads (in 15.4.1.1):  "The value of an assignment expression
  is the value assigned."

- In the case of assignments to a value of C++ class type, the code sets
  the enclosing type / embedded offset of the result value to reflect
  FROMVAL.  This is simply wrong; assignment to an object does not change
  its dynamic type.  The patch removes this, but keeps changing the
  enclosing type / pointed-to offset in the case of C++ object *pointers*.

- In the vase of assignment to internal variables, the existing code was
  simply incorrect (it returned a copy of "from", which means subsequent
  assignments no longer change the internal variable).  The patch fixes
  this by simply creating a new value for the updated internalvar using
  value_of_internalvar.

Tested on i386-linux with no regressions.

Any comments are welcome ...

Bye,
Ulrich

ChangeLog:

	* valops.c (value_assign): Returned value is never lazy.  If a
	C++ class type is returned, fix incorrect enclosing type / embedded
	offset.  If internal variable is returned, allocate new internalvar
	value using value_of_internalvar.

	* NEWS: Document changes in behavior of "print x = 0" and similar
	expressions.


Index: gdb/NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.405
diff -u -p -r1.405 NEWS
--- gdb/NEWS	13 Sep 2010 20:37:34 -0000	1.405
+++ gdb/NEWS	6 Oct 2010 18:41:26 -0000
@@ -23,6 +23,12 @@
      feature requires proper debuginfo support from the compiler; it
      was added to GCC 4.5.
 
+* GDB now follows GCC's rules on accessing volatile objects when
+  reading or writing target state during expression evaluation.
+  One notable difference to prior behavior is that "print x = 0"
+  no longer generates a read of x; the value of the assignment is
+  now always taken directly from the value being assigned.
+
 * GDB now has some support for using labels in the program's source in
   linespecs.  For instance, you can use "advance label" to continue
   execution to a label.
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.251
diff -u -p -r1.251 valops.c
--- gdb/valops.c	24 Sep 2010 14:47:53 -0000	1.251
+++ gdb/valops.c	6 Oct 2010 18:41:26 -0000
@@ -1099,13 +1099,8 @@ value_assign (struct value *toval, struc
     {
     case lval_internalvar:
       set_internalvar (VALUE_INTERNALVAR (toval), fromval);
-      val = value_copy (fromval);
-      val = value_change_enclosing_type (val, 
-					 value_enclosing_type (fromval));
-      set_value_embedded_offset (val, value_embedded_offset (fromval));
-      set_value_pointed_to_offset (val, 
-				   value_pointed_to_offset (fromval));
-      return val;
+      return value_of_internalvar (get_type_arch (type),
+				   VALUE_INTERNALVAR (toval));
 
     case lval_internalvar_component:
       set_internalvar_component (VALUE_INTERNALVAR (toval),
@@ -1291,14 +1286,23 @@ value_assign (struct value *toval, struc
       fromval = value_from_longest (type, fieldval);
     }
 
+  /* The return value is a copy of TOVAL so it shares its location
+     information, but its contents are updated from FROMVAL.  This
+     implies the returned value is not lazy, even if TOVAL was.  */
   val = value_copy (toval);
+  set_value_lazy (val, 0);
   memcpy (value_contents_raw (val), value_contents (fromval),
 	  TYPE_LENGTH (type));
-  deprecated_set_value_type (val, type);
-  val = value_change_enclosing_type (val, 
-				     value_enclosing_type (fromval));
-  set_value_embedded_offset (val, value_embedded_offset (fromval));
-  set_value_pointed_to_offset (val, value_pointed_to_offset (fromval));
+
+  /* We copy over the enclosing type and pointed-to offset from FROMVAL
+     in the case of pointer types.  For object types, the enclosing type
+     and embedded offset must *not* be copied: the target object refered
+     to by TOVAL retains its original dynamic type after assignment.  */
+  if (TYPE_CODE (type) == TYPE_CODE_PTR)
+    {
+      val = value_change_enclosing_type (val, value_enclosing_type (fromval));
+      set_value_pointed_to_offset (val, value_pointed_to_offset (fromval));
+    }
 
   return val;
 }

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