[2/9] Fix size capping in write_pieced_value

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

This patch swaps the calculations, bringing them into the same (correct)
order as in read_pieced_value.

gdb/ChangeLog:

	* dwarf2loc.c (write_pieced_value): Fix order of calculations for
	size capping.

gdb/testsuite/ChangeLog:

	* gdb.dwarf2/var-pieces.exp: Add test case for modifying a
	variable at nonzero offset.
---
 gdb/dwarf2loc.c                         | 4 ++--
 gdb/testsuite/gdb.dwarf2/var-access.exp | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)
  

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?

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

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
  

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.
  

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
  

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

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

OK, no problem.

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

What I meant is that 1) make two parts identical (but not introducing
new bugs) 2) merge the code to a single function, 3) fix the rest of
bugs in the single function,

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

There may be some complexity I didn't realize.  I don't want to make
your life harder.  If you are comfortable on this approach, fine by me.