From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 74550 invoked by alias); 15 Jan 2016 20:23:13 -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 73828 invoked by uid 89); 15 Jan 2016 20:23:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.1 required=5.0 tests=AWL,BAYES_20,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=auxv, gross, HX-Greylist:succeeded, HX-Greylist:SMTP X-Spam-User: qpsmtpd, 2 recipients X-HELO: bigwig.baldwin.cx Received: from bigwig.baldwin.cx (HELO bigwig.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Fri, 15 Jan 2016 20:23:09 +0000 Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 560CBB997; Fri, 15 Jan 2016 15:23:07 -0500 (EST) From: John Baldwin To: Pedro Alves Cc: gdb-patches@sourceware.org, binutils@sourceware.org Subject: Re: [PATCH v2 3/6] Display per-thread information for threads in FreeBSD cores. Date: Fri, 15 Jan 2016 20:23:00 -0000 Message-ID: <2320320.x23qbZCVbY@ralph.baldwin.cx> User-Agent: KMail/4.14.3 (FreeBSD/10.2-STABLE; KDE/4.14.3; amd64; ; ) In-Reply-To: <5697B8D6.9010301@redhat.com> References: <1452721551-657-1-git-send-email-jhb@FreeBSD.org> <1452721551-657-4-git-send-email-jhb@FreeBSD.org> <5697B8D6.9010301@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00330.txt.bz2 On Thursday, January 14, 2016 03:03:50 PM Pedro Alves wrote: > On 01/13/2016 09:45 PM, John Baldwin wrote: > > > > > > +/* This is how we want PTIDs from core files to be printed. */ > > + > > +static char * > > +fbsd_core_pid_to_str (struct gdbarch *gdbarch, ptid_t ptid) > > +{ > > + static char buf[80], name[64]; > > This static "name" buffer appears unused, and then masked > by the "name" pointer below. Yes, it was from an earlier rev of the patch before I switched to alloca(). Removed. > > > + struct bfd_section *section; > > + bfd_size_type size; > > + char sectionstr[32]; > > + > > + if (ptid_get_lwp (ptid) != 0) > > + { > > + snprintf (sectionstr, sizeof sectionstr, ".thrmisc/%ld", > > + ptid_get_lwp (ptid)); > > xsnprintf. Fixed here and throughout. > > + section = bfd_get_section_by_name (core_bfd, sectionstr); > > + if (section != NULL) > > + { > > + char *name; > > + > > + size = bfd_section_size (core_bfd, section); > > + name = alloca (size + 1); > > + if (bfd_get_section_contents (core_bfd, section, name, (file_ptr) 0, > > + size) && name[0] != '\0') > > This indentation / line break reads unusual to me. I think breaking > before the && would be clearer: > > if (bfd_get_section_contents (core_bfd, section, name, > (file_ptr) 0, size) > && name[0] != '\0') > > Guess this should check size > 0 as well, otherwise name[0] contains > garbage. Ok. I decided to check the size in the earlier if statement to avoid the alloca(), etc. when the size is zero. > > + { > > + name[size] = '\0'; > > + if (strcmp(name, elf_tdata (core_bfd)->core->program) != 0) > > Missing space after strcmp. Fixed. > Is this ".thrmisc" section documented somewhere? Could you add a > small comment on what this entry you're skipping means? It is not documented aside from the code unfortunately. :( If I had my way we would be dumping the full ptrace_lwpinfo structure that contains per-LWP info used by the 'nat' target instead (it has other potentially useful information such as the siginfo_t for each thread). At the moment I don't generate a NT_THRMISC note in 'gcore' because 'struct thrmisc' is currently only present in FreeBSD's headers. A somewhat related question: would it be reasonable to dump additional notes in 'gcore' only when native? For example, FreeBSD kernels dump several additional notes including things like auxv, open files, etc. The "gross" way I can think of for doing that is to simply place that additional logic under #ifdef __FreeBSD__ in fbsd-tdep.c, but that seems quite hackish given the otherwise clean split between tdep.c and nat.c. -- John Baldwin