Re: [PATCH 2/9] Fix size capping in write_pieced_value

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

On Thu, Apr 13 2017, Yao Qi wrote:

> Andreas Arnez <arnez@linux.vnet.ibm.com> writes:
>
>> A field in a structure composed of DWARF pieces overlaps one or more of
>> those pieces.  When writing to the field, the beginning of the first and
>> the end of the last of those pieces may have to be skipped.  But the
>
> Do you have an example?

OK, I guess the commit message should be improved a bit.  How about
this?

  A field f in a structure composed of DWARF pieces may be located in
  multiple pieces, where the first and last of those may contain bits
  from other fields as well.  So when writing to f, the beginning of the
  first and the end of the last of those pieces may have to be skipped.
  But the logic in write_pieced_value for handling one of those pieces
  is flawed when the first and last piece are the same, i.e., f is
  contained in a single piece:

    < - - - - - - - - - piece_size - - - - - - - - - ->
    +-------------------------------------------------+
    | skipped_bits |   f_bits   | / / / / / / / / / / |
    +-------------------------------------------------+

  The current logic determines the size of the sub-piece to operate on
  by limiting the piece size to the bit size of f and then subtracting
  the skipped bits:

    max (piece_size, f_bits) - skipped_bits

  Instead of:

    max (piece_size - skipped_bits, f_bits)

  So the resulting sub-piece size is corrupted, leading to wrong
  handling of this piece in write_pieced_value.

>
>> logic in write_pieced_value for handling this is flawed when there are
>> actually bits to skip at the beginning of the first piece: it truncates
>> the piece size towards the end *before* accounting for the skipped bits
>> at the beginning instead of the other way around.
>>
>> Note that the same bug was already found in read_pieced_value and fixed
>> there (but not in write_pieced_value), see PR 15391.
>
> Can we share the code in write_pieced_value and read_pieced_value?  The
> code computing offsets and bits should be shared.

Yes.  I have another patch (not posted yet) that merges these two
functions.  I moved that towards the end of the patch series, so the
individual fixes can be incremental.

> Also, we need more comments in the code to explain these offsets and
> bits, a diagram about the relationships of these bits and offsets is
> quite helpful.

OK.  Some of the offset variables are removed by my patches, so I guess
I'll postpone that to the merged version.  I'll see what I can come up
with and include it in v2.

--
Andreas