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

[Hui Zhu <hui_zhu@mentor.com>]

On 03/18/14 18:14, Pedro Alves wrote:
> On 03/18/2014 07:37 AM, Hui Zhu wrote:
>> On 03/18/14 00:56, Pedro Alves wrote:
>
>> According to your comments,  I make a new patch that change all
>> methods return a static buffer.
>
> Thank you.
>
>>> Bummer that we don't have a test that caught this.  :-(
>>>
>>
>> Yes, I found it when I did regression test.
>
> WDYM?  What test failed then?

Sorry, my mean is I found the issue cannot be found by regression test.

>
>> Does testsuite have some example to test a GDB feature like "gdb -p"?
>
> I guess we'd refactor attach.exp a little to test that in addition
> to "attach", on native targets.  And I supposed we could pass down the
> -p to gdb by tweaking GDBFLAGS, like several tests do.  gdb.base/corefile.exp
> sounds like the model to follow, as that spawns "gdb -core", which is very
> similar to "gdb -p".

I make a patch for that.  I will post it in a separate mail.

>
>> 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)
>
> readlink does not '\0' terminate, we need to do that ourselves.
> See below.
>
>> -    {
>> -      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;
>
> No need for three buffers.  AFAICS, this should suffice:
>
>    static char buf[PATH_MAX];
>    char name[PATH_MAX];
>
>    xsnprintf (name, PATH_MAX, "/proc/%d/exe", pid);
>    memset (buf, 0, PATH_MAX);
>    if (readlink (name, buf, PATH_MAX - 1) <= 0)
>      strcpy (buf, name);
>    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;
>>    }
>
> Same with the nul termination.  The most standard solution is:
>
>    static char buf[PATH_MAX];
>    char name[PATH_MAX];
>    ssize_t len;
>
>    xsnprintf (name, PATH_MAX, "/proc/%d/exe", pid);
>    len = readlink (name, buf, PATH_MAX - 1);
>    if (len != -1)
>      {
>        buf[len] = '\0';
>        return buf;
>      }
>    return NULL;
>

I make a new patch according to your comments.
Please help me review it.

Thanks,
Hui

2014-03-19  Hui Zhu  <hui@codesourcery.com>
	    Pedro Alves  <palves@redhat.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 name[PATH_MAX];
  
  #ifdef KERN_PROC_PATHNAME
    int mib[4];
@@ -54,14 +54,11 @@ fbsd_pid_to_exec_file (struct target_ops
      return buf;
  #endif
  
-  path = xstrprintf ("/proc/%d/file", pid);
-  if (readlink (path, buf, PATH_MAX - 1) == -1)
-    {
-      xfree (buf);
-      buf = NULL;
-    }
+  xsnprintf (name, PATH_MAX, "/proc/%d/exe", pid);
+  memset (buf, 0, PATH_MAX);
+  if (readlink (name, buf, PATH_MAX - 1) <= 0)
+    strcpy (buf, name);
  
-  xfree (path);
    return buf;
  }
  
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4011,19 +4011,15 @@ 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 name[PATH_MAX];
  
-  name1 = xmalloc (PATH_MAX);
-  name2 = xmalloc (PATH_MAX);
-  make_cleanup (xfree, name1);
-  make_cleanup (xfree, name2);
-  memset (name2, 0, PATH_MAX);
+  xsnprintf (name, PATH_MAX, "/proc/%d/exe", pid);
+  memset (buf, 0, PATH_MAX);
+  if (readlink (name, buf, PATH_MAX - 1) <= 0)
+    strcpy (buf, name);
  
-  xsnprintf (name1, PATH_MAX, "/proc/%d/exe", pid);
-  if (readlink (name1, name2, PATH_MAX - 1) > 0)
-    return name2;
-  else
-    return name1;
+  return buf;
  }
  
  /* Records the thread's register state for the corefile note
--- a/gdb/nbsd-nat.c
+++ b/gdb/nbsd-nat.c
@@ -27,17 +27,13 @@
  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 name[PATH_MAX];
  
-  path = xstrprintf ("/proc/%d/exe", pid);
-  if (readlink (path, buf, PATH_MAX - 1) == -1)
-    {
-      xfree (buf);
-      buf = NULL;
-    }
+  xsnprintf (name, PATH_MAX, "/proc/%d/exe", pid);
+  memset (buf, 0, PATH_MAX);
+  if (readlink (name, buf, PATH_MAX - 1) <= 0)
+    strcpy (buf, name);
  
-  xfree (path);
    return buf;
  }