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

Pedro Alves
2014-09-29T13:30:27Z

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