Broken cast in linux-thread-db

Broken cast in linux-thread-db

[Andreas Schwab]

Casting a pointer to CORE_ADDR results in implementation defined behaviour
when the latter is wider than a pointer.  The implementation's behaviour
might be to sign extend which is not what we want here.  Tested on
i386-suse-linux with --enable-64-bit-bfd.

Andreas.

2005-10-12  Andreas Schwab  <schwab@suse.de>

	* linux-thread-db.c (enable_thread_event): Cast pointer to
	uintptr_t to avoid implementation defined behaviour.
	(thread_db_get_thread_local_address): Likewise.

--- gdb/linux-thread-db.c.~1.10.~	2005-09-12 11:04:57.000000000 +0200
+++ gdb/linux-thread-db.c	2005-10-12 15:16:06.000000000 +0200
@@ -505,9 +505,13 @@ enable_thread_event (td_thragent_t *thre
     return err;
 
   /* Set up the breakpoint.  */
-  (*bp) = gdbarch_convert_from_func_ptr_addr (current_gdbarch,
-					      (CORE_ADDR) notify.u.bptaddr,
-					      &current_target);
+  (*bp) = (gdbarch_convert_from_func_ptr_addr
+	   (current_gdbarch,
+	    /* Don't cast directly to CORE_ADDR, which may be wider than a
+	       pointer and results in implementation defined
+	       behaviour.  */
+	    (uintptr_t) notify.u.bptaddr,
+	    &current_target));
   create_thread_event_breakpoint ((*bp));
 
   return TD_OK;
@@ -1277,7 +1281,10 @@ thread_db_get_thread_local_address (ptid
                      (("%s")), thread_db_err_str (err));
 
       /* Cast assuming host == target.  Joy.  */
-      return (CORE_ADDR) address;
+      /* Don't cast directly to CORE_ADDR, which may be wider than a
+	 pointer and results in implementation defined behaviour.  GCC
+	 would sign extend the value which is not what we want here.  */
+      return (uintptr_t) address;
     }
 
   if (target_beneath->to_get_thread_local_address)

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Broken cast in linux-thread-db

[Daniel Jacobowitz]

On Wed, Oct 12, 2005 at 04:13:11PM +0200, Andreas Schwab wrote:
> Casting a pointer to CORE_ADDR results in implementation defined behaviour
> when the latter is wider than a pointer.  The implementation's behaviour
> might be to sign extend which is not what we want here.  Tested on
> i386-suse-linux with --enable-64-bit-bfd.

This will silence the warning, but AFAICS not fix anything.  Now you're
casting it to something explicitly unsigned, but the same size as a
pointer; but GDB tries to use the target's sign extension conventions,
and a CORE_ADDR ought to be properly sign extended on MIPS.

I think the current behavior is actually more correct despite any
warning.  Were you fixing an observed problem?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Broken cast in linux-thread-db

[Andreas Schwab]

Daniel Jacobowitz <drow@false.org> writes:

> I think the current behavior is actually more correct despite any
> warning.

GCC _always_ sign extends, which is definitely wrong here.

>  Were you fixing an observed problem?

If notify.u.bptaddr has the high bit set it will be sign extended, but the
breakpoint address when the thread event breakpoint is hit will be zero
extended, so they don't match and the breakpoint is not recognized.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Broken cast in linux-thread-db

[Daniel Jacobowitz]

On Wed, Oct 12, 2005 at 04:33:27PM +0200, Andreas Schwab wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> 
> > I think the current behavior is actually more correct despite any
> > warning.
> 
> GCC _always_ sign extends, which is definitely wrong here.

Well presumably it honors POINTERS_EXTEND_UNSIGNED.

> >  Were you fixing an observed problem?
> 
> If notify.u.bptaddr has the high bit set it will be sign extended, but the
> breakpoint address when the thread event breakpoint is hit will be zero
> extended, so they don't match and the breakpoint is not recognized.

Then this will break thread debugging on MIPS, where the breakpoint
address will be sign extended.  We'll only be in linux-thread-db.c if
we have enough symbols to load shared libraries; maybe
bfd_get_sign_extend_vma (exec_bfd) and assert that exec_bfd is
provided?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Broken cast in linux-thread-db

[Andreas Schwab]

Daniel Jacobowitz <drow@false.org> writes:

> Well presumably it honors POINTERS_EXTEND_UNSIGNED.

No.

http://gcc.gnu.org/onlinedocs/gcc/Arrays-and-pointers-implementation.html#Arrays-and-pointers-implementation

>> If notify.u.bptaddr has the high bit set it will be sign extended, but the
>> breakpoint address when the thread event breakpoint is hit will be zero
>> extended, so they don't match and the breakpoint is not recognized.
>
> Then this will break thread debugging on MIPS, where the breakpoint
> address will be sign extended.

Note that CORE_ADDR is always unsigned, even on MIPS.

> We'll only be in linux-thread-db.c if we have enough symbols to load
> shared libraries; maybe bfd_get_sign_extend_vma (exec_bfd) and assert
> that exec_bfd is provided?

