MIPS bit field failures in gdb.base/store.exp
Commit Message
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?
Comments
On 09/29/2014 06:35 PM, Luis Machado wrote:
> On 09/29/2014 10:30 AM, Pedro Alves wrote:
>>> >> or do you want to author it?
>> >
>> > Nope.
> How about the following small patch?
OK.
Thanks,
Pedro Alves
On 09/30/2014 08:00 AM, Pedro Alves wrote:
> On 09/29/2014 06:35 PM, Luis Machado wrote:
>> On 09/29/2014 10:30 AM, Pedro Alves wrote:
>
>>>>>> or do you want to author it?
>>>>
>>>> Nope.
>> How about the following small patch?
>
> OK.
>
> Thanks,
> Pedro Alves
>
Thanks. I've pushed this one now.
2014-09-29 Luis Machado <lgustavo@codesourcery.com>
gdb/
* valops.c (value_assign): Check for bit field assignments
before calling architecture-specific register value
conversion functions.
@@ -1112,52 +1112,54 @@ value_assign (struct value *toval, struct value *fromval)
error (_("Value being assigned to is no longer active."));
gdbarch = get_frame_arch (frame);
- if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), type))
+
+ if (value_bitsize (toval))
{
- /* 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));
+ struct value *parent = value_parent (toval);
+ int offset = value_offset (parent) + value_offset (toval);
+ int changed_len;
+ gdb_byte buffer[sizeof (LONGEST)];
+ int optim, unavail;
+
+ changed_len = (value_bitpos (toval)
+ + value_bitsize (toval)
+ + HOST_CHAR_BIT - 1)
+ / HOST_CHAR_BIT;
+
+ if (changed_len > (int) sizeof (LONGEST))
+ error (_("Can't handle bitfields which "
+ "don't fit in a %d bit word."),
+ (int) sizeof (LONGEST) * HOST_CHAR_BIT);
+
+ if (!get_frame_register_bytes (frame, value_reg, offset,
+ changed_len, buffer,
+ &optim, &unavail))
+ {
+ if (optim)
+ throw_error (OPTIMIZED_OUT_ERROR,
+ _("value has been optimized out"));
+ if (unavail)
+ throw_error (NOT_AVAILABLE_ERROR,
+ _("value is not available"));
+ }
+
+ modify_field (type, buffer, value_as_long (fromval),
+ value_bitpos (toval), value_bitsize (toval));
+
+ put_frame_register_bytes (frame, value_reg, offset,
+ changed_len, buffer);
}
else
{
- if (value_bitsize (toval))
+ if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval),
+ type))
{
- struct value *parent = value_parent (toval);
- int offset = value_offset (parent) + value_offset (toval);
- int changed_len;
- gdb_byte buffer[sizeof (LONGEST)];
- int optim, unavail;
-
- changed_len = (value_bitpos (toval)
- + value_bitsize (toval)
- + HOST_CHAR_BIT - 1)
- / HOST_CHAR_BIT;
-
- if (changed_len > (int) sizeof (LONGEST))
- error (_("Can't handle bitfields which "
- "don't fit in a %d bit word."),
- (int) sizeof (LONGEST) * HOST_CHAR_BIT);
-
- if (!get_frame_register_bytes (frame, value_reg, offset,
- changed_len, buffer,
- &optim, &unavail))
- {
- if (optim)
- throw_error (OPTIMIZED_OUT_ERROR,
- _("value has been optimized out"));
- if (unavail)
- throw_error (NOT_AVAILABLE_ERROR,
- _("value is not available"));
- }
-
- modify_field (type, buffer, value_as_long (fromval),
- value_bitpos (toval), value_bitsize (toval));
-
- put_frame_register_bytes (frame, value_reg, offset,
- changed_len, buffer);
+ /* 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));
}
else
{