Re: [PATCH 5/9] Fix issues in write_pieced_value when targeting bit-fields

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-04-27 13:54, Andreas Arnez wrote:
> On Fri, Apr 14 2017, Simon Marchi wrote:
>
> > On 2017-04-07 13:38, Andreas Arnez wrote:
> > > There are multiple issues in write_pieced_value when dealing with a
> > > bit-field as the target value:
> > >
> > > (1) The number of bits preceding the bit-field is calculated without
> > >     considering the relative offset of the value's parent.
> >
> > If others are wondering when this can happen:
> >
> > struct s {
> >     uint64_t foo;
> >     struct {
> > 	uint32_t bar;
> > 	uint32_t bf : 10;
> >     } baz;
> > };
> >
> > If "val" is a struct value representing bf:
> >
> >  - value_offset(val) == 4 (sizeof bar)
> >  - val->parent represents the whole baz structure
> >  - value_offset(val->parent) == 8 (sizeof foo)
> >
> > If bf was a "standard", non-bitfield variable, its offset would be 12
> > directly.
>
> Right.  Now that you mention it, I realize that the test case doesn't
> cover this yet.  So I'm enhancing it.

I did not realize that, but I'm glad you did :).

> > There are multiple places that do "value_offset (parent) + 
> > value_offset
> > (value)" when value is a bitfield.  Isn't it what we want most of the
> > time?  If so, I wonder if eventually value_offset shouldn't instead 
> > return
> > the computed offset, like:
> >
> > LONGEST
> > value_offset (const struct value *value)
> > {
> >   if (value_bitsize (value))
> >     return value->offset + value_offset (value_parent (value));
> >   else
> >     return value->offset;
> > }
>
> Maybe, but what would set_value_offset do then?

Indeed, it's probably better not to break the symmetry.

> To be honest, I don't completely understand the definition of
> value_offset, so I decided not to change anything there with this 
> patch.
> Maybe we should spend some time refactoring the value API, but I think
> this is a separate task.

Yes, it was just brainstorming for future enhancements.

> > >        type_len = value_bitsize (v);
> > >      }
> > >    else
> > > @@ -1796,18 +1797,11 @@ read_pieced_value (struct value *v)
> > >  	  bits_to_skip -= this_size_bits;
> > >  	  continue;
> > >  	}
> > > -      if (bits_to_skip > 0)
> > > -	{
> > > -	  dest_offset_bits = 0;
> > > -	  source_offset_bits = bits_to_skip;
> > > -	  this_size_bits -= bits_to_skip;
> > > -	  bits_to_skip = 0;
> > > -	}
> > > -      else
> > > -	{
> > > -	  dest_offset_bits = offset;
> > > -	  source_offset_bits = 0;
> > > -	}
> > > +      source_offset_bits = bits_to_skip;
> > > +      this_size_bits -= bits_to_skip;
> > > +      bits_to_skip = 0;
> > > +      dest_offset_bits = offset;
> > > +
> >
> > Is this snippet related to one of the problems you have described?  It
> > seems to me like it's just simplifying the code, but it's not changing 
> > the
> > behavior.  If that's the case, I'd suggest putting it in its own patch
> > (along with its equivalent in write_pieced_value).
>
> This is just to mirror the change in write_pieced_value.  See below for
> the rationale of that.
>
> > >        if (this_size_bits > type_len - offset)
> > >  	this_size_bits = type_len - offset;
> > >
> > > @@ -1942,8 +1936,16 @@ write_pieced_value (struct value *to, struct
> > > value *from)
> > >    bits_to_skip = 8 * value_offset (to);
> > >    if (value_bitsize (to))
> > >      {
> > > -      bits_to_skip += value_bitpos (to);
> > > +      bits_to_skip += (8 * value_offset (value_parent (to))
> > > +		       + value_bitpos (to));
> > >        type_len = value_bitsize (to);
> > > +      /* Use the least significant bits of FROM.  */
> > > +      if (gdbarch_byte_order (get_type_arch (value_type (from)))
> > > +	  == BFD_ENDIAN_BIG)
> > > +	{
> > > +	  offset = 8 * TYPE_LENGTH (value_type (from)) - type_len;
> > > +	  type_len += offset;
> > > +	}
> >
> > I guess this is related to (1) and (2).
>
> Right.  Note that 'offset' may now be nonzero upon entering the loop.
>
> > >      }
> > >    else
> > >      type_len = 8 * TYPE_LENGTH (value_type (to));
> > > @@ -1962,25 +1964,19 @@ write_pieced_value (struct value *to, struct
> > > value *from)
> > >  	  bits_to_skip -= this_size_bits;
> > >  	  continue;
> > >  	}
> > > -      if (bits_to_skip > 0)
> > > -	{
> > > -	  dest_offset_bits = bits_to_skip;
> > > -	  source_offset_bits = 0;
> > > -	  this_size_bits -= bits_to_skip;
> > > -	  bits_to_skip = 0;
> > > -	}
> > > -      else
> > > -	{
> > > -	  dest_offset_bits = 0;
> > > -	  source_offset_bits = offset;
> > > -	}
> > > +      dest_offset_bits = bits_to_skip;
> > > +      this_size_bits -= bits_to_skip;
> > > +      bits_to_skip = 0;
> > > +      source_offset_bits = offset;
> > > +
>
> This is related to (2).  The old version assumed that 'offset' is still
> zero when there are bits to skip, so a literal zero (instead of
> 'offset') could be copied into source_offset_bits.  This assumption
> doesn't hold any longer, now that 'offset' may start with a nonzero
> value.

Ok, I though it was just some simplification.  The new code is 
equivalent to both branches of the if/else that's being removed for 
dest_offset_bits, this_size_bits and bits_to_skip.  But you're right, 
for source_offset_bits it changes the behaviour.  In any case, I like 
the new code better, less branches :).

> As you can see, the patch fixes 5 different, fairly independent bugs.
> Thus it could have been split into 5 patches, but I found that a bit
> extreme...

I think it's worth splitting it*, it will help understand each issue and 
corresponding fix independently.  It's some non-trivial work you're 
doing here, so think about external observers that want to take a look 
at it :). Patches are cheap after all.

*unless it adds to much complexity to have these intermediary states.

Thanks,

Simon