Re: RFC: implement typed DWARF stack

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

Tom Tromey wrote:
> >>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> Ulrich> Huh, interesting approach.  In a sense, that might be OK, since
> Ulrich> it mirrors what we're doing in dwarf_expr_read_reg by calling
> Ulrich> address_from_register.  On the other hand, I'm not sure
> Ulrich> value_cast always does the right thing if the size of a pointer
> Ulrich> type differs from the size of the DWARF address type ...
> 
> I had not considered that as a possibility.  I think the most obviously
> safe thing to do is just revert dwarf_expr_fetch_address to (mostly)
> resemble its pre-patch state.  I will do that and test it.

Yes, I agree your latest version of dwarf_expr_fetch_address should
be obviously safe, that is, getting the same result as prior to the
change.  I've tested your patch on Cell/B.E. with no regressions
(using both -m32 and -m64 for the PowerPC side).

Another option that occurred to me in the meantime would be to ensure
that untyped "old-style" DWARF values are represented by an *unsigned*
type (either always, and converted to signed for operations that need
it, or else just converted to unsigned in dwarf_expr_fetch_address),
so that calling value_as_address will then do the right thing ...

> Ulrich> Another issue that just occurred to me: your patch creates
> Ulrich> possibly many temporary struct value objects.  I'm wondering
> Ulrich> whether those ought to be released from the value chain at some
> Ulrich> point ...
> 
> I considered this but talked myself out of it using the following
> reasoning:
> 
> 1. Most DWARF expressions are simple, so in practice not many values
>    will be released;
> 2. The unwinder code is value based but does not seem to call
>    value_free_to_mark, so it must not be significant there;

But it does call release_value; see frame.c:frame_register_unwind:

  /* Dispose of the new value.  This prevents watchpoints from
     trying to watch the saved frame pointer.  */
  release_value (value);
  value_free (value);

> 3. In other (expression-evaluation) contexts, some caller is going to
>    free the values anyway;
> 4. The watchpoint code looks at the value stack to determine what
>    intermediate values to watch, and perhaps the values from the DWARF
>    expression are relevant (though ... it occurs to me just now that
>    this approach must be pretty broken in the presence of location
>    lists).
> 
> I am actually not sure if #4 is an argument for or against.  Maybe those
> intermediate values confuse things; there is a comment in
> value_fetch_lazy indicating that this may be the case.

Yes, that is my concern -- that there could be intermediate values that
are *not* actually appropriate to watch ...


I noticed one more minor buglet in the latest patch:

@@ -576,7 +590,7 @@ read_pieced_value (struct value *v)
 	case DWARF_VALUE_REGISTER:
 	  {
 	    struct gdbarch *arch = get_frame_arch (frame);
-	    int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->v.value);
+	    int gdb_regnum = gdbarch_dwarf2_reg_to_regnum (arch, p->v.regno);
 	    int reg_offset = source_offset;
 
 	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG
@@ -609,7 +623,7 @@ read_pieced_value (struct value *v)
 	    else
 	      {
 		error (_("Unable to access DWARF register number %s"),
-		       paddress (arch, p->v.value));
+		       paddress (arch, value_as_long (p->v.value)));

That should be p->v.regno here (and at another place a bit farther down).

Otherwise this looks good to me.

Bye,
Ulrich

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