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

Yao Qi
2017-04-19T09:15:38Z

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 (齐尧)