Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end

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

Andreas Arnez wrote:

> When taking a DW_OP_piece or DW_OP_bit_piece from a DW_OP_stack_value, the
> existing logic always takes the piece from the lowest-addressed end, which
> is wrong on big-endian targets.  The DWARF standard states that the
> "DW_OP_bit_piece operation describes a sequence of bits using the least
> significant bits of that value", and this also matches the current logic
> in GCC.  For instance, the GCC guality test case pr54970.c fails on s390x
> because of this.
> 
> This fix adjusts the piece accordingly on big-endian targets.  It is
> assumed that:
> 
> * DW_OP_piece shall take the piece from the LSB end as well;
> 
> * pieces reaching outside the stack value bits are considered undefined,
>   and a zero value can be used instead.

These assumptions look good to me.
 
> The new logic also respects the DW_OP_bit_piece offset for
> DW_OP_stack_values.  Previously the offset was ignored.  Note that it is
> still ignored for all other types of DWARF pieces.

I'm not really sure about that.  If we're going to support the offset,
shouldn't we then support it for all piece types?   I'm not sure it
is a good idea to support it for some but not others ...

>  	case DWARF_VALUE_STACK:
>  	  {
> -	    size_t n = this_size;
> +	    ULONGEST obj_size = 8 * TYPE_LENGTH (value_type (p->v.value));
>  
> -	    if (n > c->addr_size - source_offset)
> -	      n = (c->addr_size >= source_offset
> -		   ? c->addr_size - source_offset
> -		   : 0);

c->addr_size is now unused.  This is a left-over from the days when stack
values were untyped, and were always assumed to be integers the size of
a target address.  Now that we have a proper typed stack, c->addr_size
is not really relevant anymore.  It should be completely removed from
the piece_closure data structure (and all code that initializes it).

> +	    /* Use zeroes if piece reaches beyond stack value.  */
> +	    if (p->offset + p->size > obj_size)
> +	      goto skip_copy;

I'm not sure I like those gotos :-(

> -      if (p->location != DWARF_VALUE_OPTIMIZED_OUT
> -	  && p->location != DWARF_VALUE_IMPLICIT_POINTER)
> -	copy_bitwise (contents, dest_offset_bits,
> -		      intermediate_buffer, source_offset_bits % 8,
> -		      this_size_bits, bits_big_endian);
> +      copy_bitwise (contents, dest_offset_bits,
> +		    intermediate_buffer, source_offset_bits % 8,
> +		    this_size_bits, bits_big_endian);
>  
> +    skip_copy:

At this point, it seems better to just duplicate the copy_bitwise
call to all those switch clauses that actually need it.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com