[RFA] ARI fix: %p rule

[RFA] ARI fix: %p rule

[Pierre Muller]

Most of the remaining usages of %p
in printf family messages
are related to pointers in the host space
rater that target space.

  Thus the recommendation to use paddress
is not correct for these cases, and I 
used host_address_to_string instead.

Is this OK?


Pierre Muller
Pascal language support maintainer for GDB

I left a few cases out, where
the %p value was not a simple pointer
but a arithmetic operation,
I don't know the rules for C,
is an addition of a pointer and a integer
always of type pointer?

2009-04-17  Pierre Muller  <muller.u-strasbg.fr>

	ARI fix: Do not use %p, replace by call to host_address_to_string
	for host pointers.
	* darwin-nat.c (darwin_xfer_partial): Apply change.
	* gnu-nat.c (inf_continue, gnu_xfer_memory): Ditto.
	* gnu-nat.h (proc_debug): Ditto.
	* symmisc.c (maintenance_info_symtabs): Ditto.
	(maintenance_info_psymtabs): Ditto.
	* windows-nat.c (handle_load_dll): Ditto.
	(handle_unload_dll, info_w32_command, handle_exception): Ditto.
	* xtensa-tdep.c (xtensa_unwind_pc): Ditto.


Index: src/gdb/darwin-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/darwin-nat.c,v
retrieving revision 1.7
diff -u -p -r1.7 darwin-nat.c
--- src/gdb/darwin-nat.c	24 Mar 2009 07:19:46 -0000	1.7
+++ src/gdb/darwin-nat.c	16 Apr 2009 23:42:57 -0000
@@ -1197,8 +1197,10 @@ darwin_xfer_partial (struct target_ops *
 		     gdb_byte *readbuf, const gdb_byte *writebuf,
 		     ULONGEST offset, LONGEST len)
 {
-  inferior_debug (8, _("darwin_xfer_partial(%s, %d, rbuf=%p, wbuf=%p)\n"),
-		  core_addr_to_string (offset), (int)len, readbuf,
writebuf);
+  inferior_debug (8, _("darwin_xfer_partial(%s, %d, rbuf=%s, wbuf=%s)\n"),
+		  core_addr_to_string (offset), (int)len, 
+		  host_address_to_string (readbuf),
+		  host_address_to_string (writebuf));
 
   if (object != TARGET_OBJECT_MEMORY)
     return -1;
Index: src/gdb/gnu-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/gnu-nat.c,v
retrieving revision 1.61
diff -u -p -r1.61 gnu-nat.c
--- src/gdb/gnu-nat.c	17 Mar 2009 19:28:08 -0000	1.61
+++ src/gdb/gnu-nat.c	16 Apr 2009 23:42:58 -0000
@@ -113,7 +113,8 @@ void inf_continue (struct inf *inf);
 
 #define inf_debug(_inf, msg, args...) \
   do { struct inf *__inf = (_inf); \
-       debug ("{inf %d %p}: " msg, __inf->pid, __inf , ##args); } while (0)
+       debug ("{inf %d %s}: " msg, __inf->pid, \
+       host_address_to_string (__inf) , ##args); } while (0)
 
 void proc_abort (struct proc *proc, int force);
 struct proc *make_proc (struct inf *inf, mach_port_t port, int tid);
@@ -2490,9 +2491,9 @@ gnu_xfer_memory (CORE_ADDR memaddr, gdb_
     return 0;
   else
     {
-      inf_debug (gnu_current_inf, "%s %p[%d] %s %p",
-		 write ? "writing" : "reading", (void *) memaddr, len,
-		 write ? "<--" : "-->", myaddr);
+      inf_debug (gnu_current_inf, "%s %s[%d] %s %s",
+		 write ? "writing" : "reading", paddr (memaddr), len,
+		 write ? "<--" : "-->", host_address_to_string (myaddr));
       if (write)
 	return gnu_write_inferior (task, memaddr, myaddr, len);
       else
Index: src/gdb/gnu-nat.h
===================================================================
RCS file: /cvs/src/src/gdb/gnu-nat.h,v
retrieving revision 1.9
diff -u -p -r1.9 gnu-nat.h
--- src/gdb/gnu-nat.h	3 Jan 2009 05:57:51 -0000	1.9
+++ src/gdb/gnu-nat.h	16 Apr 2009 23:42:58 -0000
@@ -88,8 +88,9 @@ extern char *proc_string (struct proc *p
 
 #define proc_debug(_proc, msg, args...) \
   do { struct proc *__proc = (_proc); \
-       debug ("{proc %d/%d %p}: " msg, \
-	      __proc_pid (__proc), __proc->tid, __proc , ##args); } while
(0)
+       debug ("{proc %d/%d %s}: " msg, \
+	      __proc_pid (__proc), __proc->tid,
+	      host_address_to_string (__proc) , ##args); } while (0)
 
 extern int gnu_debug_flag;
 
Index: src/gdb/symmisc.c
===================================================================
RCS file: /cvs/src/src/gdb/symmisc.c,v
retrieving revision 1.60
diff -u -p -r1.60 symmisc.c
--- src/gdb/symmisc.c	3 Jan 2009 05:57:53 -0000	1.60
+++ src/gdb/symmisc.c	16 Apr 2009 23:43:02 -0000
@@ -948,22 +948,24 @@ maintenance_info_symtabs (char *regexp, 
 		{
 		  printf_filtered ("{ objfile %s ", objfile->name);
 		  wrap_here ("  ");
-		  printf_filtered ("((struct objfile *) %p)\n", objfile);
+		  printf_filtered ("((struct objfile *) %s)\n", 
+				   host_address_to_string (objfile));
 		  printed_objfile_start = 1;
 		}
 
 	      printf_filtered ("	{ symtab %s ", symtab->filename);
 	      wrap_here ("    ");
-	      printf_filtered ("((struct symtab *) %p)\n", symtab);
+	      printf_filtered ("((struct symtab *) %s)\n", 
+			       host_address_to_string (symtab));
 	      printf_filtered ("	  dirname %s\n",
 			       symtab->dirname ? symtab->dirname :
"(null)");
 	      printf_filtered ("	  fullname %s\n",
 			       symtab->fullname ? symtab->fullname :
"(null)");
-	      printf_filtered ("	  blockvector ((struct blockvector
*) %p)%s\n",
-			       symtab->blockvector,
+	      printf_filtered ("	  blockvector ((struct blockvector
*) %s)%s\n",
+			       host_address_to_string (symtab->blockvector),
 			       symtab->primary ? " (primary)" : "");
-	      printf_filtered ("	  linetable ((struct linetable *)
%p)\n",
-			       symtab->linetable);
+	      printf_filtered ("	  linetable ((struct linetable *)
%s)\n",
+			       host_address_to_string (symtab->linetable));
 	      printf_filtered ("	  debugformat %s\n",
symtab->debugformat);
 	      printf_filtered ("	}\n");
 	    }
@@ -1003,13 +1005,16 @@ maintenance_info_psymtabs (char *regexp,
 		{
 		  printf_filtered ("{ objfile %s ", objfile->name);
 		  wrap_here ("  ");
-		  printf_filtered ("((struct objfile *) %p)\n", objfile);
+		  printf_filtered ("((struct objfile *) %s)\n", 
+				   host_address_to_string (objfile));
 		  printed_objfile_start = 1;
 		}
 
 	      printf_filtered ("  { psymtab %s ", psymtab->filename);
 	      wrap_here ("    ");
-	      printf_filtered ("((struct partial_symtab *) %p)\n", psymtab);
+	      printf_filtered ("((struct partial_symtab *) %s)\n", 
+			       host_address_to_string (psymtab));
+
 	      printf_filtered ("    readin %s\n",
 			       psymtab->readin ? "yes" : "no");
 	      printf_filtered ("    fullname %s\n",
@@ -1051,8 +1056,9 @@ maintenance_info_psymtabs (char *regexp,
 
 		      /* Note the string concatenation there --- no comma.
*/
 		      printf_filtered ("      psymtab %s "
-				       "((struct partial_symtab *) %p)\n",
-				       dep->filename, dep);
+				       "((struct partial_symtab *) %s)\n",
+				       dep->filename, 
+				       host_address_to_string (dep));
 		    }
 		  printf_filtered ("    }\n");
 		}
Index: src/gdb/windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.188
diff -u -p -r1.188 windows-nat.c
--- src/gdb/windows-nat.c	1 Apr 2009 16:35:40 -0000	1.188
+++ src/gdb/windows-nat.c	16 Apr 2009 23:43:03 -0000
@@ -740,8 +740,8 @@ handle_load_dll (void *dummy)
   solib_end->next = windows_make_so (dll_name, event->lpBaseOfDll);
   solib_end = solib_end->next;
 
-  DEBUG_EVENTS (("gdb: Loading dll \"%s\" at %p.\n", solib_end->so_name,
-		 solib_end->lm_info->load_addr));
+  DEBUG_EVENTS (("gdb: Loading dll \"%s\" at %s.\n", solib_end->so_name,
+		 host_address_to_string (solib_end->lm_info->load_addr)));
 
   return 1;
 }
@@ -774,7 +774,8 @@ handle_unload_dll (void *dummy)
 	return 1;
       }
 
-  error (_("Error: dll starting at %p not found."), lpBaseOfDll);
+  error (_("Error: dll starting at %s not found."),
+	   host_address_to_string (lpBaseOfDll));
 
   return 0;
 }
@@ -986,8 +987,9 @@ info_w32_command (char *args, int from_t
 
 
 #define DEBUG_EXCEPTION_SIMPLE(x)       if (debug_exceptions) \
-  printf_unfiltered ("gdb: Target exception %s at %p\n", x, \
-
current_event.u.Exception.ExceptionRecord.ExceptionAddress)
+  printf_unfiltered ("gdb: Target exception %s at %s\n", x, \
+    host_address_to_string (\
+      current_event.u.Exception.ExceptionRecord.ExceptionAddress))
 
 static int
 handle_exception (struct target_waitstatus *ourstatus)
@@ -1098,9 +1100,10 @@ handle_exception (struct target_waitstat
       /* Treat unhandled first chance exceptions specially. */
       if (current_event.u.Exception.dwFirstChance)
 	return -1;
-      printf_unfiltered ("gdb: unknown target exception 0x%08lx at %p\n",
-		    current_event.u.Exception.ExceptionRecord.ExceptionCode,
-
current_event.u.Exception.ExceptionRecord.ExceptionAddress);
+      printf_unfiltered ("gdb: unknown target exception 0x%08lx at %s\n",
+	current_event.u.Exception.ExceptionRecord.ExceptionCode,
+	host_address_to_string (
+	  current_event.u.Exception.ExceptionRecord.ExceptionAddress));
       ourstatus->value.sig = TARGET_SIGNAL_UNKNOWN;
       break;
     }
Index: src/gdb/xtensa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/xtensa-tdep.c,v
retrieving revision 1.32
diff -u -p -r1.32 xtensa-tdep.c
--- src/gdb/xtensa-tdep.c	22 Feb 2009 01:02:20 -0000	1.32
+++ src/gdb/xtensa-tdep.c	16 Apr 2009 23:43:04 -0000
@@ -1018,7 +1018,8 @@ xtensa_unwind_pc (struct gdbarch *gdbarc
   gdb_byte buf[8];
   CORE_ADDR pc;
 
-  DEBUGTRACE ("xtensa_unwind_pc (next_frame = %p)\n", next_frame);
+  DEBUGTRACE ("xtensa_unwind_pc (next_frame = %s)\n", 
+		host_address_to_string (next_frame));
 
   frame_unwind_register (next_frame, gdbarch_pc_regnum (gdbarch), buf);
   pc = extract_typed_address (buf, builtin_type
(gdbarch)->builtin_func_ptr);

Re: [RFA] ARI fix: %p rule

[Pedro Alves]

On Friday 17 April 2009 07:32:43, Pierre Muller wrote:
> Most of the remaining usages of %p
> in printf family messages
> are related to pointers in the host space
> rater that target space.
> 
>   Thus the recommendation to use paddress
> is not correct for these cases, and I 
> used host_address_to_string instead.
> 
> Is this OK?

Looks okay.  Thanks.

(I have a sekret itch that keeps telling me that
host_address_to_string is such a long name for this)

> Pierre Muller
> Pascal language support maintainer for GDB
> 
> I left a few cases out, where
> the %p value was not a simple pointer
> but a arithmetic operation,
> I don't know the rules for C,
> is an addition of a pointer and a integer
> always of type pointer?
> 
> 2009-04-17  Pierre Muller  <muller.u-strasbg.fr>
> 
>         ARI fix: Do not use %p, replace by call to host_address_to_string
>         for host pointers.
>         * darwin-nat.c (darwin_xfer_partial): Apply change.
>         * gnu-nat.c (inf_continue, gnu_xfer_memory): Ditto.
>         * gnu-nat.h (proc_debug): Ditto.
>         * symmisc.c (maintenance_info_symtabs): Ditto.
>         (maintenance_info_psymtabs): Ditto.
>         * windows-nat.c (handle_load_dll): Ditto.
>         (handle_unload_dll, info_w32_command, handle_exception): Ditto.
>         * xtensa-tdep.c (xtensa_unwind_pc): Ditto.
> 

-- 
Pedro Alves

Re: [RFA] ARI fix: %p rule

[Joel Brobecker]

> (I have a sekret itch that keeps telling me that
> host_address_to_string is such a long name for this)

It's a practice I inherited from my Ada days. I like clear names
even if it they end up being a bit on the longish side. But I'm
not opposed to making it shorter if you prefer (hostaddr2str
for instance, or ptr2str).

-- 
Joel

RE: [RFA] ARI fix: %p rule

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Friday, April 17, 2009 5:37 PM
> À : Pedro Alves
> Cc : gdb-patches@sourceware.org; Pierre Muller
> Objet : Re: [RFA] ARI fix: %p rule
> 
> > (I have a sekret itch that keeps telling me that
> > host_address_to_string is such a long name for this)
> 
> It's a practice I inherited from my Ada days. I like clear names
> even if it they end up being a bit on the longish side. But I'm
> not opposed to making it shorter if you prefer (hostaddr2str
> for instance, or ptr2str).
In fact we have paddr for
a pointer in target space, so something like 
haddr for host_address would be fine for me...


Pierre Muller
Pascal language support maintainer for GDB

Re: [RFA] ARI fix: %p rule

[Joel Brobecker]

> In fact we have paddr for
> a pointer in target space, so something like 
> haddr for host_address would be fine for me...

Not too keen on you suggestion (sorry!). "p" in my mind was for "print"
and "addr" rhymes with CORE_ADDR. That's why I suggested "ptr" instead,
to remove the potential confusion.

That being said, I'm wondering if we're getting dragged into another
bikeshed discussion... I've explained the reasoning behind my
suggestions, and I will keep my mouth shut now ;-).

-- 
Joel

Re: [RFA] ARI fix: %p rule

[Pedro Alves]

On Friday 17 April 2009 17:00:31, Joel Brobecker wrote:

> That being said, I'm wondering if we're getting dragged into another
> bikeshed discussion... I've explained the reasoning behind my
> suggestions, and I will keep my mouth shut now ;-).

Yeah, sorry about starting it.  It was a sekret itch!
Why are we talking about it in public?  :-)

-- 
Pedro Alves

[PATCH/Obvious] Fix error in last ARI fix for gnu-nat.h

[Pierre Muller]

I missed to add a continuation line in the proc_debug macro in gnu-nat.h

Committed as obvious fix.

I discovered this while looking at the
THREAD_STATE_* macros (listed in ARI).

  These macros are all defined only once in
config/i386/nm-i386gnu.h
and used only in gnu-nat.c source
unconditionally. This probably means that gnu hurd can only
be compiled for i386 processor anyhow... 
Which in turn, means that we could probably move the stuff 
from gnu-nat.c to i386gnu-nat.c

 I can send a patch for this, but 
I will not be able to test compilation,
as this would require access to gnu HURD.


Pierre Muller
Pascal language support maintainer for GDB




Index: gnu-nat.h
===================================================================
RCS file: /cvs/src/src/gdb/gnu-nat.h,v
retrieving revision 1.10
diff -u -p -r1.10 gnu-nat.h
--- gnu-nat.h   17 Apr 2009 15:44:28 -0000      1.10
+++ gnu-nat.h   21 Apr 2009 07:29:32 -0000
@@ -89,7 +89,7 @@ extern char *proc_string (struct proc *p
 #define proc_debug(_proc, msg, args...) \
   do { struct proc *__proc = (_proc); \
        debug ("{proc %d/%d %s}: " msg, \
-             __proc_pid (__proc), __proc->tid,
+             __proc_pid (__proc), __proc->tid, \
              host_address_to_string (__proc) , ##args); } while (0)

 extern int gnu_debug_flag;


For info, here is the old wrong patch, that I committed :

> Index: src/gdb/gnu-nat.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gnu-nat.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 gnu-nat.h
> --- src/gdb/gnu-nat.h	3 Jan 2009 05:57:51 -0000	1.9
> +++ src/gdb/gnu-nat.h	16 Apr 2009 23:42:58 -0000
> @@ -88,8 +88,9 @@ extern char *proc_string (struct proc *p
> 
>  #define proc_debug(_proc, msg, args...) \
>    do { struct proc *__proc = (_proc); \
> -       debug ("{proc %d/%d %p}: " msg, \
> -	      __proc_pid (__proc), __proc->tid, __proc , ##args); } while
> (0)
> +       debug ("{proc %d/%d %s}: " msg, \
> +	      __proc_pid (__proc), __proc->tid,
> +	      host_address_to_string (__proc) , ##args); } while (0)
> 
>  extern int gnu_debug_flag;
> 

Re: [PATCH/Obvious] Fix error in last ARI fix for gnu-nat.h

[Pedro Alves]

On Tuesday 21 April 2009 12:26:50, Pierre Muller wrote:
> I missed to add a continuation line in the proc_debug macro in gnu-nat.h
> 
> Committed as obvious fix.
> 
> I discovered this while looking at the
> THREAD_STATE_* macros (listed in ARI).
> 

>   These macros are all defined only once in
> config/i386/nm-i386gnu.h
> and used only in gnu-nat.c source
> unconditionally. This probably means that gnu hurd can only
> be compiled for i386 processor anyhow... 

Right, but the way the code is layed out, it should be easy to
add support for other processors, in case the Hurd is ported
(I'm not sure what archs the Hurd runs on).

> Which in turn, means that we could probably move the stuff 
> from gnu-nat.c to i386gnu-nat.c

I'm not exactly sure what you're proposing here.  It looks
like you'd have to move pieces of generic gnu/Hurd code
over to i386gnu-nat.c as well.  Keep in mind that macros
afecting the native target only in nm.h files aren't a
big a deal.  GDB can't be compiled for more than one
native target anyway.

>  I can send a patch for this, but 
> I will not be able to test compilation,
> as this would require access to gnu HURD.

I can do that for you, no problem.  I've got a debian/Hurd
vmware image here.

If you want to set one up yourself, it's easy --- you'll find
prebuilt debian/Hurd vmware images on the web ready to go.
Being debian, it doesn't feel *that* much different.

-- 
Pedro Alves