utils.c: Sign-extend addresses if required by the target

utils.c: Sign-extend addresses if required by the target

[Maciej W. Rozycki]

Hello,

 There is at least one architecture, which is the MIPS one, that expands 
addresses from 32-bits to 64-bits by doing sign-extension.  It matters for 
32-bit compatibility kernel addresses, which are defined with bits 31:63 
set to one.  For these addresses, if the compatibility mode is used, it is 
common to input them as 32-bit hexadecimal constants as it would be the 
case with 32-bit processors.  However, BFD for the MIPS target is always 
64-bit and if the processor used is indeed 64-bit, these addresses have to 
be properly sign-extended to 64 bits.

 Here is probably one of the last remaining places where it is not done.  
The string_to_core_addr() is only used by the MI and Insight frontends and 
then kernel addresses are only seen on bare-iron targets (e.g. not under 
Linux, where user addresses are used only), so the problem may have 
escaped for that long.

 There is a note by Nigel included that perhaps this should be done 
through the architecture vector.  Well, I am not sure it is useful to add 
such a member there just for this single place, but perhaps it would be a 
good idea to clean up all the places it is done as a whole.  It fixes a 
regression though, so I think it is a useful fix, and the clean-up could 
be done separately, so the note should probably be kept.

 Tested for using the mipsisa32-sde-elf target, with the 
mips-sim-sde32/-EB, mips-sim-sde32/-EL, mips-sim-sde64/-mips64/-EB and 
mips-sim-sde64/-mips64/-EL boards removing 2 regressions -- this one:

-var-create a2 0x807fffa8 a
^done,name="a2",numchild="0",value="2 '\\002'",type="char"
(gdb)
FAIL: gdb.mi/mi-var-display.exp: create variable a2 in different scope

and similarly for gdb.mi/mi2-var-display.exp.

2007-11-22  Nigel Stephens  <nigel@mips.com>
            Maciej W. Rozycki  <macro@mips.com>

	* utils.c (string_to_core_addr): If executable format indicates
	that addresses should be sign-extended and there are only 8 hex
	digits in the address, then do so.
	* Makefile.in (utils.o): Depend on $(gdbcore_h).

 OK to apply?

  Maciej

14524.diff
Index: binutils-quilt/src/gdb/utils.c
===================================================================
--- binutils-quilt.orig/src/gdb/utils.c	2007-11-21 11:25:32.000000000 +0000
+++ binutils-quilt/src/gdb/utils.c	2007-11-22 14:08:27.000000000 +0000
@@ -52,6 +52,7 @@
 #include "filenames.h"
 #include "symfile.h"
 #include "gdb_obstack.h"
+#include "gdbcore.h"
 #include "top.h"
 
 #include "inferior.h"		/* for signed_pointer_to_address */
@@ -2824,7 +2825,9 @@
 CORE_ADDR
 string_to_core_addr (const char *my_string)
 {
+  int addr_bit = gdbarch_addr_bit (current_gdbarch);
   CORE_ADDR addr = 0;
+
   if (my_string[0] == '0' && tolower (my_string[1]) == 'x')
     {
       /* Assume that it is in hex.  */
@@ -2838,6 +2841,17 @@
 	  else
 	    error (_("invalid hex \"%s\""), my_string);
 	}
+
+      /* Not very modular, but if the executable format expects
+         addresses to be sign-extended, then do so if the address was
+         specified with only 32 significant bits.  Really this should
+         be determined by the target architecture, not by the object
+         file.  */
+      if (i - 2 == addr_bit / 4
+	  && exec_bfd
+	  && bfd_get_sign_extend_vma (exec_bfd))
+	addr = (addr ^ ((CORE_ADDR) 1 << (addr_bit - 1)))
+	       - ((CORE_ADDR) 1 << (addr_bit - 1));
     }
   else
     {
@@ -2851,6 +2865,7 @@
 	    error (_("invalid decimal \"%s\""), my_string);
 	}
     }
+
   return addr;
 }
 
