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

[Yao Qi <qiyaoltc@gmail.com>]

Andreas Arnez <arnez@linux.vnet.ibm.com> writes:

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

Hi Andreas,
The description to the logic can go to comments, so that we don't need
to do "git blame/log" to understand the code.

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

Given this example, the result is the same, which is
"piece_size - skipped_bits", am I missing something?

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

I'd like to merge the code first, then don't need to fix the same
problem in two functions read_pieced_value and write_pieced_value (your
patch 4/9 ~ 9/9 touches both two functions).

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

Please include it in V2.

-- 
Yao (齐尧)