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

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

[Sergio Durigan Junior]

Hi,

This is a follow-up on:
     <http://sourceware.org/ml/binutils/2012-11/msg00240.html>

Here is the second attempt to refactor the PRPSINFO handling on GDB and
Binutils.  This patch specifically touches the GDB area, but it is
dependent on the first patch which touches BFD.

The basic idea of the patch is to fill the `struct
elf_internal_prpsinfo' with proper information about the process being
debugged.  The new function `linux_nat_fill_prpsinfo' will be called
whenever the user requests GDB to generate a corefile.

I did my best to make the function easy to understand, but there are
some "tricks" there (specially the `sscanf' thing) which I'd like to ask
for opinions.  It's working OK and I don't think it's bad code, but it's
better to be safe and consult the maintainers.

I have tested this code on x86_64 (with -m32 too), PPC64 (with -m32 too)
and ARM.  I read the corefile generated using eu-readelf and it
correctly displayed the PRPSINFO section.  I would like some help with
the procfs code, because I could not find a way to test it.

Comments?  Ok to check-in?

-- 
Sergio

2012-12-17  Sergio Durigan Junior  <sergiodj@redhat.com>

	* linux-nat.c: Include `cli/cli-utils.h' and `elf-psinfo.h'.
	(linux_nat_fill_prpsinfo): New function.
	* linux-nat.h (elf_internal_prpsinfo): Forward declaration for
	structure.
	(linux_nat_fill_prpsinfo): New declaration.
	* linux-tdep.c: Include `elf-psinfo.h' and `linux-nat.h'.
	(linux_make_corefile_notes): Refactor code to gather information for
	PRPSINFO, using linux_nat_fill_prpsinfo.
	* procfs.c: Include `elf-psinfo.h'.
	(procfs_make_note_section): Refactor code to gather basic information
	for PRPSINFO.

---
 gdb/linux-nat.c  |  164 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/linux-nat.h  |   14 +++++
 gdb/linux-tdep.c |   23 ++------
 gdb/procfs.c     |   37 +++++++------
 4 files changed, 204 insertions(+), 34 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index f5ca977..15f69b1 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -67,6 +67,8 @@
 #include "linux-ptrace.h"
 #include "buffer.h"
 #include "target-descriptions.h"
+#include "cli/cli-utils.h"
+#include "elf-psinfo.h"		/* for `struct elf_internal_prpsinfo' */
 
 #ifndef SPUFS_MAGIC
 #define SPUFS_MAGIC 0x23c9b64e
@@ -4368,6 +4370,168 @@ linux_nat_collect_thread_registers (const struct regcache *regcache,
   return note_data;
 }
 
+/* See definition at linux-nat.h.  */
+
+int
+linux_nat_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;
+  /* The valid states of a process, according to the Linux kernel.  */
+  const char valid_states[] = "RSDTZW";
+  /* The state of the process.  */
+  char pr_sname;
+  /* The PID of the program which generated the corefile.  */
+  pid_t pid;
+  /* Parent PID, process group ID and session ID.  */
+  int pr_ppid, pr_pgrp, pr_sid;
+  /* 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;
+
+  gdb_assert (p != NULL);
+
+  /* Initializing the cleanup chain.  */
+  c = make_cleanup (null_cleanup, NULL);
+
+  /* Obtaining PID and filename.  */
+  pid = ptid_get_pid (inferior_ptid);
+  xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", pid);
+  fname = target_fileio_read_stralloc (filename);
+
+  if (fname == NULL || strcmp (fname, "") == 0)
+    {
+      /* No program name was read, so we won't be able to retrieve more
+	 information about the process.  */
+      return 0;
+    }
+
+  make_cleanup (xfree, fname);
+  memset (p, 0, sizeof (*p));
+
+  /* Obtaining the file stat as well.  */
+  stat (filename, &proc_st);
+
+  /* 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.  */
+  psargs = xstrdup (fname);
+  infargs = get_inferior_args ();
+
+  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/%d/stat", pid);
+  proc_stat = target_fileio_read_stralloc (filename);
+
+  if (proc_stat == NULL || strcmp (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.  */
+      do_cleanups (c);
+      return 1;
+    }
+
+  /* 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,
+		     &pr_ppid, &pr_pgrp, &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.  */
+  for (i = 0; i < sizeof (valid_states); ++i)
+    if (pr_sname == valid_states[i])
+      break;
+
+  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;
+}
+
 /* Fills the "to_make_corefile_note" target vector.  Builds the note
    section for a corefile, and returns it in a malloc buffer.  */
 
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index 50998b8..8615e4f 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -23,6 +23,11 @@
 
 struct arch_lwp_info;
 
+/* Forward declaration of the PRPSINFO structure.  For more details, look at
+   `bfd/elf-psinfo.h'.  */
+
+struct elf_internal_prpsinfo;
+
 /* Ways to "resume" a thread.  */
 
 enum resume_kind
@@ -201,3 +206,12 @@ int linux_nat_get_siginfo (ptid_t ptid, siginfo_t *siginfo);
 /* Set alternative SIGTRAP-like events recognizer.  */
 void linux_nat_set_status_is_event (struct target_ops *t,
 				    int (*status_is_event) (int status));
+
+/* 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.  */
+
+int linux_nat_fill_prpsinfo (struct elf_internal_prpsinfo *p);
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index d5ad6e3..e1c76eb 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -28,10 +28,12 @@
 #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 "linux-nat.h"
 
 #include <ctype.h>
 
@@ -1161,27 +1163,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_nat_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

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

[Jan Kratochvil]

On Mon, 17 Dec 2012 04:09:57 +0100, Sergio Durigan Junior wrote:
> I have tested this code on x86_64 (with -m32 too), PPC64 (with -m32 too)
> and ARM.  I read the corefile generated using eu-readelf and it
> correctly displayed the PRPSINFO section.

There could be a testcase using eu-readelf but I do not insist on it.


> 2012-12-17  Sergio Durigan Junior  <sergiodj@redhat.com>

AFAIK it is based on an older patch by Denys Vlasenko.


[...]
> --- 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.


> @@ -67,6 +67,8 @@
>  #include "linux-ptrace.h"
>  #include "buffer.h"
>  #include "target-descriptions.h"
> +#include "cli/cli-utils.h"
> +#include "elf-psinfo.h"		/* for `struct elf_internal_prpsinfo' */
>  
>  #ifndef SPUFS_MAGIC
>  #define SPUFS_MAGIC 0x23c9b64e
> @@ -4368,6 +4370,168 @@ linux_nat_collect_thread_registers (const struct regcache *regcache,
>    return note_data;
>  }
>  
> +/* See definition at linux-nat.h.  */
> +
> +int
> +linux_nat_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;
> +  /* The valid states of a process, according to the Linux kernel.  */
> +  const char valid_states[] = "RSDTZW";
> +  /* The state of the process.  */
> +  char pr_sname;
> +  /* The PID of the program which generated the corefile.  */
> +  pid_t pid;
> +  /* Parent PID, process group ID and session ID.  */
> +  int pr_ppid, pr_pgrp, pr_sid;
> +  /* 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;
> +
> +  gdb_assert (p != NULL);
> +
> +  /* Initializing the cleanup chain.  */
> +  c = make_cleanup (null_cleanup, NULL);

One could initialize C by the first real make_cleanup below.


> +
> +  /* Obtaining PID and filename.  */
> +  pid = ptid_get_pid (inferior_ptid);
> +  xsnprintf (filename, sizeof (filename), "/proc/%d/cmdline", pid);
> +  fname = target_fileio_read_stralloc (filename);
> +
> +  if (fname == NULL || strcmp (fname, "") == 0)

Why strcmp, GDB uses just *fname == '\0'.


> +    {
> +      /* No program name was read, so we won't be able to retrieve more
> +	 information about the process.  */

FNAME leaks if it was "".


C is not destroyed.


> +      return 0;
> +    }
> +
> +  make_cleanup (xfree, fname);
> +  memset (p, 0, sizeof (*p));
> +
> +  /* Obtaining the file stat as well.  */
> +  stat (filename, &proc_st);

One could print a warning on failed stat.  And if it fails pr_uid and pr_gid
could have better default than the uninitialized data.


> +
> +  /* 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.  */
> +  psargs = xstrdup (fname);
> +  infargs = get_inferior_args ();

get_inferior_args can throw, PSARGS leaks then.


> +
> +  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/%d/stat", pid);

pid is pid_t, I believe on some systems it may be incompatible with %d.


> +  proc_stat = target_fileio_read_stralloc (filename);

PROC_STAT leaks.


> +
> +  if (proc_stat == NULL || strcmp (proc_stat, "") == 0)

Again *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.  */
> +      do_cleanups (c);
> +      return 1;
> +    }
> +
> +  /* 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.  */

