Re: [PATCH/RFC 02/02 v2] Refactor PRPSINFO handling on GDB

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Thanks for the review.

On Tuesday, December 18 2012, Jan Kratochvil wrote:

> On Mon, 17 Dec 2012 04:09:57 +0100, Sergio Durigan Junior wrote:

>> 2012-12-17  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> AFAIK it is based on an older patch by Denys Vlasenko.

Yes, you're right, I will put his name on the ChangeLog entry too.

> [...]
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>
> linux-nat.c (and therefore neither linux-nat.h) should no longer be touched,
> it should be in linux-tdep.c now, core generating code has moved in the
> meantime.

Thanks, fixed.

>> +
>> +  if (n_fields != 6)
>> +    {
>> +      /* Again, we couldn't read the complementary information about the
>> +	 process state.  However, we already have minimal information, so we
>> +	 just return 1 here.  */
>> +      do_cleanups (c);
>> +      return 1;
>> +    }
>> +
>> +  /* Filling the structure fields.  */
>> +  for (i = 0; i < sizeof (valid_states); ++i)
>> +    if (pr_sname == valid_states[i])
>> +      break;
>
> Do you find it with strchr more complicated?  Also it does not sanity check
> the read-in PR_SNAME value.

I fixed all the issues pointed by you.  I am now using strchr to perform
the check here, but I'd like you to take a look again and check that
everything is fine.  I tried to mimic what the Linux kernel already does
(see fs/binfmt_elf.c:fill_psinfo in the Linux kernel tree), together
with what's documented in the manpage of proc(5), entry
`/proc/[pid]/stat'.


>> +  p->pr_state = i;
>> +  p->pr_sname = pr_sname;
>> +  p->pr_zomb = pr_sname == 'Z';
>> +  p->pr_nice = pr_nice;
>> +  p->pr_flag = pr_flag;
>> +  p->pr_uid = proc_st.st_uid;
>> +  p->pr_gid = proc_st.st_gid;
>> +  p->pr_ppid = pr_ppid;
>> +  p->pr_pgrp = pr_pgrp;
>> +  p->pr_sid = pr_sid;
>> +
>> +  do_cleanups (c);
>> +
>> +  return 1;
>> +}
>
> Otherwise I am fine with it after patch 1/2 gets resolved.

I will send the ChangeLog entry later.

Thanks,

-- 
Sergio

---
 gdb/linux-tdep.c |  215 +++++++++++++++++++++++++++++++++++++++++++++++++----
 gdb/procfs.c     |   37 +++++----
 2 files changed, 218 insertions(+), 34 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index d5ad6e3..b03c2d4 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -28,10 +28,13 @@
 #include "regset.h"
 #include "elf/common.h"
 #include "elf-bfd.h"            /* for elfcore_write_* */
+#include "elf-psinfo.h"		/* for `struct elf_internal_prpsinfo' */
 #include "inferior.h"
 #include "cli/cli-utils.h"
 #include "arch-utils.h"
 #include "gdb_obstack.h"
+#include "cli/cli-utils.h"
+#include "exceptions.h"
 
 #include <ctype.h>
 
@@ -1153,6 +1156,197 @@ linux_corefile_thread_callback (struct thread_info *info, void *data)
   return !args->note_data;
 }
 
