This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end
- From: Andreas Arnez <arnez at linux dot vnet dot ibm dot com>
- To: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 10 Mar 2017 20:24:04 +0100
- Subject: Re: [PATCH] PR gdb/21226: Take DWARF stack value pieces from LSB end
- Authentication-results: sourceware.org; auth=none
- References: <20170310174317.2A0E0D806B1@oc3748833570.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