From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12242 invoked by alias); 18 Dec 2012 17:16:20 -0000 Received: (qmail 12217 invoked by uid 22791); 18 Dec 2012 17:16:17 -0000 X-SWARE-Spam-Status: No, hits=-6.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_AJ,TW_CP,TW_EG,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, 18 Dec 2012 17:16:05 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qBIHG4IF026419 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 18 Dec 2012 12:16:04 -0500 Received: from host2.jankratochvil.net (ovpn-116-39.ams2.redhat.com [10.36.116.39]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qBIHFuUd007109 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Tue, 18 Dec 2012 12:15:58 -0500 Date: Tue, 18 Dec 2012 17:16:00 -0000 From: Jan Kratochvil To: Sergio Durigan Junior Cc: GDB Patches , Binutils Development , Pedro Alves , "H.J. Lu" Subject: Re: [PATCH/RFC 02/02 v2] Refactor PRPSINFO handling on GDB Message-ID: <20121218171555.GA19639@host2.jankratochvil.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2012-12/txt/msg00190.txt.bz2 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 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