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

  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.
  

On 03/19/2014 03:57 AM, Hui Zhu wrote:
>> >
>> > 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.

The patch changes the bsd implementations's behavior, because
you made them return the /proc path when readlink fails (like
the Linux version does), instead of what the current code does
or what I suggested above.