+/* Fill the PRPSINFO structure with information about the process being
+   debugged.  Returns 1 in case of success, 0 for failures.  Please note that
+   even if the structure cannot be entirely filled (e.g., GDB was unable to
+   gather information about the process UID/GID), this function will still
+   return 1 since some information was already recorded.  It will only return
+   0 iff nothing can be gathered.  */
+
+static int
+linux_fill_prpsinfo (struct elf_internal_prpsinfo *p)
+{
+  /* The filename which we will use to obtain some info about the process.
+     We will basically use this to store the `/proc/PID/FILENAME' file.  */
+  char filename[100];
+  /* The full name of the program which generated the corefile.  */
+  char *fname;
+  /* The basename of the executable.  */
+  const char *basename;
+  /* The arguments of the program.  */
+  char *psargs;
+  char *infargs;
+  /* The contents of `/proc/PID/stat' file.  */
+  char *proc_stat, *proc_stat_orig;
+  /* The valid states of a process, according to the Linux kernel.  */
+  const char valid_states[] = "RSDTZW";
+  /* The program state.  */
+  char *prog_state;
+  /* The state of the process.  */
+  char pr_sname;
+  /* The PID of the program which generated the corefile.  */
+  pid_t pid;
+  /* Process flags.  */
+  unsigned int pr_flag;
+  /* Process nice value.  */
+  long pr_nice;
+  /* The stat of the `/proc/PID/stat' file.  */
+  struct stat proc_st;
+  /* The number of fields read by `sscanf'.  */
+  int n_fields = 0;
+  /* Cleanups.  */
+  struct cleanup *c;
+  int i;
+  volatile struct gdb_exception ex;
+
+  gdb_assert (p != NULL);
+
+  /* Obtaining PID and filename.  */
+  pid = ptid_get_pid (inferior_ptid);
+  xsnprintf (filename, sizeof (filename), "/proc/%u/cmdline", pid);
+  fname = target_fileio_read_stralloc (filename);
+
+  if (fname == NULL || *fname == '\0')
+    {
+      /* No program name was read, so we won't be able to retrieve more
+	 information about the process.  */
+      xfree (fname);
+      return 0;
+    }
+
+  c = make_cleanup (xfree, fname);
+  memset (p, 0, sizeof (*p));
+
+  /* Obtaining the file stat as well.  */
+  errno = 0;
+  if (stat (filename, &proc_st) != 0)
+    {
+      warning (_("Could not stat file `%s': %s"), filename,
+	       safe_strerror (errno));
+      p->pr_uid = 0;
+      p->pr_gid = 0;
+    }
+  else
+    {
+      p->pr_uid = proc_st.st_uid;
+      p->pr_gid = proc_st.st_gid;
+    }
+
+  /* Defining the PID.  */
+  p->pr_pid = pid;
+
+  /* Copying the program name.  Only the basename matters.  */
+  basename = lbasename (fname);
+  strncpy (p->pr_fname, basename, sizeof (p->pr_fname));
+  p->pr_fname[sizeof (p->pr_fname) - 1] = '\0';
+
+  /* Generating and copying the program's arguments.  `get_inferior_args'
+     may throw, but we want to continue the execution anyway.  */
+  TRY_CATCH (ex, RETURN_MASK_ERROR)
+    {
+      infargs = get_inferior_args ();
+    }
+
+  if (ex.reason < 0)
+    {
+      warning (_("Could not obtain inferior's arguments."));
+      infargs = NULL;
+    }
+
+  psargs = xstrdup (fname);
+  if (infargs != NULL)
+    psargs = reconcat (psargs, psargs, " ", infargs, NULL);
+
+  make_cleanup (xfree, psargs);
+
+  strncpy (p->pr_psargs, psargs, sizeof (p->pr_psargs));
+  p->pr_psargs[sizeof (p->pr_psargs) - 1] = '\0';
+
+  xsnprintf (filename, sizeof (filename), "/proc/%u/stat", pid);
+  proc_stat = target_fileio_read_stralloc (filename);
+
+  if (proc_stat == NULL || *proc_stat == '\0')
+    {
+      /* Despite being unable to read more information about the process, we
+	 return 1 here because at least we have its command line, PID and
+	 arguments.  */
+      xfree (proc_stat);
+      do_cleanups (c);
+      return 1;
+    }
+
+  proc_stat_orig = proc_stat;
+  make_cleanup (xfree, proc_stat_orig);
+
+  /* Ok, we have the stats.  It's time to do a little parsing of the contents
+     of the buffer, so that we end up reading what we want.
+
+     The following parsing mechanism is strongly based on the information
+     generated by the `fs/proc/array.c' file, present in the Linux kernel
+     tree.  More details about how the information is displayed can be
+     obtained by seeing the manpage of proc(5), specifically under the entry
+     of `/proc/[pid]/stat'.  */
+
+  /* Getting rid of the PID, since we already have it.  */
+  while (isdigit (*proc_stat))
+    ++proc_stat;
+
+  proc_stat = skip_spaces (proc_stat);
+
+  /* Getting rid of the executable name, since we already have it.  We know
+     that this name will be in parentheses, so we can safely look for the
+     close-paren.  */
+  while (*proc_stat != ')')
+    ++proc_stat;
+  ++proc_stat;
+
+  proc_stat = skip_spaces (proc_stat);
+
+  n_fields = sscanf (proc_stat,
+		     "%c"		/* Process state.  */
+		     "%d%d%d"		/* Parent PID, group ID, session ID.  */
+		     "%*d%*d"		/* tty_nr, tpgid (not used).  */
+		     "%u"		/* Flags.  */
+		     "%*s%*s%*s%*s"	/* minflt, cminflt, majflt,
+					   cmajflt (not used).  */
+		     "%*s%*s%*s%*s"	/* utime, stime, cutime,
+					   cstime (not used).  */
+		     "%*s"		/* Priority (not used).  */
+		     "%ld",		/* Nice.  */
+		     &pr_sname,
+		     &p->pr_ppid, &p->pr_pgrp, &p->pr_sid,
+		     &pr_flag,
+		     &pr_nice);
+
+  if (n_fields != 6)
+    {
+      /* Again, we couldn't read the complementary information about the
+	 process state.  However, we already have minimal information, so we
+	 just return 1 here.  */
+      do_cleanups (c);
+      return 1;
+    }
+
+  /* Filling the structure fields.  */
+  prog_state = strchr (valid_states, pr_sname);
+  if (prog_state != NULL)
+    p->pr_state = prog_state - valid_states;
+  else
+    {
+      /* Zero means "Running".  */
+      p->pr_state = 0;
+    }
+
+  p->pr_sname = p->pr_state > 5 ? '.' : pr_sname;
+  p->pr_zomb = p->pr_sname == 'Z';
+  p->pr_nice = pr_nice;
+  p->pr_flag = pr_flag;
+
+  do_cleanups (c);
+
+  return 1;
+}
+
 /* Fills the "to_make_corefile_note" target vector.  Builds the note
    section for a corefile, and returns it in a malloc buffer.  */
 
