Re: RFC: implement typed DWARF stack

[Tom Tromey <tromey@redhat.com>]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Tom> This patch converts the DWARF expression evaluator to use GDB's value
Tom> types.  This approach made it easy to support floating point and also
Tom> decimal floating point; and also paves the way for any future
Tom> improvements.

Ulrich> Huh, so value_binop is back after I eliminated it :-)
Ulrich> http://sourceware.org/ml/gdb-patches/2010-06/msg00514.html

Yeah, I actually referred back to this while writing the patch :)

Ulrich> Due to the use of value_as_address in dwarf_expr_fetch_address, this
Ulrich> patch actually ought to still work on the SPU ... I'll do a test.

Thank you.

Tom> There is some ugliness involving signed and unsigned types; this arises
Tom> because "old-style" untyped DWARF values don't have a consistent type.
Tom> Also I needed a little bit of special code to handle logical right
Tom> shifts.

Ulrich> Yes, I'm wondering whether old-style values are handled correctly.  We
Ulrich> used to make sure all arithmetic is performed in ctx->addr_size bits
Ulrich> (which is taken from the DWARF section headers).  With your patch,
Ulrich> we now always use gdbarch_dwarf2_addr_size -- I'm not sure this is
Ulrich> always the same.

Good point; I am not sure either.  Another option would be to use
ctx->addr_size to choose an arch type (e.g., builtin_uint32), and then
also carry along a flag indicating whether the value is "untyped".

I think this is needed because these untyped values must be treated
differently in a couple spots :-(

Yet another idea would be to lazily instantiate these special-to-DWARF
types and make struct dwarf_gdbarch_types a little bigger.  I think I
like this idea the best.  I think in practice we only need to support 3
such types (and if we run into more in the wild we can easily add them).

Ulrich> Also, what is the reason for handling the conversion to unsigned so
Ulrich> differently in the DW_OP_mod vs. DW_OP_shr cases?

There's no reason, I will clean this up.

Ulrich> [ In fact, maybe we don't need the whole value_cast business and
Ulrich> we could just operate on ULONGEST without involving value_binop,
Ulrich> since both cases only support integers anyway ... ]

I agree, for DW_OP_shr.  I will do that.  Unfortunately I think
DW_OP_mod still needs special magic.

Ulrich> I'd rather see a dwarf_expr_push_address to keep the
Ulrich> address-type abstraction local to dwarf2expr.c ...

Will do.

Ulrich> Does DW_OP_bra really require an integral type on the stack?
Ulrich> The standard wording isn't 100% clear to me here ...

A couple of oddities were clarified in this thread:

    http://gcc.gnu.org/ml/gcc-patches/2011-05/msg00333.html

There, Cary said that DW_OP_bra should require an integral type and this
was just an oversight in the spec.

Ulrich> I guess a non_lval value would still seem cleaner here (just as done
Ulrich> below for DW_OP_GNU_reinterpret --- maybe this could be abstracted
Ulrich> into a new value_from_contents helper).

Will do.

Tom> @@ -182,7 +189,7 @@ struct dwarf_expr_piece
Tom> 
Tom> /* The piece's register number or literal value, for
Tom> DWARF_VALUE_REGISTER or DWARF_VALUE_STACK pieces.  */
Tom> -    ULONGEST value;
Tom> +    struct value *value;

Ulrich> Maybe now it would be cleaner to split this into two union members,
Ulrich> a plain "int regnum" for DWARF_VALUE_REGISTER, and the struct value
Ulrich> for DWARF_VALUE_STACK ...

That does seem better, I will do that too.

Ulrich> Otherwise this looks good to me.

Thanks very much for the review.  I'll post a new patch when I've made
the needed changes.

Tom