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

[Luis Machado <lgustavo at codesourcery dot com>]

Hi,

I've been chasing this one for a while, trying to come up with the best 
solution while not having to do many changes to GDB's type system.

On MIPS64 little endian, attempting an assignment to a bit field that 
lives in a register yields the wrong result. It just corrupts the data 
in the register depending on the specific position of the bit field 
inside the structure.

FAIL: gdb.base/store.exp: f_1.j
FAIL: gdb.base/store.exp: f_1.k
FAIL: gdb.base/store.exp: F_1.i
FAIL: gdb.base/store.exp: F_1.j
FAIL: gdb.base/store.exp: F_1.k
FAIL: gdb.base/store.exp: f_2.j
FAIL: gdb.base/store.exp: f_2.k
FAIL: gdb.base/store.exp: F_2.i
FAIL: gdb.base/store.exp: F_2.j
FAIL: gdb.base/store.exp: F_2.k
FAIL: gdb.base/store.exp: f_3.j
FAIL: gdb.base/store.exp: f_3.k
FAIL: gdb.base/store.exp: F_3.i
FAIL: gdb.base/store.exp: F_3.j
FAIL: gdb.base/store.exp: F_3.k
FAIL: gdb.base/store.exp: f_4.j
FAIL: gdb.base/store.exp: f_4.k
FAIL: gdb.base/store.exp: F_4.i
FAIL: gdb.base/store.exp: F_4.j
FAIL: gdb.base/store.exp: F_4.k

                === gdb Summary ===

# of expected passes            220
# of unexpected failures        20

Now, GDB knows how to do bit field assignment properly, but MIPS is one 
of those architectures that uses a hook for the register-to-value 
conversion. Although we can properly tell when the type being passed is 
a structure or union, we cannot tell when it is a bit field, because the 
bit field data lives in a value structure. Such data only lives in a 
"type" structure when the parent structure is being referenced, thus you 
can collect them from the flds_bnds members.

A bit field type structure looks pretty much the same as any other 
primitive type like int or char, so we can't distinguish them. Forcing 
more fields into the type structure wouldn't help much, because the type 
structs are shared.

It feels to me GDB's type system is a bit dated and needs to be more 
precise about what it is describing, but for now i just want to fix a 
pending bug.

The most elegant solution i could find without having to touch a number 
of other type-related data structures is making the 
gdbarch_convert_register_p predicate accept a value structure instead of 
a type structure. That way we can properly tell when a bit field is 
being manipulated in registers.

There is still a little problem though. We don't always have a 
meaningful value struct to pass to this predicate, like both ocurrences 
of it in findvar.c. In those cases i went for a dummy value.

In the end, it is functional but a bit ugly. Unless folks have a better 
suggestion, is this ok?

I did tests with x86, mips32 be/le and mips64 be/le. No regressions found.

The lack of bit field data in the type struct also affects other 
functions that rely on type descriptions, though there may not be 
explicit failures due to those yet.

Luis

2014-09-12  Luis Machado  <lgustavo@codesourcery.com>

	* alpha-tdep.c (alpha_convert_register_p): Exchange struct
	type parameter for struct value.
	* arch-utils.c (generic_convert_register_p): Likewise.
	* arch-utils.h (generic_convert_register_p): Likewise.
	* findvar.c (value_from_register): Update call to
	gdbarch_convert_register_p.
	(address_from_register): Likewise.
	* gdbarch.c: Regenerate.
	* gdbarch.h: Regenerate.
	* gdbarch.sh (convert_register_p): Exchange struct
	type parameter for struct value.
	* i386-tdep.c (i386_next_regnum): Likewise.
	* i387-tdep.c (i387_convert_register_p): Likewise.
	* i387-tdep.h (i387_convert_register_p): Likewise.
	* ia64-tdep.c (ia64_convert_register_p): Likewise.
	* m68k-tdep.c (m68k_convert_register_p): Likewise.
	* mips-tdep.c (mips_convert_register_p): Likewise.
	Return 0 for bit field, struct and union values.
	* rs6000-tdep.c (rs6000_convert_register_p): Exchange struct
	type parameter for struct value.
	* valops.c (value_assign): Update call to
	gdbarch_convert_register_p.
	* value.c (value_fetch_lazy): Likewise.

