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

  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.
  

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?

> 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".

> 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;