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

[Luis Machado <lgustavo@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 3598 bytes --]

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?




[-- Attachment #2: mips_bit_field.diff --]
[-- Type: text/x-patch, Size: 3399 bytes --]

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.

diff --git a/gdb/valops.c b/gdb/valops.c
index e1decf0..f177907 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -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
 	      {