From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31376 invoked by alias); 23 Aug 2018 10:40:18 -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 31366 invoked by uid 89); 23 Aug 2018 10:40:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=packing, HTo:D*fr, sk:contact, ENOMEM 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; Thu, 23 Aug 2018 10:40:14 +0000 Received: from dhcp-10-248-98-203.eduroam.wireless.private.cam.ac.uk (global-5-141.nat-2.net.cam.ac.uk [131.111.5.141]) by mail.baldwin.cx (Postfix) with ESMTPSA id 29E2A10AFCD; Thu, 23 Aug 2018 06:40:12 -0400 (EDT) Subject: Re: [PATCH v3] Generate NT_PROCSTAT_{AUXV,VMMAP,PS_STRINGS} in FreeBSD coredumps To: Simon Ser , gdb-patches@sourceware.org References: <_LRTDYZchE3m5uYvETytJBLgsrWRhqIDAXHnzDsU6kJBhItMetbpfxGe8OJD9a4K4b9brDxF9YvXWOzdDfTRBRfrZI-4XLl8mWwlrEpqmm0=@emersion.fr> From: John Baldwin Message-ID: <2058f9e1-906d-3c0f-c0b4-e33a7d9ad51d@FreeBSD.org> Date: Thu, 23 Aug 2018 10:40:00 -0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg00563.txt.bz2 On 8/21/18 3:45 PM, Simon Ser wrote: > gcore generates NT_AUXV and NT_FILE notes for Linux targets. On > FreeBSD auxv is stored in a NT_PROCSTAT_AUXV section, file mappings > are stored in a NT_PROCSTAT_VMMAP and both are prefixed with the > struct size. > > 2018-08-21 Simon Ser > * target.h (enum target_object): add FreeBSD-specific > TARGET_OBJECT_FREEBSD_VMMAP and TARGET_OBJECT_FREEBSD_PS_STRINGS > * fbsd-nat.c (fbsd_nat_target::xfer_partial): add support for > TARGET_OBJECT_FREEBSD_VMMAP and TARGET_OBJECT_FREEBSD_PS_STRINGS > * fbsd-tdep.c (fbsd_make_corefile_notes): write NT_PROCSTAT_AUXV, > NT_PROCSTAT_VMMAP and NT_PROCSTAT_PS_STRINGS notes > --- > > Changes from v2 to v3: > - Remove constants, prepend struct size for FreeBSD-specific objects > - Use gdbarch_ptr_bit instead of gdbarch_addr_bit > - Fixed typo > > This addresses all of John's comments except the VMMAP packed one. > > John, you said coredumps use the unpacked format, but looking at the > `gcore` utility source code (in usr.bin/gcore/elfcore.c) I believe it > uses the packed format for coredumps. It also makes more sense to me > to use the packed format for coredumps because it allows us not to > store PATH_MAX zeros for each entry. I may be wrong though, let me know > what you think. Ah, yes. We used to pad them, but now we pack them by default (the kernel has knobs to control packing in core dumps that default to packing). > > gdb/fbsd-nat.c | 50 ++++++++++++++++++++++++++++++++++++++++++ > gdb/fbsd-tdep.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++ > gdb/target.h | 4 ++++ > 3 files changed, 112 insertions(+) > > diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c > index 115deac0..7cd325e4 100644 > --- a/gdb/fbsd-nat.c > +++ b/gdb/fbsd-nat.c > @@ -751,6 +751,56 @@ fbsd_nat_target::xfer_partial (enum target_object object, > } > return TARGET_XFER_E_IO; > } > + case TARGET_OBJECT_FREEBSD_VMMAP: > + case TARGET_OBJECT_FREEBSD_PS_STRINGS: > + { > + gdb::byte_vector buf_storage; > + gdb_byte *buf; > + size_t buflen; > + int mib[4]; > + > + int proc_target; > + uint32_t struct_size; > + switch (object) { > + case TARGET_OBJECT_FREEBSD_VMMAP: > + proc_target = KERN_PROC_VMMAP; > + struct_size = sizeof (struct kinfo_vmentry); > + break; > + case TARGET_OBJECT_FREEBSD_PS_STRINGS: > + proc_target = KERN_PROC_PS_STRINGS; > + struct_size = sizeof (void *); > + break; > + } The indentation of this block seems off, perhaps it's not using tabs but only spaces? > + > + if (writebuf != NULL) > + return TARGET_XFER_E_IO; > + > + mib[0] = CTL_KERN; > + mib[1] = KERN_PROC; > + mib[2] = proc_target; > + mib[3] = pid; > + > + buflen = sizeof (struct_size) + offset + len; Presumably this should just be 'offset + len' as 'struct_size' is part of the logical stream described by 'offset + len', but see below where I think we need to rework buf_storage and buf. > + buf_storage.resize (buflen); > + buf = buf_storage.data (); > + > + memcpy (buf, &struct_size, sizeof (struct_size)); > + > + if (sysctl (mib, 4, buf + sizeof (struct_size), &buflen, NULL, 0) == 0) > + { Hmm, if the caller asks to read a subset of the "stream", then buflen will be too short and this will fail with ENOMEM or the like. (It seems I failed to handle this properly in the auxv case as well *sigh*.) Probably you need to follow the pattern that gcore uses in procstat_sysctl() in elfcore.c where you first request the length via a NULL buffer, then size buf_storage to buflen + sizeof struct size, then do the sysctl to fetch the entire buffer and finally do the memcpy out of buf into readbuf. > diff --git a/gdb/target.h b/gdb/target.h > index 18c4a84c..83f1172c 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -203,6 +203,10 @@ enum target_object > of the process ID of the process in question, in hexadecimal > format. */ > TARGET_OBJECT_EXEC_FILE, > + /* FreeBSD file mappings */ > + TARGET_OBJECT_FREEBSD_VMMAP, Perhaps 'virtual memory mappings' > + /* FreeBSD process strings */ > + TARGET_OBJECT_FREEBSD_PS_STRINGS, > /* Possible future objects: TARGET_OBJECT_FILE, ... */ > }; > > -- John Baldwin