diff mbox

[03/15,v2] Make gdbserver CORE_ADDR unsigned

Message ID 1405520243-17282-4-git-send-email-gbenson@redhat.com
State Changes Requested, archived
Headers

Commit Message

Gary Benson July 16, 2014, 2:17 p.m. UTC 
  gdbserver defines CORE_ADDR to be signed.  This seems erroneous to
me; and furthermore likely to cause problems in common/, as it is
different from gdb's definition.

gdb/gdbserver/
2014-07-16  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* server.h (CORE_ADDR): Now unsigned.
---
 gdb/gdbserver/ChangeLog |    5 +++++
 gdb/gdbserver/server.h  |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

Comments

Doug Evans July 17, 2014, 8:37 a.m. UTC | #1 
On Wed, Jul 16, 2014 at 7:17 AM, Gary Benson <gbenson@redhat.com> wrote:
> gdbserver defines CORE_ADDR to be signed.  This seems erroneous to
> me; and furthermore likely to cause problems in common/, as it is
> different from gdb's definition.
>
> gdb/gdbserver/
> 2014-07-16  Tom Tromey  <tromey@redhat.com>
>             Gary Benson  <gbenson@redhat.com>
>
>         * server.h (CORE_ADDR): Now unsigned.

LGTM
Pedro Alves July 17, 2014, 4:41 p.m. UTC | #2 
The only issue that I think might come out of this is
that on MIPS, addresses are signed, so pointers are sign
extended.  But we can definitely handle whatever fallout this
may cause, if any, when we see it.  Clearly if GDB's native
targets can handle that, so should gdbserver's, and in any
case the issue should be pretty localized.  Just pointing it
out FYI, to keep an eye out for it.
Maciej W. Rozycki July 17, 2014, 11:10 p.m. UTC | #3 
On Thu, 17 Jul 2014, Pedro Alves wrote:

> The only issue that I think might come out of this is
> that on MIPS, addresses are signed, so pointers are sign
> extended.  But we can definitely handle whatever fallout this
> may cause, if any, when we see it.  Clearly if GDB's native
> targets can handle that, so should gdbserver's, and in any
> case the issue should be pretty localized.  Just pointing it
> out FYI, to keep an eye out for it.

 Also SH64 AFAICT, it defines `elf_backend_sign_extend_vma' to true in 
bfd/elf32-sh64.c.  We have the following comment in gdb/mips-tdep.c:

/* MIPS believes that the PC has a sign extended value.  Perhaps the
   all registers should be sign extended for simplicity?  */

which is of course true in that we need to sign-extend all integer 
registers (that includes GPRs and CP0 registers; maybe some control 
registers as well); where applicable that is, i.e. debugging a strict 
32-bit ABI on 64-bit hardware.  Then on the other hand the values in these 
registers should already have been truncated to 32 bits and then 
sign-extended before they have been written in the first place.

 Overall it's tricky, hardware does not always enforce proper 
sign-extension required by the ABI, e.g. an o32 kernel-mode program is 
free to set GPRs or the PC to a value outside the range supported by the 
ABI (the Linux kernel sometimes takes advantage of this possibility), and 
we have no way I believe to make the user aware of this happening, while 
that might be The Bug they're after.

  Maciej
diff mbox

Patch

diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 6eb1a16..2d55513 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -87,7 +87,7 @@  typedef unsigned char gdb_byte;
 
 /* FIXME: This should probably be autoconf'd for.  It's an integer type at
    least the size of a (void *).  */
-typedef long long CORE_ADDR;
+typedef unsigned long long CORE_ADDR;
 
 typedef long long LONGEST;
 typedef unsigned long long ULONGEST;