RFC: Correct "paddr_t" in gdb_proc_service.h

RFC: Correct "paddr_t" in gdb_proc_service.h

[Daniel Jacobowitz]

If there is no <proc_service.h>, GDB will typedef unsigned long to paddr_t,
which is then used in various libthread_db interfaces.  Even when this code
was designed to work on Solaris systems in addition to GNU/Linux, this
wasn't right; Solaris doesn't provide a paddr_t that has anything to do with
libthread_db, only an unrelated one that deals with physical addressing.
The type that glibc's libthread_db uses is psaddr_t, which is a pointer
type.

When I wrote this patch (last year) I went through the original changelog
entries for this file from 2000; they suggest that this change is correct
and there should be no paddr_t references in GDB.

I admit that I can't rememeber now what problem this solved; it may be
nothing, just an inconsistency I noticed while working on N32 support. But I
think it's a worthwhile change to match the prototypes in libthread_db from
whence these functions are called.

Any comments on this change?

-- 
Daniel Jacobowitz
CodeSourcery

2006-03-15  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb_proc_service.h (paddr_t): Use psaddr_t if available.
	* proc-service.c (ps_xfer_memory): Cast paddr_t to unsigned
	long.
	(ps_pglobal_lookup): Cast CORE_ADDR to paddr_t.

