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

Andreas Arnez
2017-04-19T14:36:00Z

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