Re: [patch] fix pre-/post- in-/decrement

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Daniel Jacobowitz wrote:
> Try this - it's easiest to see what's going on with set debug target
> 1, or else by using gdbserver and set debug remote 1 (the latter is
> easier for me to read).  I've edited out some reads relating to the
> stack frame.
> 
> (gdb) set $p = (int *) $sp
> (gdb) p *$p
> Sending packet: $m7fffffffdff0,4#2e...Packet received: 01000000
> $4 = 1
> (gdb) set *$p = 2
> Sending packet: $X7fffffffdff0,4:\002\000\000\000#55...Packet received: OK
> (gdb) print *$p = 1
> Sending packet: $X7fffffffdff0,4:\001\000\000\000#54...Packet received: OK
> Sending packet: $m7fffffffdff0,4#2e...Packet received: 01000000
> $5 = 1
> (gdb)
> 
> I suspect that your patch will be called to handle the value_assign
> for the set command, and it will result in an unnecessary read from
> the target.  You want the resulting value to be unwritable, but it can
> still be lazy; you don't need the value.
> 
> Of course, those two things are not orthogonal in GDB's current
> representation.  So I don't know how to do this.  But don't break the
> current behavior, please - it's important both for performance and for
> embedded correctness.

It would appear that even the current behavior, as shown in your trace,
already contains an unnecessary load.  There should be no need to perform
a memory read to evaluate "print *$p = 1".

In fact, value_assign contains code that apparently tries to avoid just
that, but it seems this no longer works as expected with lazy values:

  val = value_copy (toval);
  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));


Note that if "toval" was lazy, "val" is likewise lazy -- but then, its
contents are filled in, and are subsequently completely ignored because
the value is lazy.

The rest of this sequence seems dubious as well.  The type of "toval"
(and hence "val") will already be "type" -- modulo typedefs, but I
don't see why you'd want an assignment operator to remove typedefs.

Changing the enclosing type and embedded offset seems just wrong: the
constructed "val" contents will have only "toval"'s enclosing contents,
not "fromval"'s.  (In addition, if the enclosing types are actually
different, the value_change_enclosing_type will just completely wipe
out "val"'s contents again ...)

The pointed_to_offset call looks OK.  (I'm not sure whether the whole
pointed_to_offset logic is needed at all today, but I guess that's a
different topic ...)


I think the way to fix this problem would be to add a call to
  set_value_lazy (val, 0);
at this point -- we have filled in the contents, and thus the value
indeed is no longer lazy if it ever was.

This would -even without Ken's patch fix- the unnecessary read shown above,
and would have the additional effect that Ken's patch will introduce no
further reads from the target, as all values returned from value_assign
will already be non-lazy.

Thoughts?


Bye,
Ulrich

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