The comments could be better tab aligned into some column.  Also spaces in the
sscanf string are not needed.


> +		     &pr_sname,
> +		     &pr_ppid, &pr_pgrp, &pr_sid,
> +		     &pr_flag,
> +		     &pr_nice);

I think you could read some of the fields - at least pr_ppid - directly into P
without local variables.


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


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


Thanks,
Jan

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

[Sergio Durigan Junior]

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

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

[Sergio Durigan Junior]

On Tuesday, December 25 2012, I wrote:

> I will send the ChangeLog entry later.

Here is the ChangeLog entry, along with an updated version of the
patch which doesn't use elf-psinfo.h anymore.

Thanks,

-- 
Sergio

2012-12-29  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Denys Vlasenko  <dvlasenk@redhat.com>

	* linux-tdep.c: Include `cli/cli-utils.h' and `exceptions.h'.
	(linux_fill_prpsinfo): New function.
	(linux_make_corefile_notes): Refactor code to gather information for
	PRPSINFO, using linux_nat_fill_prpsinfo.
	* procfs.c (procfs_make_note_section): Refactor code to gather basic
	information for PRPSINFO.

---
 gdb/linux-tdep.c |  214 +++++++++++++++++++++++++++++++++++++++++++++++++----
 gdb/procfs.c     |   36 +++++----
 2 files changed, 216 insertions(+), 34 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index d5ad6e3..18b817f 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -32,6 +32,8 @@
 #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 +1155,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 +1354,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..af0c994 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -5471,39 +5471,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

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

