Re: RFC: implement typed DWARF stack

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

Tom Tromey wrote:

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

Huh, so value_binop is back after I eliminated it :-)
http://sourceware.org/ml/gdb-patches/2010-06/msg00514.html
Due to the use of value_as_address in dwarf_expr_fetch_address, this
patch actually ought to still work on the SPU ... I'll do a test.

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

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

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

+		/* We have to special-case "old-style" untyped values
+		   -- these must have mod computed using unsigned
+		   math.  */
+		if (value_type (first) == address_type)
+		  {
+		    first = value_cast (uaddress_type, first);
+		    second = value_cast (uaddress_type, second);
+		  }

vs.

+		dwarf_require_integral (value_type (first));
+		dwarf_require_integral (value_type (second));
+		if (!TYPE_UNSIGNED (value_type (first)))
+		  {
+		    struct type *utype
+		      = get_unsigned_type (ctx->gdbarch, value_type (first));
+
+		    first = value_cast (utype, first);
+		  }

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

> -  dwarf_expr_push (ctx, initial, initial_in_stack_memory);
> +  dwarf_expr_push (ctx, value_from_ulongest (dwarf_expr_address_type (ctx, 1),
> +					     initial),
> +		   initial_in_stack_memory);

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

>  	case DW_OP_bra:
> -	  offset = extract_signed_integer (op_ptr, 2, byte_order);
> -	  op_ptr += 2;
> -	  if (dwarf_expr_fetch (ctx, 0) != 0)
> -	    op_ptr += offset;
> -	  dwarf_expr_pop (ctx);
> +	  {
> +	    struct value *val;
> +
> +	    offset = extract_signed_integer (op_ptr, 2, byte_order);
> +	    op_ptr += 2;
> +	    val = dwarf_expr_fetch (ctx, 0);
> +	    dwarf_require_integral (value_type (val));

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

> +	case DW_OP_GNU_const_type:
> +	  {
> +	    ULONGEST type_die;
> +	    int n;
> +	    const gdb_byte *data;
> +	    struct type *type;
> +
> +	    op_ptr = read_uleb128 (op_ptr, op_end, &type_die);
> +	    n = *op_ptr++;
> +	    data = op_ptr;
> +	    op_ptr += n;
> +
> +	    type = dwarf_get_base_type (ctx, type_die, n);
> +
> +	    /* Note that the address does not matter, since there is
> +	       no way to fetch it.  */
> +	    result_val = value_from_contents_and_address (type, data, 0);

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

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

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

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