Re: [PATCH] MIPS bit field failures in gdb.base/store.exp

[Pedro Alves <alves.ped@gmail.com>]

On 09/29/2014 01:36 PM, Luis Machado wrote:
> Hi,
> 
> On 09/26/2014 12:16 PM, Pedro Alves wrote:
>> On 09/25/2014 08:31 PM, Luis Machado wrote:
>>
>>> ping! Any ideas on different approaches suitable for this problem or is
>>> the proposed fix ok (with either passing a value struct or a bit size)?
>>
>> Sorry, it's not easy to have a quick opinion without thinking this
>> through...
>>
>> So, in value_assign, the case in question, we see:
>>
>> 	gdbarch = get_frame_arch (frame);
>> 	if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), type))
>> 	  {
>> 	    /* If TOVAL is a special machine register requiring
>> 	       conversion of program values to a special raw
>> 	       format.  */
>> 	    gdbarch_value_to_register (gdbarch, frame,
>> 				       VALUE_REGNUM (toval), type,
>> 				       value_contents (fromval));
>> 	  }
>>
>> Notice how gdbarch_value_to_register takes the fromval's contents
>> as a buffer, only, and isn't passed down anything that would make it
>> possible to find out whether it's writing to a bitfield, so that
>> the implementation could do a read-modify-write itself and
>> write to the proper bitfield offset.
>>
>> So, it seems to me that until we find an arch that needs to handle
>> bitfields especially (I'm having trouble imagining why that
>> would be necessary), we should just change value_assign's
>> lval_register handling from:
>>
>> 	if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), type))
>> 	  {	
>>               gdbarch_value_to_register ();
>> 	  }
>> 	else
>> 	  {
>> 	    if (value_bitsize (toval))
>>                 {
>>                     // read-modify-write
>>                 }
>>              else
>> 	       {
>> 		   put_frame_register_bytes ();
>>                 }
>>            }
>>
>> to:
>>
>>         if (value_bitsize (toval))
>>            {
>>                // read-modify-write
>>            }
>> 	else
>> 	  {
>>               if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), type))
>> 	       {
>>                    gdbarch_value_to_register ();
>> 	       }
>>              else
>> 	       {
>> 		  put_frame_register_bytes ();
>>                 }
>>            }
> 

> Though a bit less generic, that also seems to be a reasonable solution 
> for now, and it fixes the failures i saw for MIPS.

The proper solution for an arch that needs to treat bitfields differently
might well be to do without gdbarch_convert_register_p and change
gdbarch_value_to_register's parameters to
'gdbarch_value_to_register(gdbarch, toval, fromval)', and rename it
to gdbarch_register_assign while at it, as it's only called by value_assign.

Like:

	if (gdbarch_register_assign_p (gdbarch))
	  {	
              gdbarch_register_assign (gdbarch, toval, fromval);
	  }
	else
	  {
            // default fallback
          }

(Or install the fallback code as fallback gdbarch_register_assign
implementation and then just call gdbarch_register_assign directly.)

Seems unnecessary to do until we find a user that wants to treat
bitfields differently though.  Or viewed another way, we're discussing
what that "default fallback" code should look like.  :-)

> Out of the top of my
> head i also don't recall a target that handles bit fields in a special 
> way. Should i go with this patch for the next submission 

Yes, please.

> or do you want to author it?

Nope.

Thanks,
Pedro Alves