Re: [PATCH [2/2] Convert the unavailable vector to be bit, not byte, based.

[Pedro Alves <palves at redhat dot com>]

On 12/09/2013 09:04 PM, Andrew Burgess wrote:
> On 05/12/2013 4:18 PM, Pedro Alves wrote:
>> On 12/04/2013 03:26 PM, Andrew Burgess wrote:
>>
>>>  
>>>        /* Compare the _available_ contents.  */
>>> -      if (memcmp (val1->contents + offset1,
>>> -		  val2->contents + offset2,
>>> -		  l1) != 0)
>>> +      if (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT),
>>> +		  val2->contents + (offset2 / TARGET_CHAR_BIT),
>>> +		  (l1 / TARGET_CHAR_BIT)) != 0)
>>>  	return 0;
>>
>> As memcmp compares bytes, isn't this potentially comparing bits
>> at the beginning and end of the values' buffers, when it
>> should not?   That is, it looks like the
>> 'offset1 % TARGET_CHAR_BIT != 0' and
>> '(offset1 + l1) % TARGET_CHAR_BIT' cases should be considered here?
>>
> 
> Thanks for the review.  You're right, this isn't doing the correct thing
> here, I should have written something like this:
> 
> 
> +      if (memcmp (val1->contents + (offset1 / TARGET_CHAR_BIT),
> +		  val2->contents + (offset2 / TARGET_CHAR_BIT),
> +		  ((l1 + TARGET_CHAR_BIT - 1)/ TARGET_CHAR_BIT)) != 0)
> 
> That is round the start down to the nearest byte boundary, and round the
> length up to the nearest byte boundary.
> 
> I figured that as the value content buffer is always a round number of
> bytes then there will always be memory backing the access, and as the
> content buffer is zero initialized comparing the "unavailable" bits will
> not cause the compare to fail.

Yeah, given value_available_contents_eq's interface works with bytes,
not bits, we know we'll always be comparing up to byte boundaries, so
even though this iteration may compare more bits than the current
iterated range covers, it's fine, we'd compare them the next
iteration anyway.  Something like:

     bit offset:   0 1 2 3  4 5 6 7
val1's contents: [ 1 U U U  2 2 2 2 ]
val2's contents: [ 1 U U U  5 5 5 5 ]
bits to compare: [ X X X X  X X X X ]

The first time, we'll memcmp bits bits 0-7 and return false,
even though the first available range (bit 0), compares equal.

This deserves a comment though.

> Alternatively, I can update to only access the bits we'd like to compare.

Hmm.  That'd be necessary if value_available_contents_eq accepted
bit offsets instead of byte offsets.  E.g,. consider two values
with these contents:

     bit offset:   0 1 2 3  4 5 6 7
val1's contents: [ 1 U U U  5 5 5 5 ]
val2's contents: [ 0 U U U  5 5 5 5 ]
bits to compare: [       X  X X X X ]

We'd have to make sure to not compare (or mask off before
comparing) the bit at offset 0.

I wonder if it wouldn't make it for more readable code
(and less fragile, e.g., what if we want to stop zero
initializing the contents buffer) if we handled this.  That is,
rename value_available_contents_eq to something like
value_available_contents_bits_eq (and make it static), have it accept
bit offsets, and take that case into consideration.  Then,
make value_available_contents_eq a wrapper that multiplies offsets/lengths
by TARGET_CHAR_BIT.

		if (!get_frame_register_bytes (frame, gdb_regnum, reg_offset,
					       this_size, buffer,
					       &optim, &unavail))
		  {
		    /* Just so garbage doesn't ever shine through.  */
		    memset (buffer, 0, this_size);


> Which approach would would be best?

I'm fine either way, but if we go the former route, then
please add a comment mentioning the assumptions.

> 
> Thanks for your advice,
> Andrew
> 

-- 
Pedro Alves