RFC: Strip the ISA bit when printing symbol offsets

RFC: Strip the ISA bit when printing symbol offsets

[Daniel Jacobowitz]

When you compile a function in ARM's Thumb mode, the low bit of the
function's address is set to indicate the use of an alternate
instruction set.

This currently causes failures in two tests (mi-var-child.exp and
member-ptr.exp), because we print $hex <function+1> instead of the
expected $hex <function>.

What do you think of this patch, which causes us to strip off the low
bit when computing the offset if FUNCTION is a function symbol (rather
than a data variable)?

It's not 100% honest - if the function contains embedded data, for
instance, we might print the altered offset for something that isn't
an instruction.  But it's right more often than what we have now,
since the address including low bit set jumps to the first instruction
of FUNCTION.

Comments?  OK to commit?

Tested on arm-none-eabi and x86_64-linux.

2009-11-13  Daniel Jacobowitz  <dan@codesourcery.com>

	* defs.h (print_address_symbolic, build_address_symbolic): Update
	prototypes.
	* printcmd.c (print_address_symbolic): Take a gdbarch argument.
	Pass it to build_address_symbolic.  All callers updated.
	(build_address_symbolic): Take a gdbarch argument.  Use
	gdbarch_addr_bits_remove for functions.  All callers updated.

---
 gdb/breakpoint.c    |    3 ++-
 gdb/defs.h          |    7 ++++---
 gdb/disasm.c        |    2 +-
 gdb/printcmd.c      |   21 +++++++++++++++------
 gdb/tui/tui-stack.c |    3 ++-
 5 files changed, 24 insertions(+), 12 deletions(-)

Index: gdb-mainline/gdb/breakpoint.c
===================================================================
--- gdb-mainline.orig/gdb/breakpoint.c	2009-11-13 10:57:09.000000000 -0800
+++ gdb-mainline/gdb/breakpoint.c	2009-11-13 10:57:11.000000000 -0800
@@ -3862,7 +3862,8 @@ static void print_breakpoint_location (s
     }
   else
     {
-      print_address_symbolic (loc->address, stb->stream, demangle, "");
+      print_address_symbolic (loc->gdbarch, loc->address, stb->stream,
+			      demangle, "");
       ui_out_field_stream (uiout, "at", stb);
     }
 
Index: gdb-mainline/gdb/defs.h
===================================================================
--- gdb-mainline.orig/gdb/defs.h	2009-11-13 10:57:09.000000000 -0800
+++ gdb-mainline/gdb/defs.h	2009-11-13 10:57:11.000000000 -0800
@@ -596,10 +596,11 @@ extern int info_verbose;
 
 extern void set_next_address (struct gdbarch *, CORE_ADDR);
 
-extern void print_address_symbolic (CORE_ADDR, struct ui_file *, int,
-				    char *);
+extern void print_address_symbolic (struct gdbarch *, CORE_ADDR,
+				    struct ui_file *, int, char *);
 
