Re: [PATCH] Handle bitfields inside inner structs for internalvars

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, February 08 2013, Jan Kratochvil wrote:

> On Wed, 06 Feb 2013 21:39:10 +0100, Sergio Durigan Junior wrote:
>> --- a/gdb/testsuite/gdb.base/bitfields.exp
>> +++ b/gdb/testsuite/gdb.base/bitfields.exp
>> @@ -245,6 +245,31 @@ proc bitfield_at_offset {} {
>>      gdb_test "print container.two.u3" ".* = 3"
>>  }
>>  
>> +proc bitfield_internalvar {} {
>> +    global gdb_prompt
>> +
>> +    # First, we create an internal var holding an instance of
>> +    # the struct (zeroed out).
>> +    gdb_test "set \$myvar = \(struct internalvartest\) \{0\}" "" \
>
> Backslashes are excessive here, parentheses are not special characters for the
> TCL interpretation and this string is not regexp:
>        gdb_test "set \$myvar = (struct internalvartest) \{0\}" "" \

Thanks for the review.  These extra backslashes were added by me while
testing a failure that I was seeing, sorry for letting those sneak in.

>> --- a/gdb/valops.c
>> +++ b/gdb/valops.c
>> @@ -1233,11 +1233,18 @@ value_assign (struct value *toval, struct value *fromval)
>>  				   VALUE_INTERNALVAR (toval));
>>  
>>      case lval_internalvar_component:
>> -      set_internalvar_component (VALUE_INTERNALVAR (toval),
>> -				 value_offset (toval),
>> -				 value_bitpos (toval),
>> -				 value_bitsize (toval),
>> -				 fromval);
>> +      {
>> +	int offset = value_offset (toval);
>> +
>> +	if (value_bitsize (toval))
>> +	  offset += value_offset (value_parent (toval));
>
> value_address rather tests for value_parent existence; although value_bitsize
> is right as value_parent is currently not used elsewhere.
>
> 	if (value_parent (toval))

Do you think it's clearer to use `value_parent' here instead of
`value_bitsize'?

> 	  {
> 	    /* VALUE_INTERNALVAR below corresponds to the parent value while
> 	       offset is relative to the parent value.  */
> 	    gdb_assert (value_parent (value_parent (toval)) == NULL);
> 	    offset += value_offset (value_parent (toval));
> 	  }
>
> And it was not so clear to me on the first look so I have added this comment
> if you are OK with it.

Thanks, the comment is fine by me.  I'll wait for your reply to the
question above, and then proceed.

-- 
Sergio