From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 122657 invoked by alias); 5 Jan 2018 19:43:54 -0000 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 Received: (qmail 122644 invoked by uid 89); 5 Jan 2018 19:43:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy= X-HELO: mail.baldwin.cx Received: from bigwig.baldwin.cx (HELO mail.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 05 Jan 2018 19:43:52 +0000 Received: from ralph.baldwin.cx (astound-66-234-202-155.ca.astound.net [66.234.202.155]) by mail.baldwin.cx (Postfix) with ESMTPSA id 95E3F10AFAE; Fri, 5 Jan 2018 14:43:50 -0500 (EST) From: John Baldwin To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2 4/5] Support 'info proc' for native FreeBSD processes. Date: Fri, 05 Jan 2018 19:43:00 -0000 Message-ID: <2876637.AoPeRzWIMe@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.1-STABLE; KDE/4.14.30; amd64; ; ) In-Reply-To: References: <20180104014923.11899-1-jhb@FreeBSD.org> <20180104014923.11899-5-jhb@FreeBSD.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-IsSubscribed: yes X-SW-Source: 2018-01/txt/msg00117.txt.bz2 On Thursday, January 04, 2018 10:20:05 PM Simon Marchi wrote: > On 2018-01-03 08:49 PM, John Baldwin wrote: > > - Command line arguments are fetched via the kern.proc.args. > > sysctl. > > - The 'cwd' and 'exe' values are obtained from the per-process > > file descriptor table returned by kinfo_getfile() from libutil. > > - 'mappings' is implemented by walking the array of VM map entries > > returned by kinfo_getvmmap() from libutil. > > - 'status' output is generated by outputting fields from the structure > > returned by the kern.proc.pid. sysctl. > > - 'stat' is aliased to 'status'. > > Hi John, > > The patch LGTM in general, I noted 2 comments, the first one could be done > as a separate patch (after this series), if you agree with it. The second, > I'm fine if you just fix it before pushing. > > > + struct kinfo_vmentry *kve = vmentl.get (); > > + for (int i = 0; i < nvment; i++, kve++) > > + { > > + ULONGEST start, end; > > + > > + start = kve->kve_start; > > + end = kve->kve_end; > > +#ifdef __LP64__ > > + printf_filtered (" %18s %18s %10s %10s %9s %s\n", > > + hex_string (start), > > + hex_string (end), > > + hex_string (end - start), > > + hex_string (kve->kve_offset), > > + fbsd_vm_map_entry_flags (kve->kve_flags, > > + kve->kve_protection), > > + kve->kve_path); > > +#else > > + printf_filtered ("\t%10s %10s %10s %10s %9s %s\n", > > + hex_string (start), > > + hex_string (end), > > + hex_string (end - start), > > + hex_string (kve->kve_offset), > > + fbsd_vm_map_entry_flags (kve->kve_flags, > > + kve->kve_protection), > > + kve->kve_path); > > +#endif > > + } > > It might be a good idea to factor out the printing of vm map entries from here > and the core code, to make sure that they will always be printed the same way. > It's up to you. So I thought about that (and that's why the fbsd_vm_map_entry_flags is shared in fbsd-tdep.c). I would perhaps need some way to tell the common code which layout to use as it's based on gdbarch_addr_bit() in the tdep code but uses an #ifdef in this version. I also considered doing this for the 'status' implementation, but I would probably need to export the 'kinfo_proc_layout' structures from the tdep file and use an #ifdef in the native target to choose which layout to use. The 'status' is also a bit trickier as some fields are printed for native but not for cores, and the native version uses getpagesize() to convert page counts to Kb that the core version can't do. Merging the 'mappings' probably is more doable though than 'status' as it really just needs 'addr_bit' passed in. (I believe I can't quite pass in 'target_gdbarch()' since I might be attached to a 32-bit process but be running 'info proc mappings' with the PID of a 64-bit processes for which target_gdbarch() would be wrong.) > > + } > > + else > > + warning (_("unable to fetch virtual memory map")); > > + } > > +#endif > > + if (do_status) > > + { > > + if (!fbsd_fetch_kinfo_proc(pid, &kp)) > > Missing space here. But didn't we fetch it already (line 329)? > IIUC, we only need it in this scope, so you could move the > relevant variables here, and only call fbsd_fetch_kinfo_proc here. > I suppose it needed to be this way when stat and status were not > merged. Oh, whoops, I missed finishing that cleanup when collapsing down to a single 'status'. :-/ Yes, the intention was to remove 'kp_valid' entirely. -- John Baldwin