Index: binutils-quilt/src/gdb/Makefile.in
===================================================================
--- binutils-quilt.orig/src/gdb/Makefile.in	2007-11-21 11:34:19.000000000 +0000
+++ binutils-quilt/src/gdb/Makefile.in	2007-11-22 14:01:00.000000000 +0000
@@ -2892,7 +2892,7 @@
 	$(exceptions_h) $(tui_h) $(gdbcmd_h) $(serial_h) $(bfd_h) \
 	$(target_h) $(demangle_h) $(expression_h) $(language_h) $(charset_h) \
 	$(annotate_h) $(filenames_h) $(symfile_h) $(inferior_h) $(top_h) \
-	$(gdb_curses_h) $(readline_h) $(gdb_obstack_h)
+	$(gdb_curses_h) $(readline_h) $(gdb_obstack_h) $(gdbcore_h)
 v850-tdep.o: v850-tdep.c $(defs_h) $(frame_h) $(frame_base_h) $(trad_frame_h) \
 	$(frame_unwind_h) $(dwarf2_frame_h) $(gdbtypes_h) $(inferior_h) \
 	$(gdb_string_h) $(gdb_assert_h) $(gdbcore_h) $(arch_utils_h) \

Re: utils.c: Sign-extend addresses if required by the target

[Daniel Jacobowitz]

On Thu, Nov 22, 2007 at 04:25:18PM +0000, Maciej W. Rozycki wrote:
> 2007-11-22  Nigel Stephens  <nigel@mips.com>
>             Maciej W. Rozycki  <macro@mips.com>
> 
> 	* utils.c (string_to_core_addr): If executable format indicates
> 	that addresses should be sign-extended and there are only 8 hex
> 	digits in the address, then do so.
> 	* Makefile.in (utils.o): Depend on $(gdbcore_h).
> 
>  OK to apply?

OK.  This routine is used by MI, but should not be - a long term TODO
item is to change the way we present frame IDs to MI frontends to not
be based on the stack pointer, and then its use will go away.  But
that's not going to happen too soon.


-- 
Daniel Jacobowitz
CodeSourcery

Re: utils.c: Sign-extend addresses if required by the target

[Maciej W. Rozycki]

On Sun, 16 Dec 2007, Daniel Jacobowitz wrote:

> OK.  This routine is used by MI, but should not be - a long term TODO
> item is to change the way we present frame IDs to MI frontends to not
> be based on the stack pointer, and then its use will go away.  But
> that's not going to happen too soon.

 What about Insight?  Not that I would die for it, but the function is 
called from a few places in gdbtk/generic/gdbtk-{cmds,stack,varobj}.c and 
given the amount of development done on Insight these days I would imagine 
these uses are not going to go away anytime soon.

 Thanks for the review.

  Maciej

Re: utils.c: Sign-extend addresses if required by the target

[Daniel Jacobowitz]

On Mon, Dec 17, 2007 at 05:37:44PM +0000, Maciej W. Rozycki wrote:
> On Sun, 16 Dec 2007, Daniel Jacobowitz wrote:
> 
> > OK.  This routine is used by MI, but should not be - a long term TODO
> > item is to change the way we present frame IDs to MI frontends to not
> > be based on the stack pointer, and then its use will go away.  But
> > that's not going to happen too soon.
> 
>  What about Insight?  Not that I would die for it, but the function is 
> called from a few places in gdbtk/generic/gdbtk-{cmds,stack,varobj}.c and 
> given the amount of development done on Insight these days I would imagine 
> these uses are not going to go away anytime soon.

Right.  I'm not going to touch it; I don't know what Insight uses it
for, or whether it should.  But it's probably using it for the same
reason MI is, since Insight predates varobjs.

I try not to break Insight knowingly, but if we go to clean up MI
frame identification and that breaks Insight, I'll leave it to the
Insight developers to clean up.  That's the downside of being so
tightly tied into GDB.

-- 
Daniel Jacobowitz
CodeSourcery

Re: utils.c: Sign-extend addresses if required by the target

[Maciej W. Rozycki]

On Mon, 17 Dec 2007, Daniel Jacobowitz wrote:

> I try not to break Insight knowingly, but if we go to clean up MI
> frame identification and that breaks Insight, I'll leave it to the
> Insight developers to clean up.  That's the downside of being so
> tightly tied into GDB.

 OK, thanks for this clarification.  I have applied this change thus.

  Maciej