From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3243 invoked by alias); 23 Nov 2012 19:34:10 -0000 Received: (qmail 3017 invoked by uid 22791); 23 Nov 2012 19:34:09 -0000 X-SWARE-Spam-Status: No, hits=-7.9 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS 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; Fri, 23 Nov 2012 19:34:00 +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 qANJXo76005754 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 23 Nov 2012 14:33:50 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qANJXfGI018053; Fri, 23 Nov 2012 14:33:43 -0500 Message-ID: <50AFCF95.1080809@redhat.com> Date: Fri, 23 Nov 2012 19:34:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: Andreas Tobler CC: gdb-patches@sourceware.org Subject: Re: [PATCH] FreeBSD powerpc support References: <50AAA80B.1000509@fgznet.ch> In-Reply-To: <50AAA80B.1000509@fgznet.ch> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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-11/txt/msg00630.txt.bz2 Hello, On 11/19/2012 09:43 PM, Andreas Tobler wrote: > Hi all, > > After a longer timeout here again :) > > The attached patch adds support for FreeBSD PowerPC, 32-bit and 64-bit. > It is derived from ppcobsd* and ppc-linux with FreeBSD additions. > > There is room for improvement :) And I will continue, but I'd like to > see this patch in the gdb cvs because it will be much easier for me fix > outstanding issues when I can work with cvs iso. of local revisions. Eh. If it's easier, then maybe you're not using the proper tools; there's always quilt. :-) Or better nowadays, you could also put it in a public git repo somewhere. We have a git mirror of cvs. That said, I'm really not against putting it in early, if it's not riddled with hacks. > Also, other people might have a use of this work, even if not complete. > > Currently missing/incomplete: > - Altivec support, kernel bits are missing. > - HW watchpoints, also kernel bits are missing. > - thread support. > - tls support. > - some sig tests. I've skimmed the patch, and didn't notice anything horrible. Then again, I'm on the low end of the scale that measures PowerPC or FreeBSD expertness... - Please make sure there's a blank line between introductory comment and function. - I noticed that a few functions don't have introductory comment. - If the function implements of a target/gdbarch/etc. method, then comment it as such. E.g., /* This is the implementation of gdbarch method FOOBAR. */ - I noticed some functions with long comments are copies of existing code of other ports. I wonder if we could perhaps share more code. > +/* Read a PPC instruction from memory. PPC instructions are always > + * big-endian, no matter what endianness the program is running in, so > + * we can't use read_memory_integer or one of its friends here. read_memory_unsigned_integer nowadays has a byte_order parameter, so just pass it BFD_ENDIAN_BIG, and you're set. > + */ > +static unsigned int > +read_insn (CORE_ADDR pc) > +{ > + unsigned char buf[4]; > + > + read_memory (pc, buf, 4); > + return (buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3]; > +} > + > + > +#define PPC64_STANDARD_LINKAGE2_LEN \ > + (sizeof (ppc64_standard_linkage2) / sizeof (ppc64_standard_linkage2[0])) > + Use the existing ARRAY_SIZE macro. +/* Signal trampolines. */ > + > +static int > +ppcfbsd_sigtramp_frame_sniffer (const struct frame_unwind *self, > + struct frame_info *this_frame, > + void **this_cache) > +{ > + struct gdbarch *gdbarch = get_frame_arch (this_frame); > + enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > + CORE_ADDR pc = get_frame_pc (this_frame); > + CORE_ADDR start_pc = (pc & ~(ppcfbsd_page_size - 1)); > + const int *offset; > + const char *name; > + > + find_pc_partial_function (pc, &name, NULL, NULL); > + if (name) > + return 0; For some reason this bailing out if name is not null jumped at me. It's not obvious to me what that means, so it felt like it deserves a comment. On 11/19/2012 09:43 PM, Andreas Tobler wrote: > --- configure.host 30 May 2012 19:41:34 -0000 1.107 > +++ configure.host 19 Nov 2012 21:24:15 -0000 > @@ -125,6 +125,7 @@ > > powerpc-*-aix* | rs6000-*-*) > gdb_host=aix ;; > +powerpc*-*-freebsd*) gdb_host=fbsd ;; This seems to be 'powerpc-*-freebsd*' elsewhere I looked (top level, bfd). Why the extra wildcard? -- Pedro Alves