diff --git a/gdb/alpha-tdep.c b/gdb/alpha-tdep.c
index 9e4a8d8..f8d2a2a 100644
--- a/gdb/alpha-tdep.c
+++ b/gdb/alpha-tdep.c
@@ -230,8 +230,10 @@ alpha_sts (struct gdbarch *gdbarch, void *out, const void *in)
 
 static int
 alpha_convert_register_p (struct gdbarch *gdbarch, int regno,
-			  struct type *type)
+			  struct value *val)
 {
+  struct type *type = value_type (val);
+
   return (regno >= ALPHA_FP0_REGNUM && regno < ALPHA_FP0_REGNUM + 31
 	  && TYPE_LENGTH (type) != 8);
 }
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 5ae9fb3..711aad5 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -218,7 +218,7 @@ legacy_virtual_frame_pointer (struct gdbarch *gdbarch,
 
 int
 generic_convert_register_p (struct gdbarch *gdbarch, int regnum,
-			    struct type *type)
+			    struct value *val)
 {
   return 0;
 }
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 46d1573..c36516e 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -102,7 +102,7 @@ extern int generic_in_function_epilogue_p (struct gdbarch *gdbarch,
 
 /* By default, registers are not convertible.  */
 extern int generic_convert_register_p (struct gdbarch *gdbarch, int regnum,
-				       struct type *type);
+				       struct value *val);
 
 extern int default_stabs_argument_has_addr (struct gdbarch *gdbarch,
 					    struct type *type);
diff --git a/gdb/findvar.c b/gdb/findvar.c
index cb98b86..ae16eee 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -701,11 +701,16 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
   struct gdbarch *gdbarch = get_frame_arch (frame);
   struct type *type1 = check_typedef (type);
   struct value *v;
+  struct value *dummy_value = allocate_value_lazy (type);
 
