Re: [rfc] Auto-detect MIPS 32-bit and 64-bit register packets

[Daniel Jacobowitz <drow at false dot org>]

On Thu, Nov 09, 2006 at 04:08:38PM -0500, Daniel Jacobowitz wrote:
> This is even more useful than the amd64 patch.  If you have an i386 or amd64
> ELF file, it's obvious which register layout to expect.  If you have a MIPS
> binary, it may be considerably less clear.  For one thing, mips_isa_regsize
> is currently set to return 64-bit for binaries based on the BFD architecture
> rather than the ELF class; so o32 binaries optimized for a 64-bit processor
> cause GDB to expect 64-bit registers!  For another, I have a gdbserver port
> for MIPS64 coming up next; if GDB can handle receiving the 64-bit register
> layout from it, then a single gdbserver can be used (in most cases) for
> all three MIPS ABIs.

Accordingly, though I've dropped the amd64 patch, I've committed this
one.  I'll follow up with a NEWS entry for this and mips64 gdbserver
in a few minutes.

There are only two changes from the last version: some common bits
which were formerly part of the i386/amd64 patch are now part of this
one, and there is now a more sensible warning if you load an n32 binary
and connect to an o32 debugger, instead of an internal error.

-- 
Daniel Jacobowitz
CodeSourcery

2006-11-09  Daniel Jacobowitz  <dan@codesourcery.com>

	* Makefile.in (mips-tdep.o, target-descriptions.o): Update.
	* target-descriptions.c (struct property): New.
	(struct target_desc): Add properties member.
	(tdesc_property, set_tdesc_property): New.
	* target-descriptions.h (tdesc_property, set_tdesc_property):
	Declare.
	* mips-tdep.c (PROPERTY_GP32, PROPERTY_GP64): New constants.
	(struct gdbarch_tdep): Add register_size_valid_p and register_size.
	(mips_isa_regsize): Use them.
	(mips_register_g_packet_guesses): New.
	(mips_gdbarch_init): Call it.  If a target description is supplied,
	check for internal properties.

---
 gdb/Makefile.in           |    4 +-
 gdb/mips-tdep.c           |   71 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/target-descriptions.c |   48 +++++++++++++++++++++++++++++++
 gdb/target-descriptions.h |    8 +++++
 4 files changed, 129 insertions(+), 2 deletions(-)

