[rfc] Adjust address size on MIPS

[rfc] Adjust address size on MIPS

[Daniel Jacobowitz]

I recently tried to use a 64-bit stub to debug an O32 MIPS program.
It fell down because GDB would send a "m83000000" packet, instead of
the proper "mffffffff83000000" packet (sign extended 64-bit
addresses).  I think that if a stub sends us some 64-bit registers in
the "g" packet, the polite thing to do would be to send it 64-bit
addresses by default.

Does this sound wrong to anyone?  I can't actually test this at the
moment (I can't run tests on that target and my normal board is still
packed from moving house).  But I'll get around to testing it once I
have the board up again.

-- 
Daniel Jacobowitz
CodeSourcery

2007-08-03  Daniel Jacobowitz  <dan@codesourcery.com>

	* mips-tdep.c (mips_gdbarch_init): Use tdesc_register_size.
	Set gdbarch_addr_bit if known.
	* target-descriptions.c (tdesc_find_register_early): New.
	(tdesc_numbered_register): Use it.
	(tdesc_register_size): New.
	* target-descriptions.h (tdesc_register_size): Declare.

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.440
diff -u -p -r1.440 mips-tdep.c
--- mips-tdep.c	20 Jul 2007 17:29:59 -0000	1.440
+++ mips-tdep.c	3 Aug 2007 17:38:41 -0000
@@ -4848,6 +4848,7 @@ mips_gdbarch_init (struct gdbarch_info i
   int i, num_regs;
   enum mips_fpu_type fpu_type;
   struct tdesc_arch_data *tdesc_data = NULL;
+  int known_wordsize = -1;
 
   /* Check any target description for validity.  */
   if (tdesc_has_registers (info.target_desc))
@@ -4894,6 +4895,8 @@ mips_gdbarch_init (struct gdbarch_info i
 	  return NULL;
 	}
 
+      known_wordsize = tdesc_register_size (feature, "pc") / 8;
+
       feature = tdesc_find_feature (info.target_desc,
 				    "org.gnu.gdb.mips.cp0");
       if (feature == NULL)
@@ -5082,12 +5085,20 @@ mips_gdbarch_init (struct gdbarch_info i
     fprintf_unfiltered (gdb_stdlog,
 			"mips_gdbarch_init: fpu_type = %d\n", fpu_type);
 
+  if (info.target_desc && known_wordsize == -1)
+    {
+      /* Some useful properties can be inferred from the target.  */
+      if (tdesc_property (info.target_desc, PROPERTY_GP32) != NULL)
+	known_wordsize = 4;
+      else if (tdesc_property (info.target_desc, PROPERTY_GP64) != NULL)
+	known_wordsize = 8;
+    }
+
   /* Check for blatant incompatibilities.  */
 
   /* If we have only 32-bit registers, then we can't debug a 64-bit
      ABI.  */
-  if (info.target_desc
-      && tdesc_property (info.target_desc, PROPERTY_GP32) != NULL
+  if (known_wordsize == 4
       && mips_abi != MIPS_ABI_EABI32
       && mips_abi != MIPS_ABI_O32)
     {
@@ -5132,19 +5143,16 @@ mips_gdbarch_init (struct gdbarch_info i
   tdep->register_size_valid_p = 0;
   tdep->register_size = 0;
 
-  if (info.target_desc)
+  if (known_wordsize)
     {
-      /* 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;
-	}
+      tdep->register_size_valid_p = 1;
+      tdep->register_size = known_wordsize;
+
+      /* The address space must be as big as the PC register.  Make
+	 sure we send sufficiently large addresses to the target
+	 system.  If we don't know this, we'll use the size of
+	 a pointer instead.  */
+      set_gdbarch_addr_bit (gdbarch, known_wordsize * 8);
     }
 
   /* Initially set everything according to the default ABI/ISA.  */
Index: target-descriptions.c
===================================================================
RCS file: /cvs/src/src/gdb/target-descriptions.c,v
retrieving revision 1.10
diff -u -p -r1.10 target-descriptions.c
--- target-descriptions.c	3 Jul 2007 01:23:01 -0000	1.10
+++ target-descriptions.c	3 Aug 2007 17:38:42 -0000
@@ -432,10 +432,9 @@ tdesc_data_cleanup (void *data_untyped)
 
 /* Search FEATURE for a register named NAME.  */
 
-int
-tdesc_numbered_register (const struct tdesc_feature *feature,
-			 struct tdesc_arch_data *data,
-			 int regno, const char *name)
+static struct tdesc_reg *
+tdesc_find_register_early (const struct tdesc_feature *feature,
+			   const char *name)
 {
   int ixr;
   struct tdesc_reg *reg;
@@ -444,15 +443,28 @@ tdesc_numbered_register (const struct td
        VEC_iterate (tdesc_reg_p, feature->registers, ixr, reg);
        ixr++)
     if (strcasecmp (reg->name, name) == 0)
-      {
-	/* Make sure the vector includes a REGNO'th element.  */
-	while (regno >= VEC_length (tdesc_reg_p, data->registers))
-	  VEC_safe_push (tdesc_reg_p, data->registers, NULL);
-	VEC_replace (tdesc_reg_p, data->registers, regno, reg);
-	return 1;
-      }
+      return reg;
 
-  return 0;
+  return NULL;
+}
+
+/* Search FEATURE for a register named NAME.  Assign REGNO to it.  */
+
+int
+tdesc_numbered_register (const struct tdesc_feature *feature,
+			 struct tdesc_arch_data *data,
+			 int regno, const char *name)
+{
+  struct tdesc_reg *reg = tdesc_find_register_early (feature, name);
+
+  if (reg == NULL)
+    return 0;
+
+  /* Make sure the vector includes a REGNO'th element.  */
+  while (regno >= VEC_length (tdesc_reg_p, data->registers))
+    VEC_safe_push (tdesc_reg_p, data->registers, NULL);
+  VEC_replace (tdesc_reg_p, data->registers, regno, reg);
+  return 1;
 }
 
 /* Search FEATURE for a register whose name is in NAMES.  */
@@ -471,6 +483,19 @@ tdesc_numbered_register_choices (const s
   return 0;
 }
 
+/* Search FEATURE for a register named NAME, and return its size in
+   bits.  The register must exist.  */
+
+int
+tdesc_register_size (const struct tdesc_feature *feature,
+		     const char *name)
+{
+  struct tdesc_reg *reg = tdesc_find_register_early (feature, name);
+
+  gdb_assert (reg != NULL);
+  return reg->bitsize;
+}
+
 /* Look up a register by its GDB internal register number.  */
 
 static struct tdesc_reg *
Index: target-descriptions.h
===================================================================
RCS file: /cvs/src/src/gdb/target-descriptions.h,v
retrieving revision 1.7
diff -u -p -r1.7 target-descriptions.h
--- target-descriptions.h	13 Jun 2007 18:26:59 -0000	1.7
+++ target-descriptions.h	3 Aug 2007 17:38:42 -0000
@@ -105,6 +105,12 @@ int tdesc_numbered_register_choices (con
 				     struct tdesc_arch_data *data,
 				     int regno, const char *const names[]);
 
+/* Search FEATURE for a register named NAME, and return its size in
+   bits.  The register must exist.  */
+
+int tdesc_register_size (const struct tdesc_feature *feature,
+			 const char *name);
+
 
 /* Accessors for target descriptions.  */
 

Re: [rfc] Adjust address size on MIPS

[Mark Kettenis]

> Date: Fri, 3 Aug 2007 13:58:22 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> I recently tried to use a 64-bit stub to debug an O32 MIPS program.
> It fell down because GDB would send a "m83000000" packet, instead of
> the proper "mffffffff83000000" packet (sign extended 64-bit
> addresses).  I think that if a stub sends us some 64-bit registers in
> the "g" packet, the polite thing to do would be to send it 64-bit
> addresses by default.
> 
> Does this sound wrong to anyone? 

This feels like a bad hack to me.  If it is sending 64-bit addresses
should PROPERTY_GP64 be set in the first place?

Re: [rfc] Adjust address size on MIPS

[Daniel Jacobowitz]

On Fri, Aug 03, 2007 at 08:37:28PM +0200, Mark Kettenis wrote:
> > Date: Fri, 3 Aug 2007 13:58:22 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > I recently tried to use a 64-bit stub to debug an O32 MIPS program.
> > It fell down because GDB would send a "m83000000" packet, instead of
> > the proper "mffffffff83000000" packet (sign extended 64-bit
> > addresses).  I think that if a stub sends us some 64-bit registers in
> > the "g" packet, the polite thing to do would be to send it 64-bit
> > addresses by default.
> > 
> > Does this sound wrong to anyone? 
> 
> This feels like a bad hack to me.  If it is sending 64-bit addresses
> should PROPERTY_GP64 be set in the first place?

Isn't that exactly when it should be set?

Some context - I'm not sure how much of this you're already familiar
with, probably quite a bit.

The remote stub is controlling some program (about which it may know
very little) and has some register/address size (which may be
independent of the program).  This comes about because you can run an
o32 program unmodified in a 64-bit environment.

What's happening to me right now is that the remote stub is 64-bit,
but the program is 32-bit.  GDB sees that registers are 64-bit and
sets the gdbarch appropriately.  But since the program is o32, GDB
knows that pointers are 32-bit, and then uses that as the address
size.  So we set a breakpoint at "0x83000000" based on the address in
the symbol file.

If we were a well-behaved MIPS o32 program using a store instruction,
this would be sign extended behind the scenes and everything would be
happy.  But in the world of the 64-bit stub, you might have 4G RAM at
0 and a different 4G RAM at 0xffffffff00000000.  The stub doesn't know
which one of those we mean, so it takes us at our word and tries
0x83000000.  Which, if you have less than 3GB of RAM, is probably not
mapped as a 64-bit address.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Adjust address size on MIPS

[Mark Kettenis]

> Date: Fri, 3 Aug 2007 14:59:17 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> Mail-Followup-To: Mark Kettenis <mark.kettenis@xs4all.nl>,
> 	gdb-patches@sourceware.org
> Content-Disposition: inline
> X-XS4ALL-DNSBL-Checked: mxdrop6.xs4all.nl checked 208.75.86.248 against DNS blacklists
> X-Virus-Scanned: by XS4ALL Virus Scanner
> X-XS4ALL-Spam-Score: 0.0 () DK_POLICY_SIGNSOME
> X-XS4ALL-Spam: NO
> Envelope-To: mark.kettenis@xs4all.nl
> X-UIDL: 1186167567._smtp.mxdrop6.29859,S=3540
> 
> On Fri, Aug 03, 2007 at 08:37:28PM +0200, Mark Kettenis wrote:
> > > Date: Fri, 3 Aug 2007 13:58:22 -0400
> > > From: Daniel Jacobowitz <drow@false.org>
> > > 
> > > I recently tried to use a 64-bit stub to debug an O32 MIPS program.
> > > It fell down because GDB would send a "m83000000" packet, instead of
> > > the proper "mffffffff83000000" packet (sign extended 64-bit
> > > addresses).  I think that if a stub sends us some 64-bit registers in
> > > the "g" packet, the polite thing to do would be to send it 64-bit
> > > addresses by default.
> > > 
> > > Does this sound wrong to anyone? 
> > 
> > This feels like a bad hack to me.  If it is sending 64-bit addresses
> > should PROPERTY_GP64 be set in the first place?
> 
> Isn't that exactly when it should be set?

Sorry, I meant to write "shouldn't PROPERTY_GP64 have been set in the
first place.

Or differently phrased, if we receive a g packet with 64-bit we set
PROPERTY_GP64 isn't it?  So shouldn't we use that property to set
decide whether we should sign extend the values in m packets?

I guess the fundamental problem here is that there is no consensus
about how gdb should handle running 32-bit mips code on a 64-bit cpu.
Should it present the isa of the executable, or should it present the
isa of the hardware the executable is running on.

Re: [rfc] Adjust address size on MIPS

[Daniel Jacobowitz]

On Fri, Aug 03, 2007 at 09:28:23PM +0200, Mark Kettenis wrote:
> Sorry, I meant to write "shouldn't PROPERTY_GP64 have been set in the
> first place.
> 
> Or differently phrased, if we receive a g packet with 64-bit we set
> PROPERTY_GP64 isn't it?  So shouldn't we use that property to set
> decide whether we should sign extend the values in m packets?

Yes.  That's what this patch (hopefully) will do: if we detect 64-bit
registers, tell the rest of GDB to assume 64-bit addresses.  The
problem comes from the default address size being the pointer size,
and all the attendant MIPS ABI confusion.

> I guess the fundamental problem here is that there is no consensus
> about how gdb should handle running 32-bit mips code on a 64-bit cpu.
> Should it present the isa of the executable, or should it present the
> isa of the hardware the executable is running on.

Amen.  I've tried to improve this lately, but it's still rather a mess.

-- 
Daniel Jacobowitz
CodeSourcery