This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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?



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
 	      {

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]