Index: src/gdb/Makefile.in
===================================================================
--- src.orig/gdb/Makefile.in	2006-11-09 10:41:32.000000000 -0500
+++ src/gdb/Makefile.in	2006-11-09 10:42:40.000000000 -0500
@@ -2377,7 +2377,7 @@ mips-tdep.o: mips-tdep.c $(defs_h) $(gdb
 	$(block_h) $(reggroups_h) $(opcode_mips_h) $(elf_mips_h) \
 	$(elf_bfd_h) $(symcat_h) $(sim_regno_h) $(dis_asm_h) \
 	$(frame_unwind_h) $(frame_base_h) $(trad_frame_h) $(infcall_h) \
-	$(floatformat_h)
+	$(floatformat_h) $(remote_h) $(target_descriptions_h)
 mipsv4-nat.o: mipsv4-nat.c $(defs_h) $(inferior_h) $(gdbcore_h) $(target_h) \
 	$(regcache_h) $(gregset_h)
 memory-map.o: memory-map.c $(defs_h) $(memory_map_h) $(xml_support_h) \
@@ -2765,7 +2765,7 @@ target.o: target.c $(defs_h) $(gdb_strin
 	$(gdb_wait_h) $(dcache_h) $(regcache_h) $(gdb_assert_h) $(gdbcore_h) \
 	$(exceptions_h) $(target_descriptions_h)
 target-descriptions.o: target-descriptions.c $(defs_h) $(arch_utils_h) \
-	$(target_h) $(target_descriptions_h) $(gdb_assert_h)
+	$(target_h) $(target_descriptions_h) $(vec_h) $(gdb_assert_h)
 target-memory.o: target-memory.c $(defs_h) $(vec_h) $(target_h) \
 	$(memory_map_h) $(gdb_assert_h)
 thread.o: thread.c $(defs_h) $(symtab_h) $(frame_h) $(inferior_h) \
Index: src/gdb/target-descriptions.c
===================================================================
--- src.orig/gdb/target-descriptions.c	2006-11-09 10:41:24.000000000 -0500
+++ src/gdb/target-descriptions.c	2006-11-09 10:41:46.000000000 -0500
@@ -26,15 +26,26 @@
 #include "arch-utils.h"
 #include "target.h"
 #include "target-descriptions.h"
+#include "vec.h"
 
 #include "gdb_assert.h"
 
 /* Types.  */
 
+typedef struct property
+{
+  const char *key;
+  const char *value;
+} property_s;
+DEF_VEC_O(property_s);
+
 struct target_desc
 {
   /* The architecture reported by the target, if any.  */
   const struct bfd_arch_info *arch;
+
+  /* Any architecture-specific properties specified by the target.  */
+  VEC(property_s) *properties;
 };
 
 /* Global state.  These variables are associated with the current
@@ -133,6 +144,23 @@ tdesc_architecture (const struct target_
   return target_desc->arch;
 }
 
+/* Return the string value of a property named KEY, or NULL if the
+   property was not specified.  */
+
+const char *
+tdesc_property (const struct target_desc *target_desc, const char *key)
+{
+  struct property *prop;
+  int ix;
+
+  for (ix = 0; VEC_iterate (property_s, target_desc->properties, ix, prop);
+       ix++)
+    if (strcmp (prop->key, key) == 0)
+      return prop->value;
+
+  return NULL;
+}
+
 /* Methods for constructing a target description.  */
 
 struct target_desc *
@@ -147,3 +175,23 @@ set_tdesc_architecture (struct target_de
 {
   target_desc->arch = arch;
 }
+
+void
+set_tdesc_property (struct target_desc *target_desc,
+		    const char *key, const char *value)
+{
+  struct property *prop, new_prop;
+  int ix;
+
+  gdb_assert (key != NULL && value != NULL);
+
+  for (ix = 0; VEC_iterate (property_s, target_desc->properties, ix, prop);
+       ix++)
+    if (strcmp (prop->key, key) == 0)
+      internal_error (__FILE__, __LINE__,
+		      _("Attempted to add duplicate property \"%s\""), key);
+
+  new_prop.key = key;
+  new_prop.value = value;
+  VEC_safe_push (property_s, target_desc->properties, &new_prop);
+}
Index: src/gdb/target-descriptions.h
===================================================================
--- src.orig/gdb/target-descriptions.h	2006-11-09 10:41:24.000000000 -0500
+++ src/gdb/target-descriptions.h	2006-11-09 10:41:46.000000000 -0500
@@ -51,10 +51,18 @@ const struct target_desc *target_current
 const struct bfd_arch_info *tdesc_architecture
   (const struct target_desc *);
 
+/* Return the string value of a property named KEY, or NULL if the
+   property was not specified.  */
+
+const char *tdesc_property (const struct target_desc *,
+			    const char *key);
+
 /* Methods for constructing a target description.  */
 
 struct target_desc *allocate_target_description (void);
 void set_tdesc_architecture (struct target_desc *,
 			     const struct bfd_arch_info *);
+void set_tdesc_property (struct target_desc *,
+			 const char *key, const char *value);
 
 #endif /* TARGET_DESCRIPTIONS_H */
Index: src/gdb/mips-tdep.c
===================================================================
--- src.orig/gdb/mips-tdep.c	2006-11-09 10:34:47.000000000 -0500
+++ src/gdb/mips-tdep.c	2006-11-09 10:41:46.000000000 -0500
@@ -55,6 +55,8 @@
 #include "trad-frame.h"
 #include "infcall.h"
 #include "floatformat.h"
+#include "remote.h"
+#include "target-descriptions.h"
 
 static const struct objfile_data *mips_pdr_data;
 
@@ -119,6 +121,11 @@ static enum mips_fpu_type mips_fpu_type 
 
 static int mips_debug = 0;
 
+/* Properties (for struct target_desc) describing the g/G packet
+   layout.  */
+#define PROPERTY_GP32 "internal: transfers-32bit-registers"
+#define PROPERTY_GP64 "internal: transfers-64bit-registers"
+
 /* MIPS specific per-architecture information */
 struct gdbarch_tdep
 {
@@ -141,6 +148,13 @@ struct gdbarch_tdep
   const struct mips_regnum *regnum;
   /* Register names table for the current register set.  */
   const char **mips_processor_reg_names;
+
+  /* The size of register data available from the target, if known.
+     This doesn't quite obsolete the manual
+     mips64_transfers_32bit_regs_p, since that is documented to force
+     left alignment even for big endian (very strange).  */
+  int register_size_valid_p;
+  int register_size;
 };
 
 static int
@@ -245,6 +259,13 @@ mips_abi (struct gdbarch *gdbarch)
 int
 mips_isa_regsize (struct gdbarch *gdbarch)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  /* If we know how big the registers are, use that size.  */
+  if (tdep->register_size_valid_p)
+    return tdep->register_size;
+
+  /* Fall back to the previous behavior.  */
   return (gdbarch_bfd_arch_info (gdbarch)->bits_per_word
 	  / gdbarch_bfd_arch_info (gdbarch)->bits_per_byte);
 }
@@ -4711,6 +4732,37 @@ global_mips_abi (void)
   internal_error (__FILE__, __LINE__, _("unknown ABI string"));
 }
 
+static void
+mips_register_g_packet_guesses (struct gdbarch *gdbarch)
+{
+  static struct target_desc *tdesc_gp32, *tdesc_gp64;
+
+  if (tdesc_gp32 == NULL)
+    {
+      /* Create feature sets with the appropriate properties.  The values
+	 are not important.  */
+
+      tdesc_gp32 = allocate_target_description ();
+      set_tdesc_property (tdesc_gp32, PROPERTY_GP32, "");
+
+      tdesc_gp64 = allocate_target_description ();
+      set_tdesc_property (tdesc_gp64, PROPERTY_GP64, "");
+    }
+
+  /* If the size matches the set of 32-bit or 64-bit integer registers,
+     assume that's what we've got.  */
+  register_remote_g_packet_guess (gdbarch, 38 * 4, tdesc_gp32);
+  register_remote_g_packet_guess (gdbarch, 38 * 8, tdesc_gp64);
+
+  /* If the size matches the full set of registers GDB traditionally
+     knows about, including floating point, for either 32-bit or
+     64-bit, assume that's what we've got.  */
+  register_remote_g_packet_guess (gdbarch, 90 * 4, tdesc_gp32);
+  register_remote_g_packet_guess (gdbarch, 90 * 8, tdesc_gp64);
+
+  /* Otherwise we don't have a useful guess.  */
+}
+
 static struct gdbarch *
 mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 {
@@ -4885,6 +4937,23 @@ mips_gdbarch_init (struct gdbarch_info i
   tdep->found_abi = found_abi;
   tdep->mips_abi = mips_abi;
   tdep->mips_fpu_type = fpu_type;
+  tdep->register_size_valid_p = 0;
+  tdep->register_size = 0;
+
+  if (info.target_desc)
+    {
+      /* Some useful properties can be inferred from the target.  */
+      if (tdesc_property (info.target_desc, PROPERTY_GP32) != NULL)
+	{
+	  tdep->register_size_valid_p = 1;
+	  tdep->register_size = 4;
+	}
+      else if (tdesc_property (info.target_desc, PROPERTY_GP64) != NULL)
+	{
+	  tdep->register_size_valid_p = 1;
+	  tdep->register_size = 8;
+	}
+    }
 
   /* Initially set everything according to the default ABI/ISA.  */
   set_gdbarch_short_bit (gdbarch, 16);
@@ -5152,6 +5221,8 @@ mips_gdbarch_init (struct gdbarch_info i
 
   set_gdbarch_single_step_through_delay (gdbarch, mips_single_step_through_delay);
 
+  mips_register_g_packet_guesses (gdbarch);
+
   /* Hook in OS ABI-specific overrides, if they have been registered.  */
   gdbarch_init_osabi (info, gdbarch);