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

Jan Kratochvil
2012-12-31T19:41:54Z

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