Like this?

2005-10-12  Andreas Schwab  <schwab@suse.de>

	* Makefile.in (linux-thread-db.o): Depend on $(gdbcore_h).

	* linux-thread-db.c (enable_thread_event): Extend pointer value as
	specified by target.
	(thread_db_get_thread_local_address): Likewise.

Index: gdb/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.755
diff -u -a -p -u -p -a -r1.755 gdb/Makefile.in
--- gdb/Makefile.in	28 Sep 2005 02:55:41 -0000	1.755
+++ gdb/Makefile.in	12 Oct 2005 15:11:48 -0000
@@ -2182,7 +2182,7 @@ linux-nat.o: linux-nat.c $(defs_h) $(inf
 linux-thread-db.o: linux-thread-db.c $(defs_h) $(gdb_assert_h) \
 	$(gdb_proc_service_h) $(gdb_thread_db_h) $(bfd_h) $(exceptions_h) \
 	$(gdbthread_h) $(inferior_h) $(symfile_h) $(objfiles_h) $(target_h) \
-	$(regcache_h) $(solib_svr4_h)
+	$(regcache_h) $(solib_svr4_h) $(gdbcore_h)
 lynx-nat.o: lynx-nat.c $(defs_h) $(frame_h) $(inferior_h) $(target_h) \
 	$(gdbcore_h) $(regcache_h)
 m2-exp.o: m2-exp.c $(defs_h) $(gdb_string_h) $(expression_h) $(language_h) \
--- gdb/linux-thread-db.c	12 Sep 2005 11:04:57 +0200	1.10
+++ gdb/linux-thread-db.c	12 Okt 2005 15:11:31 +0200	
@@ -36,6 +36,7 @@
 #include "target.h"
 #include "regcache.h"
 #include "solib-svr4.h"
+#include "gdbcore.h"
 
 #ifdef HAVE_GNU_LIBC_VERSION_H
 #include <gnu/libc-version.h>
@@ -505,9 +506,14 @@ enable_thread_event (td_thragent_t *thre
     return err;
 
   /* Set up the breakpoint.  */
-  (*bp) = gdbarch_convert_from_func_ptr_addr (current_gdbarch,
-					      (CORE_ADDR) notify.u.bptaddr,
-					      &current_target);
+  gdb_assert (exec_bfd);
+  (*bp) = (gdbarch_convert_from_func_ptr_addr
+	   (current_gdbarch,
+	    /* Do proper sign extension for the target.  */
+	    (bfd_get_sign_extend_vma (exec_bfd) > 0
+	     ? (CORE_ADDR) (intptr_t) notify.u.bptaddr
+	     : (CORE_ADDR) (uintptr_t) notify.u.bptaddr),
+	    &current_target));
   create_thread_event_breakpoint ((*bp));
 
   return TD_OK;
@@ -1277,7 +1283,11 @@ thread_db_get_thread_local_address (ptid
                      (("%s")), thread_db_err_str (err));
 
       /* Cast assuming host == target.  Joy.  */
-      return (CORE_ADDR) address;
+      /* Do proper sign extension for the target.  */
+      gdb_assert (exec_bfd);
+      return (bfd_get_sign_extend_vma (exec_bfd) > 0
+	      ? (CORE_ADDR) (intptr_t) address
+	      : (CORE_ADDR) (uintptr_t) address);
     }
 
   if (target_beneath->to_get_thread_local_address)

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Broken cast in linux-thread-db

[Daniel Jacobowitz]

On Wed, Oct 12, 2005 at 05:59:21PM +0200, Andreas Schwab wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> 
> > Well presumably it honors POINTERS_EXTEND_UNSIGNED.
> 
> No.
> 
> http://gcc.gnu.org/onlinedocs/gcc/Arrays-and-pointers-implementation.html#Arrays-and-pointers-implementation

Huh, fascinating.

> >> If notify.u.bptaddr has the high bit set it will be sign extended, but the
> >> breakpoint address when the thread event breakpoint is hit will be zero
> >> extended, so they don't match and the breakpoint is not recognized.
> >
> > Then this will break thread debugging on MIPS, where the breakpoint
> > address will be sign extended.
> 
> Note that CORE_ADDR is always unsigned, even on MIPS.

Yes.  But we don't treat it that way; it contains sign extended values.

> > We'll only be in linux-thread-db.c if we have enough symbols to load
> > shared libraries; maybe bfd_get_sign_extend_vma (exec_bfd) and assert
> > that exec_bfd is provided?
> 
> Like this?
> 
> 2005-10-12  Andreas Schwab  <schwab@suse.de>
> 
> 	* Makefile.in (linux-thread-db.o): Depend on $(gdbcore_h).
> 
> 	* linux-thread-db.c (enable_thread_event): Extend pointer value as
> 	specified by target.
> 	(thread_db_get_thread_local_address): Likewise.

Yes, precisely.  This is OK.

[I'm assuming no one's going to try to build a native gdb on a Linux
system predating intptr_t.  Probably a good assumption.]

-- 
Daniel Jacobowitz
CodeSourcery, LLC