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

[Pedro Alves <palves@redhat.com>]

On 12/04/2013 03:26 PM, Andrew Burgess wrote:

>     	* gdb/dwarf2loc.c (read_pieced_value): Mark bits, not bytes
>     	unavailable, use correct bit length.

No "gdb/" prefix.


>     	(unpack_value_bits_as_long_1): Check availability in bits, bot
>     	bytes.

Typo: "bot".

>     	* gdb/value.h (int value_bits_available): Declare new function.

Stale "int".

>     	(mark_value_bits_unavailable): Declare new function.


>  *val1, int offset1,
>  	return 0;
>  
>        /* 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?

> +    gdb_test "tfind 0" "Found trace frame 0, tracepoint .*"
> +    gdb_test "p x" "\\\$${decimal} = {a = 0 '\\\\000', b = <unavailable>, c = 0 '\\\\000', d = 0 '\\\\000', e = 0 '\\\\000', f = 0 '\\\\000', g = 0 '\\\\000', h = 0 '\\\\000', i = 0 '\\\\000', j = 0 '\\\\000'}"
> +    gdb_test "p y" "\\\$${decimal} = {a = 0 '\\\\000', b = 0 '\\\\000', c = 0 '\\\\000', d = 0 '\\\\000', e = <unavailable>, f = 0 '\\\\000', g = 0 '\\\\000', h = 0 '\\\\000', i = 0 '\\\\000', j = 0 '\\\\000'}"
> +    gdb_test "p z" "\\\$${decimal} = {a = 0 '\\\\000', b = 0 '\\\\000', c = 0 '\\\\000', d = 0 '\\\\000', e = 0 '\\\\000', f = 0 '\\\\000', g = 0 '\\\\000', h = 0 '\\\\000', i = <unavailable>, j = 0 '\\\\000'}"

(I think you could have used "p/d" or "p/x" to simplify the output.)

> +
> +    gdb_test "tfind none" "No longer looking at any trace frame.*"


> +void
> +end ()

end (void)

There are more instances.




> +  struct s v = { 0, 1, 2};

Missing space before }.

> +  struct t w = { 5, 0, 1, 0, 1, 0, 1, 0, 1, 7 };


Otherwise looks good to me.

-- 
Pedro Alves