Index: src/gdb/gdb_proc_service.h
===================================================================
--- src.orig/gdb/gdb_proc_service.h	2006-03-15 10:21:32.000000000 -0500
+++ src/gdb/gdb_proc_service.h	2006-03-15 10:21:36.000000000 -0500
@@ -1,5 +1,5 @@
 /* <proc_service.h> replacement for systems that don't have it.
-   Copyright (C) 2000 Free Software Foundation, Inc.
+   Copyright (C) 2000, 2006 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -48,10 +48,11 @@ typedef enum
 typedef unsigned int lwpid_t;
 #endif
 
-typedef unsigned long paddr_t;
-
 #ifndef HAVE_PSADDR_T
 typedef unsigned long psaddr_t;
+typedef unsigned long paddr_t;
+#else
+typedef psaddr_t paddr_t;
 #endif
 
 #ifndef HAVE_PRGREGSET_T
Index: src/gdb/proc-service.c
===================================================================
--- src.orig/gdb/proc-service.c	2006-03-15 10:21:32.000000000 -0500
+++ src/gdb/proc-service.c	2006-03-15 10:22:49.000000000 -0500
@@ -75,9 +75,9 @@ ps_xfer_memory (const struct ps_prochand
   inferior_ptid = pid_to_ptid (ph->pid);
 
   if (write)
-    ret = target_write_memory (addr, buf, len);
+    ret = target_write_memory ((unsigned long) addr, buf, len);
   else
-    ret = target_read_memory (addr, buf, len);
+    ret = target_read_memory ((unsigned long) addr, buf, len);
 
   do_cleanups (old_chain);
 
@@ -181,7 +181,7 @@ ps_pglobal_lookup (gdb_ps_prochandle_t p
   if (ms == NULL)
     return PS_NOSYM;
 
-  *sym_addr = SYMBOL_VALUE_ADDRESS (ms);
+  *sym_addr = (paddr_t) (unsigned long) SYMBOL_VALUE_ADDRESS (ms);
   return PS_OK;
 }
 

Re: RFC: Correct "paddr_t" in gdb_proc_service.h

[Michael Snyder]

Daniel Jacobowitz wrote:
> If there is no <proc_service.h>, GDB will typedef unsigned long to paddr_t,
> which is then used in various libthread_db interfaces.  Even when this code
> was designed to work on Solaris systems in addition to GNU/Linux, this
> wasn't right; Solaris doesn't provide a paddr_t that has anything to do with
> libthread_db, only an unrelated one that deals with physical addressing.
> The type that glibc's libthread_db uses is psaddr_t, which is a pointer
> type.
> 
> When I wrote this patch (last year) I went through the original changelog
> entries for this file from 2000; they suggest that this change is correct
> and there should be no paddr_t references in GDB.
> 
> I admit that I can't rememeber now what problem this solved; it may be
> nothing, just an inconsistency I noticed while working on N32 support. But I
> think it's a worthwhile change to match the prototypes in libthread_db from
> whence these functions are called.
> 
> Any comments on this change?
> 

Seems ok -- except the cast for target_read_memory
should be (CORE_ADDR) addr, n'est ce pas?

Re: RFC: Correct "paddr_t" in gdb_proc_service.h

[Daniel Jacobowitz]

On Wed, Mar 15, 2006 at 11:36:50AM -0500, Daniel Jacobowitz wrote:
> -    ret = target_write_memory (addr, buf, len);
> +    ret = target_write_memory ((unsigned long) addr, buf, len);
>    else
> -    ret = target_read_memory (addr, buf, len);
> +    ret = target_read_memory ((unsigned long) addr, buf, len);

On Wed, Mar 15, 2006 at 11:45:53AM -0800, Michael Snyder wrote:
> Seems ok -- except the cast for target_read_memory
> should be (CORE_ADDR) addr, n'est ce pas?

Oh, I'm glad you asked :-)  No, it shouldn't be, and yes, it should
raise a red flag.  psaddr_t is defined to void * by glibc.  That means
that if we cast it directly to a CORE_ADDR, we may get a warning about
it being the wrong size, from GCC.

Ugh, that means on MIPS for correctness we ought to be sign extending
it.  I don't know the right way to do that.  It didn't affect my
testing, because 32-bit user addresses have the high bit clear.

Um... I'll have to think about it some more :-(

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Correct "paddr_t" in gdb_proc_service.h

[Mark Kettenis]

> Date: Wed, 15 Mar 2006 11:36:50 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> If there is no <proc_service.h>, GDB will typedef unsigned long to paddr_t,
> which is then used in various libthread_db interfaces.  Even when this code
> was designed to work on Solaris systems in addition to GNU/Linux, this
> wasn't right; Solaris doesn't provide a paddr_t that has anything to do with
> libthread_db, only an unrelated one that deals with physical addressing.

Yes indeed.  On BSD paddr_t is an usnigned integer type used to store
physical memory addresses

> The type that glibc's libthread_db uses is psaddr_t, which is a pointer
> type.
> 
> When I wrote this patch (last year) I went through the original changelog
> entries for this file from 2000; they suggest that this change is correct
> and there should be no paddr_t references in GDB.

I think that's right, and therefore I don't understand your change at all :(.

> 2006-03-15  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* gdb_proc_service.h (paddr_t): Use psaddr_t if available.
> 	* proc-service.c (ps_xfer_memory): Cast paddr_t to unsigned
> 	long.
> 	(ps_pglobal_lookup): Cast CORE_ADDR to paddr_t.
> 
> Index: src/gdb/gdb_proc_service.h
> ===================================================================
> --- src.orig/gdb/gdb_proc_service.h	2006-03-15 10:21:32.000000000 -0500
> +++ src/gdb/gdb_proc_service.h	2006-03-15 10:21:36.000000000 -0500
> @@ -1,5 +1,5 @@
>  /* <proc_service.h> replacement for systems that don't have it.
> -   Copyright (C) 2000 Free Software Foundation, Inc.
> +   Copyright (C) 2000, 2006 Free Software Foundation, Inc.
>  
>     This file is part of GDB.
>  
> @@ -48,10 +48,11 @@ typedef enum
>  typedef unsigned int lwpid_t;
>  #endif
>  
> -typedef unsigned long paddr_t;
> -
>  #ifndef HAVE_PSADDR_T
>  typedef unsigned long psaddr_t;
> +typedef unsigned long paddr_t;
> +#else
> +typedef psaddr_t paddr_t;
>  #endif
>  
>  #ifndef HAVE_PRGREGSET_T
> Index: src/gdb/proc-service.c
> ===================================================================
> --- src.orig/gdb/proc-service.c	2006-03-15 10:21:32.000000000 -0500
> +++ src/gdb/proc-service.c	2006-03-15 10:22:49.000000000 -0500
> @@ -75,9 +75,9 @@ ps_xfer_memory (const struct ps_prochand
>    inferior_ptid = pid_to_ptid (ph->pid);
>  
>    if (write)
> -    ret = target_write_memory (addr, buf, len);
> +    ret = target_write_memory ((unsigned long) addr, buf, len);
>    else
> -    ret = target_read_memory (addr, buf, len);
> +    ret = target_read_memory ((unsigned long) addr, buf, len);
>  
>    do_cleanups (old_chain);
>  
> @@ -181,7 +181,7 @@ ps_pglobal_lookup (gdb_ps_prochandle_t p
>    if (ms == NULL)
>      return PS_NOSYM;
>  
> -  *sym_addr = SYMBOL_VALUE_ADDRESS (ms);
> +  *sym_addr = (paddr_t) (unsigned long) SYMBOL_VALUE_ADDRESS (ms);
>    return PS_OK;
>  }
>  
> 

Re: RFC: Correct "paddr_t" in gdb_proc_service.h

[Daniel Jacobowitz]

On Wed, Mar 15, 2006 at 11:34:07PM +0100, Mark Kettenis wrote:
> > Date: Wed, 15 Mar 2006 11:36:50 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > If there is no <proc_service.h>, GDB will typedef unsigned long to paddr_t,
> > which is then used in various libthread_db interfaces.  Even when this code
> > was designed to work on Solaris systems in addition to GNU/Linux, this
> > wasn't right; Solaris doesn't provide a paddr_t that has anything to do with
> > libthread_db, only an unrelated one that deals with physical addressing.
> 
> Yes indeed.  On BSD paddr_t is an usnigned integer type used to store
> physical memory addresses
> 
> > The type that glibc's libthread_db uses is psaddr_t, which is a pointer
> > type.
> > 
> > When I wrote this patch (last year) I went through the original changelog
> > entries for this file from 2000; they suggest that this change is correct
> > and there should be no paddr_t references in GDB.
> 
> I think that's right, and therefore I don't understand your change at all :(.

Well, we do have paddr_t references.  Want me to replace them all with
psaddr_t references while I'm here?  (The same casts will still be
necessary, since again, psaddr_t is a pointer type.)

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Correct "paddr_t" in gdb_proc_service.h

[Mark Kettenis]

> Date: Wed, 15 Mar 2006 17:36:29 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> > > When I wrote this patch (last year) I went through the original changelog
> > > entries for this file from 2000; they suggest that this change is correct
> > > and there should be no paddr_t references in GDB.
> > 
> > I think that's right, and therefore I don't understand your change
> > at all :(.
> 
> Well, we do have paddr_t references.  Want me to replace them all with
> psaddr_t references while I'm here?  (The same casts will still be
> necessary, since again, psaddr_t is a pointer type.)

Yes, I think you should.  And it looks like we really need a standard
function to convert a native pointer to a CORE_ADDR in gdb.

Mark

Re: RFC: Correct "paddr_t" in gdb_proc_service.h

[Daniel Jacobowitz]

On Wed, Mar 15, 2006 at 11:59:09PM +0100, Mark Kettenis wrote:
> Yes, I think you should.  And it looks like we really need a standard
> function to convert a native pointer to a CORE_ADDR in gdb.

Yes, it does.  Patch withdrawn; I'll come up with something better.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Correct "paddr_t" in gdb_proc_service.h

[Michael Snyder]

Mark Kettenis wrote:
> And it looks like we really need a standard
> function to convert a native pointer to a CORE_ADDR in gdb.

Second.  ;-)