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

Sergio Durigan Junior
2012-12-25T17:38:31Z

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