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

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

[Luis Machado]

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

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

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

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,
 \f
 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);
 

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

[Luis Machado]

On 09/12/2014 05:10 PM, Luis Machado wrote:
> 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

Ping! Any thoughts on the proposed solution?

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

[Pedro Alves]

Hi Luis,

On 09/12/2014 09:10 PM, Luis Machado wrote:

> 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.

If we can't do that, then ...

> 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.

... this leaves me wondering about what you're thinking we'd
do differently if we had infinite time?

> 
> 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?

Well, why not pass down value_bitsize() (an integer) instead of
the whole value?

> 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.

That's a bit vague.  :-)  Got pointers?

Thanks,
Pedro Alves

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

[Luis Machado]

Hi,

On 09/19/2014 02:11 PM, Pedro Alves wrote:
> Hi Luis,
>
> On 09/12/2014 09:10 PM, Luis Machado wrote:
>
>> 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.
>
> If we can't do that, then ...
>
>> 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.
>
> ... this leaves me wondering about what you're thinking we'd
> do differently if we had infinite time?
>

That is a dangerous question! It could create infinite work. :-)

In a quick response, I'd move that data out of the value struct and put 
it in the type struct, but that would require additional changes to 
prevent sharing the base types that have bit field data in them.

For a longer answer, i think for everyday normal use GDB's current type 
mechanism works OK, but if you go beyond that and start doing alternate 
named address spaces, different kinds of pointers and strange type 
castings you start to reach the limits of the assumptions GDB makes (or 
has made in the past). There is definitely room for improvement in the 
type system.

The difficulty to change this is because the type mechanism is a bit 
entangled with the symbol machinery and the DWARF reader, with potential 
for breakage.

I haven't dealt much with C++ types, but from what I've dealt with, it 
seems we have some difficulties in there too. Again, it works, but could 
be better.

Anyway, i think this has potential for a bigger and interesting discussion.

>>
>> 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?
>
> Well, why not pass down value_bitsize() (an integer) instead of
> the whole value?
>

I thought about doing that, but seems weird to have a function parameter 
just for the bit field information. I'm not sure if we're not missing 
other bits of information that may be useful for register/value 
conversion, that's why i went with passing the value structure. It holds 
everything anyway, not just the bit field data.

Passing the bit size is OK with me though.

>> 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.
>
> That's a bit vague.  :-)  Got pointers?
>

I meant the functions that have to act on the type that is passed as a 
parameter rather than the value itself, like the following:

gdbarch_register_to_value
gdbarch_value_to_register
gdbarch_value_from_register

The dummy call hook already acts on the value structs of the parameters, 
so it doesn't sound totally wrong to want to pass the value struct in 
this case too.

Luis

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

[Luis Machado]

On 09/19/2014 02:39 PM, Luis Machado wrote:
> Hi,
>
> On 09/19/2014 02:11 PM, Pedro Alves wrote:
>> Hi Luis,
>>
>> On 09/12/2014 09:10 PM, Luis Machado wrote:
>>
>>> 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.
>>
>> If we can't do that, then ...
>>
>>> 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.
>>
>> ... this leaves me wondering about what you're thinking we'd
>> do differently if we had infinite time?
>>
>
> That is a dangerous question! It could create infinite work. :-)
>
> In a quick response, I'd move that data out of the value struct and put
> it in the type struct, but that would require additional changes to
> prevent sharing the base types that have bit field data in them.
>
> For a longer answer, i think for everyday normal use GDB's current type
> mechanism works OK, but if you go beyond that and start doing alternate
> named address spaces, different kinds of pointers and strange type
> castings you start to reach the limits of the assumptions GDB makes (or
> has made in the past). There is definitely room for improvement in the
> type system.
>
> The difficulty to change this is because the type mechanism is a bit
> entangled with the symbol machinery and the DWARF reader, with potential
> for breakage.
>
> I haven't dealt much with C++ types, but from what I've dealt with, it
> seems we have some difficulties in there too. Again, it works, but could
> be better.
>
> Anyway, i think this has potential for a bigger and interesting discussion.
>
>>>
>>> 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?
>>
>> Well, why not pass down value_bitsize() (an integer) instead of
>> the whole value?
>>
>
> I thought about doing that, but seems weird to have a function parameter
> just for the bit field information. I'm not sure if we're not missing
> other bits of information that may be useful for register/value
> conversion, that's why i went with passing the value structure. It holds
> everything anyway, not just the bit field data.
>
> Passing the bit size is OK with me though.
>
>>> 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.
>>
>> That's a bit vague.  :-)  Got pointers?
>>
>
> I meant the functions that have to act on the type that is passed as a
> parameter rather than the value itself, like the following:
>
> gdbarch_register_to_value
> gdbarch_value_to_register
> gdbarch_value_from_register
>
> The dummy call hook already acts on the value structs of the parameters,
> so it doesn't sound totally wrong to want to pass the value struct in
> this case too.

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)?

Thanks,
Luis

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

[Pedro Alves]

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 ();
               }
          }

Thanks,
Pedro Alves

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

[Luis Machado]

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. 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 or do you want 
to author it?

Thanks,
Luis

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

[Pedro Alves]

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

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

[Luis Machado]

[-- 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
 	      {

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

[Pedro Alves]

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

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

[Luis Machado]

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.