Re: [PATCH 0/1] Fix internal warning when "gdb -p xxx"

[Hui Zhu <hui_zhu at mentor dot com>]

On 03/18/14 00:56, Pedro Alves wrote:
> On 03/17/2014 04:12 PM, Pedro Alves wrote:
> > On 03/17/2014 04:00 PM, Hui Zhu wrote:
> >
> > > The most of the pid_to_exec_file target_ops method for some platforms will
> > > allocate memory for exec_file and add them to cleanup.
> >
> > Which platforms?
>
> OK, I see several do that, including linux-nat.c.

I am sorry that I should not use "most".
Just darwin and linux add buf to cleanup.

fbsd and nbsd allocate memory but don't add them to cleanup.

windows use the static buf.

> IMO, that ends up being a silly interface.  The current
> interface is documented as:
>
> /* Attempts to find the pathname of the executable file
>     that was run to create a specified process.
>
>     The process PID must be stopped when this operation is used.
>
>     If the executable file cannot be determined, NULL is returned.
>
>     Else, a pointer to a character string containing the pathname
>     is returned.  This string should be copied into a buffer by
>     the client if the string will not be immediately used, or if
>     it must persist.  */
>
> #define target_pid_to_exec_file(pid) \
>       (current_target.to_pid_to_exec_file) (&current_target, pid)
>
> The "This string should be copied into a buffer by the client if
> the string will not be immediately used, or if it must persist."
> part hints that the implementation is supposed to return a pointer
> to a static buffer, like e.g., target_pid_to_str, paddress, and
> friends, etc.
>
> Either we make target_pid_to_exec_file return a pointer to
> a malloc buffer that the caller is responsible for xfree'ing (and
> adjust the interface comments in target.h) or we make targets
> indeed return a pointer to a static buffer, as the current
> method's description hints at.  Returning a malloced buffer, and
> installing a cleanup like that is a silly interface, IMO.  Note
> that GDB used to have more random memory-release cleanups installed
> like this, but we've removed most, I believe.  Although it's really
> not harmful to install a cleanup that just releases memory
> later at any random time, OTOH, it potentially makes debugging
> nasty cleanup issues harder, so we've moved away from doing that,
> and we now have that warning.

According to your comments,  I make a new patch that change all
methods return a static buffer.

Please help me review it.

> Bummer that we don't have a test that caught this.  :-(

Yes, I found it when I did regression test.
Does testsuite have some example to test a GDB feature like "gdb -p"?

Thanks,
Hui

2014-03-18  Hui Zhu  <hui@codesourcery.com>

	* darwin-nat.c (darwin_pid_to_exec_file): Change xmalloc to
	static buffer.
	* fbsd-nat.c (fbsd_pid_to_exec_file): Ditto.
	* linux-nat.c (linux_child_pid_to_exec_file): Ditto.
	* nbsd-nat.c (nbsd_pid_to_exec_file): Ditto.

--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1991,12 +1991,9 @@ set_enable_mach_exceptions (char *args,
 static char *
 darwin_pid_to_exec_file (struct target_ops *self, int pid)
 {
-  char *path;
+  static char path[PATH_MAX];
   int res;
 
-  path = xmalloc (PATH_MAX);
-  make_cleanup (xfree, path);
-
   res = proc_pidinfo (pid, PROC_PIDPATHINFO, 0, path, PATH_MAX);
   if (res >= 0)
     return path;
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -40,8 +40,8 @@ char *
 fbsd_pid_to_exec_file (struct target_ops *self, int pid)
 {
   size_t len = PATH_MAX;
-  char *buf = xcalloc (len, sizeof (char));
-  char *path;
+  static char buf[PATH_MAX];
+  char *path, *ret;
 
 #ifdef KERN_PROC_PATHNAME
   int mib[4];
@@ -56,13 +56,12 @@ fbsd_pid_to_exec_file (struct target_ops
 
   path = xstrprintf ("/proc/%d/file", pid);
   if (readlink (path, buf, PATH_MAX - 1) == -1)
-    {
-      xfree (buf);
-      buf = NULL;
-    }
+    ret = NULL;
+  else
+    ret = buf;
 
   xfree (path);
-  return buf;
+  return ret;
 }
 
 static int
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4011,19 +4011,18 @@ linux_nat_thread_name (struct target_ops
 static char *
 linux_child_pid_to_exec_file (struct target_ops *self, int pid)
 {
-  char *name1, *name2;
+  static char buf[PATH_MAX];
+  char name1[PATH_MAX], name2[PATH_MAX];
 
-  name1 = xmalloc (PATH_MAX);
-  name2 = xmalloc (PATH_MAX);
-  make_cleanup (xfree, name1);
-  make_cleanup (xfree, name2);
   memset (name2, 0, PATH_MAX);
 
   xsnprintf (name1, PATH_MAX, "/proc/%d/exe", pid);
   if (readlink (name1, name2, PATH_MAX - 1) > 0)
-    return name2;
+    strcpy (buf, name2);
   else
-    return name1;
+    strcpy (buf, name1);
+
+  return buf;
 }
 
 /* Records the thread's register state for the corefile note
--- a/gdb/nbsd-nat.c
+++ b/gdb/nbsd-nat.c
@@ -28,16 +28,15 @@ char *
 nbsd_pid_to_exec_file (struct target_ops *self, int pid)
 {
   size_t len = PATH_MAX;
-  char *buf = xcalloc (len, sizeof (char));
-  char *path;
+  static char buf[PATH_MAX];
+  char *path, *ret;
 
   path = xstrprintf ("/proc/%d/exe", pid);
   if (readlink (path, buf, PATH_MAX - 1) == -1)
-    {
-      xfree (buf);
-      buf = NULL;
-    }
+    ret = NULL;
+  else
+    ret = buf;
 
   xfree (path);
-  return buf;
+  return ret;
 }