This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][PR threads/15824] when get threads name failed from info threads with linux kernel version earlier than 2.6.33


Will,
not every comments are addressed by this updated patch. I'll point them out again. Please make sure your new patch will address all of them.

On 08/16/2013 01:32 PM, Will Huang wrote:
+2013-08-14  Will Huang<will.huang@aliyun-inc.com>
+
+       PR threads/15824
   ^^^^^^^
Each line should be prefixed with 'TAB' instead of 8 spaces.

+       * linux-nat.c (linux_nat_thread_name): Add get thread name from
+       procfs "/proc/$pid/task/$tid/status".
+       * commo/linux-procfs.c (linux_proc_get_thread_name
+       linux_proc_get_string): New.

People prefer to give one entry for one function separately, like:

	* commo/linux-procfs.c (linux_proc_get_thread_name): New
	function.
	(linux_proc_get_string): New function.
+
  2013-04-26  Joel Brobecker<brobecker@adacore.com>

         * NEWS: Change "since GDB 7.5" into "in GDB 7.6".
diff -uNr ./gdb.org/common/linux-procfs.c ./gdb/common/linux-procfs.c
--- ./gdb.org/common/linux-procfs.c     2013-01-01 14:32:54.000000000 +0800
+++ ./gdb/common/linux-procfs.c 2013-08-16 11:48:49.000000000 +0800
@@ -55,6 +55,55 @@
    return retval;
  }

+/* Return the string length of FILED /proc/LWPID/task/TID/status.
+   Return -1 if not found.  */
+
+static int
+linux_proc_get_string (pid_t lwpid, pid_t tid, char *target,
+                       size_t t_size, const char *field)
+{
+  size_t field_len = strlen (field);
+  FILE *status_file;
+  char buf[100];
+  int retval = -1;
+
+  if (tid > 0)
+       snprintf (buf, sizeof (buf), "/proc/%d/task/%d/status",
+                                       (int) lwpid, (int) tid);

Use xsnprintf, and the indentation looks wrong.

+  else
+  {
+       /*if TID is zero, ingnore it*/
+       snprintf (buf, sizeof (buf), "/proc/%d/status", (int) lwpid);
+  }

and here....

+
+  status_file = fopen (buf, "r");

Use gdb_fopen_cloexec?

+  if (status_file == NULL)
+    {
+      warning (_("unable to open /proc file '%s'"), buf);
+      return -1;
+    }
+
+  while (fgets (buf, sizeof (buf), status_file))
+    if (strncmp (buf, field, field_len) == 0 && buf[field_len] == ':')
+      {
+       size_t pos = field_len + 1;
+       while((buf[pos] == ' ' || buf[pos] == '\t')
+                       && (pos < sizeof (buf) - 1))

Wrong indentation.

+         pos++;
+
+       strncpy (target, &buf[pos], t_size);
+       target[t_size - 1] = '\0';
+       retval = strlen (target);
+       if(retval > 0 && target[retval - 1] == '\n')
+               target[retval - 1] = '\0';

Wrong indentation.


diff -uNr ./gdb.org/linux-nat.c ./gdb/linux-nat.c
--- ./gdb.org/linux-nat.c       2013-02-13 22:59:49.000000000 +0800
+++ ./gdb/linux-nat.c   2013-08-16 13:22:31.000000000 +0800
@@ -4294,6 +4294,14 @@

        fclose (comm_file);
      }
+  else
+    {
+      static char comm[COMM_LEN + 1];
+      int size = linux_proc_get_thread_name (pid, lwp, comm, COMM_LEN + 1);

A blank line is needed.

The type of pid is int and type of lwp is long, but the types of
parameters of linux_proc_get_thread_name are pid_t and pid_t.  It is
inconsistent.

+      if (size > 0)
+        result = comm;
+
+    }

  #undef COMM_LEN
  #undef FORMAT
diff -uNr ./gdb.org/testsuite/ChangeLog ./gdb/testsuite/ChangeLog
--- ./gdb.org/testsuite/ChangeLog       2013-04-25 20:22:26.000000000 +0800
+++ ./gdb/testsuite/ChangeLog   2013-08-16 11:49:02.000000000 +0800
@@ -1,3 +1,9 @@
+2013-08-14  Will Huang<will.huang@aliyun-inc.com>
+
+       PR threads/15824
+       * gdb.threads/manythreads.exp: Add test for get thread
+       original name

Period is missing.

--
Yao (éå)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]