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

[Andreas Arnez <arnez at linux dot vnet dot ibm dot com>]

On Wed, Apr 19 2017, Yao Qi wrote:

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

Right, I'll add some general explanation and a diagram about the various
bits and offsets (as requested below).

However, most of the commit message explains a specific bug in a piece
of code that won't exist any more.  This aspect doesn't make sense to be
included in the comments, I think.

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

Argh, the "max" above is obviously meant to be "min".  Sorry for the
confusion.

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

Not sure I understand.  Do you mean to merge the functions first while
preserving existing logic, including all the bugs and differences?  I
had started along this path and gave up on it, because I found it too
complicated.  From that attempt I've concluded that the current approach
is much less error-prone and easier to follow.

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

Sure.

--
Andreas