[0/1] Fix internal warning when "gdb -p xxx"
Commit Message
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.
Comments
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.
@@ -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;
@@ -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;
}
@@ -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
@@ -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;
}