From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12511 invoked by alias); 25 Dec 2012 17:38:31 -0000 Received: (qmail 12343 invoked by uid 22791); 25 Dec 2012 17:38:30 -0000 X-SWARE-Spam-Status: No, hits=-6.1 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_AJ,TW_CP,TW_JF,TW_XS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 25 Dec 2012 17:38:19 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qBPHcHXH027158 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 25 Dec 2012 12:38:17 -0500 Received: from psique (ovpn-113-50.phx2.redhat.com [10.3.113.50]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qBPHcDiJ002640; Tue, 25 Dec 2012 12:38:14 -0500 From: Sergio Durigan Junior To: Jan Kratochvil Cc: GDB Patches , Binutils Development , Pedro Alves , "H.J. Lu" Subject: Re: [PATCH/RFC 02/02 v2] Refactor PRPSINFO handling on GDB References: <20121218171555.GA19639@host2.jankratochvil.net> X-URL: http://www.redhat.com Date: Tue, 25 Dec 2012 17:38:00 -0000 In-Reply-To: <20121218171555.GA19639@host2.jankratochvil.net> (Jan Kratochvil's message of "Tue, 18 Dec 2012 18:15:55 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-12/txt/msg00813.txt.bz2 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 > > 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 @@ -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