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

[Andreas Arnez <arnez@linux.vnet.ibm.com>]

On Fri, Mar 10 2017, Ulrich Weigand wrote:

> 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 ...

This patch specifically focuses on fixing the bug with stack value
pieces; I didn't want to intermingle it with fixes for the other piece
types.  So I'll drop the offset support from this patch and re-introduce
it with another patch that adds offset support for all piece types.

>
>>  	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).

OK, I'll look into providing a separate patch for that.

>
>> +	    /* 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.

OK.  (The original author seems to have thought differently, and I tried
to follow the existing scheme, to keep the changes local.)

>
> Bye,
> Ulrich