[Jan Kratochvil]

On Sun, 30 Dec 2012 02:53:18 +0100, Sergio Durigan Junior wrote:
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index d5ad6e3..18b817f 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
[...]
> @@ -1153,6 +1155,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;

It could be (also for C++ compliance with its overloaded strchr):
	const 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)

A nitpick but a failed syscall has to initialize ERRNO so the explicit
initialization is not needed.

(It is used for example for ptrace(PTRACE_PEEKTEXT) where one cannot find out
whether the syscall was successful or not from the ptrace return value.)


> +    {
> +      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."));

You could print also ex.message here.


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

Originally:
# > +  xsnprintf (filename, sizeof (filename), "/proc/%d/stat", pid);
# 
# pid is pid_t, I believe on some systems it may be incompatible with %d.

Sorry I did mean to use:
	xsnprintf (filename, sizeof (filename), "/proc/%d/stat", (int) pid);


> +  proc_stat = target_fileio_read_stralloc (filename);

Moving make_cleanup (xfree, proc_stat); here will not need xfree (proc_stat);
below.


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

Probably some leftover, it is enough to call:
	make_cleanup (xfree, proc_stat);

PROC_STAT is passed by value, not by reference.


> +
> +  /* 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'.  */
[...]


Thanks,
Jan

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

[Sergio Durigan Junior]

On Monday, December 31 2013, Jan Kratochvil wrote:

> On Sun, 30 Dec 2012 02:53:18 +0100, Sergio Durigan Junior wrote:

Thanks for the comments.  Here is the updated version of it, reflecting
also the changes proposed on the BFD part.

Thanks,

-- 
Sergio

2013-01-04  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Denys Vlasenko  <dvlasenk@redhat.com>

	* linux-tdep.c: Include `cli/cli-utils.h' and `exceptions.h'.
	(linux_fill_prpsinfo): New function.
	(linux_make_corefile_notes): Refactor code to gather information for
	PRPSINFO, using linux_nat_fill_prpsinfo.
	* procfs.c (procfs_make_note_section): Refactor code to gather basic
	information for PRPSINFO.

---
 gdb/linux-tdep.c |  210 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 gdb/procfs.c     |   36 +++++-----
 2 files changed, 212 insertions(+), 34 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 836da62..fb62207 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -32,6 +32,8 @@
 #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 +1155,193 @@ 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;
+  /* The valid states of a process, according to the Linux kernel.  */
+  const char valid_states[] = "RSDTZW";
+  /* The program state.  */
+  const 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.  */
+  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: %s"), ex.message);
+      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/%d/stat", (int) pid);
+  proc_stat = target_fileio_read_stralloc (filename);
+  make_cleanup (xfree, proc_stat);
+
+  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.  */
+      do_cleanups (c);
+      return 1;
+    }
+
+  /* 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 +1350,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 20da81a..3921034 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -5471,39 +5471,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

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

[Pedro Alves]

The subject is a bit misleading, as this does more than
just refactoring PRPSINFO handling; it dumps more fields
as well.  Splitting the refactoring and the actual
extending into separate patches, along with a short
description of what's new in the cores would have been neat,
but I'm coming back late to the party, and don't insist.

> +  /* 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 ();
> +    }
> +

Hmm?  We were not doing that before.  What exception is that?

-- 
Pedro Alves

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

[Sergio Durigan Junior]

On Thursday, January 10 2013, Pedro Alves wrote:

> The subject is a bit misleading, as this does more than
> just refactoring PRPSINFO handling; it dumps more fields
> as well.  Splitting the refactoring and the actual
> extending into separate patches, along with a short
> description of what's new in the cores would have been neat,
> but I'm coming back late to the party, and don't insist.

Wow, OK, sorry about that, it wasn't my intention.

Anyway, at first I thought that the extension could be seen as
refactoring as well, that's why I didn't bother making another patch.

>> +  /* 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 ();
>> +    }
>> +
>
> Hmm?  We were not doing that before.  What exception is that?

`get_inferior_args' calls `construct_inferior_arguments', which can call
`error' in an specific scenario (not STARTUP_WITH_SHELL, arguments that
contain spaces).

-- 
Sergio

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

[Pedro Alves]

On 01/11/2013 03:53 AM, Sergio Durigan Junior wrote:

> Anyway, at first I thought that the extension could be seen as
> refactoring as well, that's why I didn't bother making another patch.

http://en.wikipedia.org/wiki/Code_refactoring

"Code refactoring is a \"disciplined technique for restructuring an existing
body of code, altering its internal structure without changing
its external behavior\", undertaken in order to improve some of the
nonfunctional attributes of the software. "

;-)

> 
>>> +  /* 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 ();
>>> +    }
>>> +
>>
>> Hmm?  We were not doing that before.  What exception is that?
> 
> `get_inferior_args' calls `construct_inferior_arguments', which can call
> `error' in an specific scenario (not STARTUP_WITH_SHELL, arguments that
> contain spaces).

This is an example of something that should be split into
its own change, along with its own rationale.  This is
independent of any refactoring of PRPSINFO handling.
We're already calling get_inferior_args nowadays, and I don't
ever remember this error being reported as a problem.
The reason must be that STARTUP_WITH_SHELL is always defined
to 1 nowadays.  It hasn't been zapped because it'd actually
be nice to make it a user setting.  And once that is done,
we'd indeed trip on that error.

In any case, it seems to me that that error/exception/limitation
could/should be lifted by making construct_inferior_arguments
quote the arguments with spaces.

-- 
Pedro Alves

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

[Sergio Durigan Junior]

On Friday, January 11 2013, Pedro Alves wrote:

> On 01/11/2013 03:53 AM, Sergio Durigan Junior wrote:

>>>> +  /* 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 ();
>>>> +    }
>>>> +
>>>
>>> Hmm?  We were not doing that before.  What exception is that?
>> 
>> `get_inferior_args' calls `construct_inferior_arguments', which can call
>> `error' in an specific scenario (not STARTUP_WITH_SHELL, arguments that
>> contain spaces).
>
> This is an example of something that should be split into
> its own change, along with its own rationale.  This is
> independent of any refactoring of PRPSINFO handling.
> We're already calling get_inferior_args nowadays, and I don't
> ever remember this error being reported as a problem.

My first version of the patch didn't contain the TRY_CATCH part.  It was
Jan who made this suggestion, and I thought it made sense.

I really think a TRY_CATCH does not cause any harm here, but if you
insist, I can easily remove it from the patch.

-- 
Sergio

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

[Jan Kratochvil]

On Fri, 11 Jan 2013 18:03:01 +0100, Sergio Durigan Junior wrote:
> On Friday, January 11 2013, Pedro Alves wrote:
> > On 01/11/2013 03:53 AM, Sergio Durigan Junior wrote:
> >>>> +  /* 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 ();
> >>>> +    }
> >>>> +
> >>>
> >>> Hmm?  We were not doing that before.  What exception is that?
> >> 
> >> `get_inferior_args' calls `construct_inferior_arguments', which can call
> >> `error' in an specific scenario (not STARTUP_WITH_SHELL, arguments that
> >> contain spaces).
> >
> > This is an example of something that should be split into
> > its own change, along with its own rationale.  This is
> > independent of any refactoring of PRPSINFO handling.
> > We're already calling get_inferior_args nowadays, and I don't
> > ever remember this error being reported as a problem.
> 
> My first version of the patch didn't contain the TRY_CATCH part.  It was
> Jan who made this suggestion, and I thought it made sense.
> 
> I really think a TRY_CATCH does not cause any harm here, but if you
> insist, I can easily remove it from the patch.

I do not think there should be code leaking memory in a case of throwsn
exception when the callee contains an error() call.  Even if the error() call
is only in a conditional which with the current setup around can never happen.

I agree one can improve get_inferior_args in a way it no longer throws.
That would be another way to fix it.  I do not mind which way the memory leak
of linux_nat_fill_prpsinfo gets fixed.


Thanks,
Jan

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

[Pedro Alves]

On 01/11/2013 05:11 PM, Jan Kratochvil wrote:
> On Fri, 11 Jan 2013 18:03:01 +0100, Sergio Durigan Junior wrote:
>> On Friday, January 11 2013, Pedro Alves wrote:
>>> On 01/11/2013 03:53 AM, Sergio Durigan Junior wrote:
>>>>>> +  /* 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 ();
>>>>>> +    }
>>>>>> +
>>>>>
>>>>> Hmm?  We were not doing that before.  What exception is that?
>>>>
>>>> `get_inferior_args' calls `construct_inferior_arguments', which can call
>>>> `error' in an specific scenario (not STARTUP_WITH_SHELL, arguments that
>>>> contain spaces).
>>>
>>> This is an example of something that should be split into
>>> its own change, along with its own rationale.  This is
>>> independent of any refactoring of PRPSINFO handling.
>>> We're already calling get_inferior_args nowadays, and I don't
>>> ever remember this error being reported as a problem.
>>
>> My first version of the patch didn't contain the TRY_CATCH part.  It was
>> Jan who made this suggestion, and I thought it made sense.

Sorry, I missed that.

>> I really think a TRY_CATCH does not cause any harm here, but if you
>> insist, I can easily remove it from the patch.
> 
> I do not think there should be code leaking memory in a case of throwsn
> exception when the callee contains an error() call.  

Agreed, of course.

> Even if the error() call is only in a conditional which with
> the current setup around can never happen.

Agree.  IMO, strictly considering the leak, a cleanup for whatever
is leaking would be better though.

> I agree one can improve get_inferior_args in a way it no longer throws.
> That would be another way to fix it.  I do not mind which way the memory leak
> of linux_nat_fill_prpsinfo gets fixed.

-- 
Pedro Alves