@@ -1161,27 +1355,14 @@ linux_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size,
 			   linux_collect_thread_registers_ftype collect)
 {
   struct linux_corefile_thread_data thread_args;
+  struct elf_internal_prpsinfo prpsinfo;
   char *note_data = NULL;
   gdb_byte *auxv;
   int auxv_len;
 
-  /* Process information.  */
-  if (get_exec_file (0))
-    {
-      const char *fname = lbasename (get_exec_file (0));
-      char *psargs = xstrdup (fname);
-
-      if (get_inferior_args ())
-        psargs = reconcat (psargs, psargs, " ", get_inferior_args (),
-			   (char *) NULL);
-
-      note_data = elfcore_write_prpsinfo (obfd, note_data, note_size,
-                                          fname, psargs);
-      xfree (psargs);
-
-      if (!note_data)
-	return NULL;
-    }
+  if (linux_fill_prpsinfo (&prpsinfo))
+    note_data = elfcore_write_prpsinfo (obfd, note_data, note_size,
+					&prpsinfo);
 
   /* Thread register information.  */
   thread_args.gdbarch = gdbarch;
diff --git a/gdb/procfs.c b/gdb/procfs.c
index 1c5cc13..3125c02 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -25,6 +25,7 @@
 #include "target.h"
 #include "gdbcore.h"
 #include "elf-bfd.h"		/* for elfcore_write_* */
+#include "elf-psinfo.h"		/* for struct elf_internal_prpsinfo */
 #include "gdbcmd.h"
 #include "gdbthread.h"
 #include "regcache.h"
@@ -5471,39 +5472,41 @@ procfs_make_note_section (bfd *obfd, int *note_size)
   struct cleanup *old_chain;
   gdb_gregset_t gregs;
   gdb_fpregset_t fpregs;
-  char fname[16] = {'\0'};
-  char psargs[80] = {'\0'};
+  struct elf_internal_prpsinfo prpsinfo;
   procinfo *pi = find_procinfo_or_die (PIDGET (inferior_ptid), 0);
   char *note_data = NULL;
-  char *inf_args;
   struct procfs_corefile_thread_data thread_args;
   gdb_byte *auxv;
   int auxv_len;
   enum gdb_signal stop_signal;
 
+  memset (&prpsinfo, 0, sizeof (prpsinfo));
+
   if (get_exec_file (0))
     {
-      strncpy (fname, lbasename (get_exec_file (0)), sizeof (fname));
-      fname[sizeof (fname) - 1] = 0;
-      strncpy (psargs, get_exec_file (0), sizeof (psargs));
-      psargs[sizeof (psargs) - 1] = 0;
+      char *inf_args;
+      char *psargs;
+
+      strncpy (prpsinfo.pr_fname, lbasename (get_exec_file (0)),
+	       sizeof (prpsinfo.pr_fname));
+      prpsinfo.pr_fname[sizeof (prpsinfo.pr_fname) - 1] = '\0';
+
+      psargs = xstrdup (prpsinfo.pr_fname);
 
       inf_args = get_inferior_args ();
-      if (inf_args && *inf_args &&
-	  strlen (inf_args) < ((int) sizeof (psargs) - (int) strlen (psargs)))
-	{
-	  strncat (psargs, " ",
-		   sizeof (psargs) - strlen (psargs));
-	  strncat (psargs, inf_args,
-		   sizeof (psargs) - strlen (psargs));
-	}
+      if (inf_args != NULL)
+	psargs = reconcat (psargs, psargs, " ", inf_args, NULL);
+
+      strncpy (prpsinfo.pr_psargs, psargs, sizeof (prpsinfo.pr_psargs));
+      prpsinfo.pr_psargs[sizeof (prpsinfo.pr_psargs) - 1] = '\0';
+
+      xfree (psargs);
     }
 
   note_data = (char *) elfcore_write_prpsinfo (obfd,
 					       note_data,
 					       note_size,
-					       fname,
-					       psargs);
+					       &prpsinfo);
 
   stop_signal = find_stop_signal ();
 
-- 
1.7.7.6