-  if (gdbarch_convert_register_p (gdbarch, regnum, type1))
+  /* We don't have enough value information at this point, so pass in a dummy
+     value so the conversion predicate can decide what to do.  */
+  if (gdbarch_convert_register_p (gdbarch, regnum, dummy_value))
     {
       int optim, unavail, ok;
 
+      value_free (dummy_value);
+
       /* The ISA/ABI need to something weird when obtaining the
          specified value from this register.  It might need to
          re-order non-adjacent, starting with REGNUM (see MIPS and
@@ -739,6 +744,7 @@ value_from_register (struct type *type, int regnum, struct frame_info *frame)
       read_frame_register_value (v, frame);
     }
 
+  value_free (dummy_value);
   return v;
 }
 
@@ -751,6 +757,7 @@ address_from_register (int regnum, struct frame_info *frame)
   struct gdbarch *gdbarch = get_frame_arch (frame);
   struct type *type = builtin_type (gdbarch)->builtin_data_ptr;
   struct value *value;
+  struct value *dummy_value = allocate_value_lazy (type);
   CORE_ADDR result;
 
   /* This routine may be called during early unwinding, at a time
@@ -762,11 +769,12 @@ address_from_register (int regnum, struct frame_info *frame)
 
   /* Some targets require a special conversion routine even for plain
      pointer types.  Avoid constructing a value object in those cases.  */
-  if (gdbarch_convert_register_p (gdbarch, regnum, type))
+  if (gdbarch_convert_register_p (gdbarch, regnum, dummy_value))
     {
       gdb_byte *buf = alloca (TYPE_LENGTH (type));
       int optim, unavail, ok;
 
+      value_free (dummy_value);
       ok = gdbarch_register_to_value (gdbarch, frame, regnum, type,
 				      buf, &optim, &unavail);
       if (!ok)
@@ -795,6 +803,7 @@ address_from_register (int regnum, struct frame_info *frame)
 
   result = value_as_address (value);
   release_value (value);
+  value_free (dummy_value);
   value_free (value);
 
   return result;
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index b0ee79d..bd97fde 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -2327,13 +2327,13 @@ set_gdbarch_believe_pcc_promotion (struct gdbarch *gdbarch,
 }
 
 int
-gdbarch_convert_register_p (struct gdbarch *gdbarch, int regnum, struct type *type)
+gdbarch_convert_register_p (struct gdbarch *gdbarch, int regnum, struct value *val)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->convert_register_p != NULL);
   if (gdbarch_debug >= 2)
     fprintf_unfiltered (gdb_stdlog, "gdbarch_convert_register_p called\n");
-  return gdbarch->convert_register_p (gdbarch, regnum, type);
+  return gdbarch->convert_register_p (gdbarch, regnum, val);
 }
 
 void
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 0303b2e..f83a995 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -410,8 +410,8 @@ extern void set_gdbarch_get_longjmp_target (struct gdbarch *gdbarch, gdbarch_get
 extern int gdbarch_believe_pcc_promotion (struct gdbarch *gdbarch);
 extern void set_gdbarch_believe_pcc_promotion (struct gdbarch *gdbarch, int believe_pcc_promotion);
 
-typedef int (gdbarch_convert_register_p_ftype) (struct gdbarch *gdbarch, int regnum, struct type *type);
-extern int gdbarch_convert_register_p (struct gdbarch *gdbarch, int regnum, struct type *type);
+typedef int (gdbarch_convert_register_p_ftype) (struct gdbarch *gdbarch, int regnum, struct value *val);
+extern int gdbarch_convert_register_p (struct gdbarch *gdbarch, int regnum, struct value *val);
 extern void set_gdbarch_convert_register_p (struct gdbarch *gdbarch, gdbarch_convert_register_p_ftype *convert_register_p);
 
 typedef int (gdbarch_register_to_value_ftype) (struct frame_info *frame, int regnum, struct type *type, gdb_byte *buf, int *optimizedp, int *unavailablep);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 2a8bca8..8a41f33 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -496,7 +496,7 @@ F:int:get_longjmp_target:struct frame_info *frame, CORE_ADDR *pc:frame, pc
 #
 v:int:believe_pcc_promotion:::::::
 #
-m:int:convert_register_p:int regnum, struct type *type:regnum, type:0:generic_convert_register_p::0
+m:int:convert_register_p:int regnum, struct value *val:regnum, val:0:generic_convert_register_p::0
 f:int:register_to_value:struct frame_info *frame, int regnum, struct type *type, gdb_byte *buf, int *optimizedp, int *unavailablep:frame, regnum, type, buf, optimizedp, unavailablep:0
 f:void:value_to_register:struct frame_info *frame, int regnum, struct type *type, const gdb_byte *buf:frame, regnum, type, buf:0
 # Construct a value representing the contents of register REGNUM in
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 1e68505..c29e4dd 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -3619,8 +3619,9 @@ i386_next_regnum (int regnum)
 
 static int
 i386_convert_register_p (struct gdbarch *gdbarch,
-			 int regnum, struct type *type)
+			 int regnum, struct value *val)
 {
+  struct type *type = value_type (val);
   int len = TYPE_LENGTH (type);
 
   /* Values may be spread across multiple registers.  Most debugging
@@ -3642,7 +3643,7 @@ i386_convert_register_p (struct gdbarch *gdbarch,
 	return 1;
     }
 
-  return i387_convert_register_p (gdbarch, regnum, type);
+  return i387_convert_register_p (gdbarch, regnum, val);
 }
 
 /* Read a value of type TYPE from register REGNUM in frame FRAME, and
diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index d66ac6a..6e707eb 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -336,8 +336,9 @@ i387_print_float_info (struct gdbarch *gdbarch, struct ui_file *file,
 
 int
 i387_convert_register_p (struct gdbarch *gdbarch, int regnum,
-			 struct type *type)
+			 struct value *val)
 {
+  struct type *type = value_type (val);
   if (i386_fp_regnum_p (gdbarch, regnum))
     {
       /* Floating point registers must be converted unless we are
diff --git a/gdb/i387-tdep.h b/gdb/i387-tdep.h
index e2eb0f5..4b2c242 100644
--- a/gdb/i387-tdep.h
+++ b/gdb/i387-tdep.h
@@ -90,7 +90,7 @@ extern void i387_print_float_info (struct gdbarch *gdbarch,
    needs any special handling.  */
 
 extern int i387_convert_register_p (struct gdbarch *gdbarch, int regnum,
-				    struct type *type);
+				    struct value *val);
 
 /* Read a value of type TYPE from register REGNUM in frame FRAME, and
    return its contents in TO.  */
diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index 776a8be..68ceae7 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -1211,8 +1211,10 @@ ia64_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
    and the special ia64 floating point register format.  */
 
 static int
-ia64_convert_register_p (struct gdbarch *gdbarch, int regno, struct type *type)
+ia64_convert_register_p (struct gdbarch *gdbarch, int regno, struct value *val)
 {
+  struct type *type = value_type (val);
+
   return (regno >= IA64_FR0_REGNUM && regno <= IA64_FR127_REGNUM
 	  && type != ia64_ext_type (gdbarch));
 }
diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index 6bb7e33..94d52e6 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -188,8 +188,10 @@ m68k_register_name (struct gdbarch *gdbarch, int regnum)
 
 static int
 m68k_convert_register_p (struct gdbarch *gdbarch,
-			 int regnum, struct type *type)
+			 int regnum, struct value *val)
 {
+  struct type *type = value_type (val);
+
   if (!gdbarch_tdep (gdbarch)->fpregs_present)
     return 0;
   return (regnum >= M68K_FP0_REGNUM && regnum <= M68K_FP0_REGNUM + 7
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 188580f..4d20784 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -847,10 +847,23 @@ mips_convert_register_gpreg_case_p (struct gdbarch *gdbarch, int regnum,
 
 static int
 mips_convert_register_p (struct gdbarch *gdbarch,
-			 int regnum, struct type *type)
-{
-  return (mips_convert_register_float_case_p (gdbarch, regnum, type)
-	  || mips_convert_register_gpreg_case_p (gdbarch, regnum, type));
+			 int regnum, struct value *val)
+{
+  int bitfield_or_struct = 0;
+  struct type *type = check_typedef (value_type (val));
+
+  /* Check for bitfields, structures or unions.  We do not handle
+     them here.  We leave those for generic code to handle.  */
+  if (value_bitsize (val) != 0
+      || TYPE_CODE (type) == TYPE_CODE_STRUCT
+      || TYPE_CODE (type) == TYPE_CODE_UNION)
+    bitfield_or_struct = 1;
+
+  return (!bitfield_or_struct
+	  && (mips_convert_register_float_case_p (gdbarch, regnum,
+						  value_type (val))
+	      || mips_convert_register_gpreg_case_p (gdbarch, regnum,
+						     value_type (val))));
 }
 
 static int
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index dabf448..28a5dba 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -2510,9 +2510,10 @@ rs6000_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
 
 static int
 rs6000_convert_register_p (struct gdbarch *gdbarch, int regnum,
-			   struct type *type)
+			   struct value *val)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  struct type *type = value_type (val);
 
   return (tdep->ppc_fp0_regnum >= 0
 	  && regnum >= tdep->ppc_fp0_regnum
diff --git a/gdb/valops.c b/gdb/valops.c
index e1decf0..49d36ad 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1112,7 +1112,7 @@ 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 (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), toval))
 	  {
 	    /* If TOVAL is a special machine register requiring
 	       conversion of program values to a special raw
diff --git a/gdb/value.c b/gdb/value.c
index 6620f96..3c2a8da 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3809,7 +3809,7 @@ value_fetch_lazy (struct value *val)
 	     register values should have the register's natural type,
 	     so they do not apply.  */
 	  gdb_assert (!gdbarch_convert_register_p (get_frame_arch (frame),
-						   regnum, type));
+						   regnum, new_val));
 
 	  new_val = get_frame_register_value (frame, regnum);