On 09/29/2014 10:30 AM, Pedro Alves wrote:
> 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.

How about the following small patch?