-extern int build_address_symbolic (CORE_ADDR addr,
+extern int build_address_symbolic (struct gdbarch *,
+				   CORE_ADDR addr,
 				   int do_demangle, 
 				   char **name, 
 				   int *offset, 
Index: gdb-mainline/gdb/disasm.c
===================================================================
--- gdb-mainline.orig/gdb/disasm.c	2009-11-13 10:57:09.000000000 -0800
+++ gdb-mainline/gdb/disasm.c	2009-11-13 10:57:11.000000000 -0800
@@ -116,7 +116,7 @@ dump_insns (struct gdbarch *gdbarch, str
       ui_out_text (uiout, pc_prefix (pc));
       ui_out_field_core_addr (uiout, "address", gdbarch, pc);
 
-      if (!build_address_symbolic (pc, 0, &name, &offset, &filename,
+      if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
 				   &line, &unmapped))
 	{
 	  /* We don't care now about line, filename and
Index: gdb-mainline/gdb/printcmd.c
===================================================================
--- gdb-mainline.orig/gdb/printcmd.c	2009-11-13 10:57:09.000000000 -0800
+++ gdb-mainline/gdb/printcmd.c	2009-11-13 11:01:17.000000000 -0800
@@ -562,7 +562,8 @@ set_next_address (struct gdbarch *gdbarc
    settings of the demangle and asm_demangle variables.  */
 
 void
-print_address_symbolic (CORE_ADDR addr, struct ui_file *stream,
+print_address_symbolic (struct gdbarch *gdbarch, CORE_ADDR addr,
+			struct ui_file *stream,
 			int do_demangle, char *leadin)
 {
   char *name = NULL;
@@ -575,7 +576,7 @@ print_address_symbolic (CORE_ADDR addr, 
   struct cleanup *cleanup_chain = make_cleanup (free_current_contents, &name);
   make_cleanup (free_current_contents, &filename);
 
-  if (build_address_symbolic (addr, do_demangle, &name, &offset,
+  if (build_address_symbolic (gdbarch, addr, do_demangle, &name, &offset,
 			      &filename, &line, &unmapped))
     {
       do_cleanups (cleanup_chain);
@@ -615,7 +616,8 @@ print_address_symbolic (CORE_ADDR addr, 
    success, when all the info in the OUT paramters is valid. Return 1
    otherwise. */
 int
-build_address_symbolic (CORE_ADDR addr,  /* IN */
+build_address_symbolic (struct gdbarch *gdbarch,
+			CORE_ADDR addr,  /* IN */
 			int do_demangle, /* IN */
 			char **name,     /* OUT */
 			int *offset,     /* OUT */
@@ -658,6 +660,13 @@ build_address_symbolic (CORE_ADDR addr, 
 
   if (symbol)
     {
+      /* If this is a function (i.e. a code address), strip out any
+	 non-address bits.  For instance, display a pointer to the
+	 first instruction of a Thumb function as <function>; the
+	 second instruction will be <function+2>, even though the
+	 pointer is <function+3>.  This matches the ISA behavior.  */
+      addr = gdbarch_addr_bits_remove (gdbarch, addr);
+
       name_location = BLOCK_START (SYMBOL_BLOCK_VALUE (symbol));
       if (do_demangle || asm_demangle)
 	name_temp = SYMBOL_PRINT_NAME (symbol);
@@ -722,7 +731,7 @@ print_address (struct gdbarch *gdbarch,
 	       CORE_ADDR addr, struct ui_file *stream)
 {
   fputs_filtered (paddress (gdbarch, addr), stream);
-  print_address_symbolic (addr, stream, asm_demangle, " ");
+  print_address_symbolic (gdbarch, addr, stream, asm_demangle, " ");
 }
 
 /* Return a prefix for instruction address:
@@ -763,11 +772,11 @@ print_address_demangle (struct gdbarch *
   else if (opts.addressprint)
     {
       fputs_filtered (paddress (gdbarch, addr), stream);
-      print_address_symbolic (addr, stream, do_demangle, " ");
+      print_address_symbolic (gdbarch, addr, stream, do_demangle, " ");
     }
   else
     {
-      print_address_symbolic (addr, stream, do_demangle, "");
+      print_address_symbolic (gdbarch, addr, stream, do_demangle, "");
     }
 }
 \f
Index: gdb-mainline/gdb/tui/tui-stack.c
===================================================================
--- gdb-mainline.orig/gdb/tui/tui-stack.c	2009-11-13 10:57:09.000000000 -0800
+++ gdb-mainline/gdb/tui/tui-stack.c	2009-11-13 10:57:11.000000000 -0800
@@ -218,7 +218,8 @@ tui_get_function_from_frame (struct fram
   struct ui_file *stream = tui_sfileopen (256);
   char *p;
 
-  print_address_symbolic (get_frame_pc (fi), stream, demangle, "");
+  print_address_symbolic (get_frame_arch (fi), get_frame_pc (fi),
+			  stream, demangle, "");
   p = tui_file_get_strbuf (stream);
 
   /* Use simple heuristics to isolate the function name.  The symbol

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Strip the ISA bit when printing symbol offsets

[Joel Brobecker]

> 2009-11-13  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* defs.h (print_address_symbolic, build_address_symbolic): Update
> 	prototypes.
> 	* printcmd.c (print_address_symbolic): Take a gdbarch argument.
> 	Pass it to build_address_symbolic.  All callers updated.
> 	(build_address_symbolic): Take a gdbarch argument.  Use
> 	gdbarch_addr_bits_remove for functions.  All callers updated.

I tried to find something wrong with this, but couldn't :).
So, no objection.

-- 
Joel

Re: RFC: Strip the ISA bit when printing symbol offsets

[Eli Zaretskii]

> Date: Fri, 13 Nov 2009 16:27:16 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> When you compile a function in ARM's Thumb mode, the low bit of the
> function's address is set to indicate the use of an alternate
> instruction set.
> 
> This currently causes failures in two tests (mi-var-child.exp and
> member-ptr.exp), because we print $hex <function+1> instead of the
> expected $hex <function>.
> 
> What do you think of this patch, which causes us to strip off the low
> bit when computing the offset if FUNCTION is a function symbol (rather
> than a data variable)?

Isn't it true that an address of a function should always be aligned,
so that its LSB is always zero?  If so, I don't think we need to pass
the architecture pointer to build_address_symbolic, we could just
always reset the LSB.  WDYT?

Re: RFC: Strip the ISA bit when printing symbol offsets

[Daniel Jacobowitz]

On Sat, Nov 14, 2009 at 11:26:42AM +0200, Eli Zaretskii wrote:
> Isn't it true that an address of a function should always be aligned,
> so that its LSB is always zero?  If so, I don't think we need to pass
> the architecture pointer to build_address_symbolic, we could just
> always reset the LSB.  WDYT?

No, that's not true.  On a typical RISC architecture instructions are
more than one byte and require alignment, but on x86_64:

drow@caradoc:~% cat f.c
int a()
{
  return 0;
}
int b()
{
  return 2;
}
drow@caradoc:~% gcc -Os -c f.c
drow@caradoc:~% objdump -dr f.o

f.o:     file format elf64-x86-64


Disassembly of section .text:

0000000000000000 <a>:
   0:   31 c0                   xor    %eax,%eax
   2:   c3                      retq

0000000000000003 <b>:
   3:   b8 02 00 00 00          mov    $0x2,%eax
   8:   c3                      retq

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Strip the ISA bit when printing symbol offsets

[Daniel Jacobowitz]

On Fri, Nov 13, 2009 at 02:24:44PM -0800, Joel Brobecker wrote:
> > 2009-11-13  Daniel Jacobowitz  <dan@codesourcery.com>
> > 
> > 	* defs.h (print_address_symbolic, build_address_symbolic): Update
> > 	prototypes.
> > 	* printcmd.c (print_address_symbolic): Take a gdbarch argument.
> > 	Pass it to build_address_symbolic.  All callers updated.
> > 	(build_address_symbolic): Take a gdbarch argument.  Use
> > 	gdbarch_addr_bits_remove for functions.  All callers updated.
> 
> I tried to find something wrong with this, but couldn't :).
> So, no objection.

Thanks.  I've belatedly committed this.

-- 
Daniel Jacobowitz
CodeSourcery