From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15899 invoked by alias); 12 Dec 2012 20:07:21 -0000 Received: (qmail 15889 invoked by uid 22791); 12 Dec 2012 20:07:20 -0000 X-SWARE-Spam-Status: No, hits=-2.8 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.fgznet.ch (HELO smtp.fgznet.ch) (81.92.96.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 12 Dec 2012 20:07:15 +0000 Received: from deuterium.andreas.nets (dhclient-91-190-14-19.flashcable.ch [91.190.14.19]) by smtp.fgznet.ch (8.13.8/8.13.8/Submit_SMTPAUTH) with ESMTP id qBCK76Oe071221; Wed, 12 Dec 2012 21:07:08 +0100 (CET) (envelope-from andreast-list@fgznet.ch) Message-ID: <50C8E3EA.1050300@fgznet.ch> Date: Wed, 12 Dec 2012 20:07:00 -0000 From: Andreas Tobler User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:16.0) Gecko/20121026 Thunderbird/16.0.2 MIME-Version: 1.0 To: Pedro Alves CC: gdb-patches@sourceware.org Subject: Re: [PATCH] FreeBSD powerpc support References: <50AAA80B.1000509@fgznet.ch> <50AFCF95.1080809@redhat.com> <50B4FC29.9050006@fgznet.ch> <50C79283.1050608@redhat.com> In-Reply-To: <50C79283.1050608@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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-12/txt/msg00424.txt.bz2 On 11.12.12 21:07, Pedro Alves wrote: > On 11/27/2012 05:45 PM, Andreas Tobler wrote: > >>> - 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. >> >> True, here I do not know how to share, maybe a common ppc64-tdep-common.c? > > The tdep files are usually called ARCH-OS-tdep.c, so that would be > ppc64-common-tdep.c. That raises the question of what's different between > ppc64-common-tdep.c and ppc64-tdep.c. Clearer alternatives could be > ppc64-unix-tdep.c or some other token that describes the commonality between > what's being shared (as opposed to just the fact that something is shared, > as in "common"). In any case, the name of the file is the minor detail. Ok. I'll try to prepare something. Hopefully I'll find some time over the next weeks :) >>> read_memory_unsigned_integer nowadays has a byte_order parameter, >>> so just pass it BFD_ENDIAN_BIG, and you're set. >> >> Done. Even tested on ppc64-linux. I might send a patch for >> ppc-linux-tdep.c as well. Likewise for the below. > > That'd be nice. Ok, will do so. >>> 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. >> >> Also done, I hope I match the expectations. > > You have, thanks. > >> >>> 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? >> >> >> Hm, here I'm not sure. My targets report as powerpc-unknown-freebsd* >> (32-bit) and powerpc64-unknown-freebsd* (64-bit). So I thought I have to >> match both. Is this not correct? > > I guess that's fine then. > >> Attached a revised version of the diff. > > Thanks. Please always paste the ChangeLog entry along with the patch > too, even if it hadn't needed changes. Will do so next time. >> +#include "features/rs6000/powerpc-32l.c" >> +#include "features/rs6000/powerpc-64l.c" > > This is a problem. I notice you have tdesc related things in your patch, > but nothing is actually making use of the target descriptions -- neither > the core support, nor the native debugger support code is implementing > the "return target's target description" hooks. Furthermore, you're > using the target descriptions and calling the same initialization > functions that ppc-linux-tdep.c calls. Please try an --enable-targets=all > build -- I'm guessing you'll see multiple-definition link errors. Ok, I didn't try the enable-targets=all yet. I must have gotten something wrong here. Back when I started this patch I thought this was needed to pass the 'xml' tests. But now I see that I pass these test w/o this tdesc stuff. I removed it now. Pedro, thanks for the feedback. If you're ok I try to update the patch with a new file which contains the 'common' code for ppc64. Andreas