From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22414 invoked by alias); 9 Dec 2010 03:35:48 -0000 Received: (qmail 22287 invoked by uid 22791); 9 Dec 2010 03:35:47 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp.gentoo.org (HELO smtp.gentoo.org) (140.211.166.183) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 09 Dec 2010 03:35:42 +0000 Received: from vapier.localnet (localhost [127.0.0.1]) by smtp.gentoo.org (Postfix) with ESMTP id DF7091B4039; Thu, 9 Dec 2010 03:35:39 +0000 (UTC) From: Mike Frysinger To: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] gdb: bfin: new port Date: Thu, 09 Dec 2010 03:35:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.36; KDE/4.5.2; x86_64; ; ) Cc: Joel Brobecker References: <1289867547-19134-1-git-send-email-vapier@gentoo.org> <20101208212538.GA18023@adacore.com> In-Reply-To: <20101208212538.GA18023@adacore.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart1340838.SXGPa4aENU"; protocol="application/pgp-signature"; micalg=pgp-sha1 Content-Transfer-Encoding: 7bit Message-Id: <201012082235.28454.vapier@gentoo.org> 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: 2010-12/txt/msg00102.txt.bz2 --nextPart1340838.SXGPa4aENU Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-length: 4324 On Wednesday, December 08, 2010 16:25:38 Joel Brobecker wrote: > First of all, I apologize of the delay in reviewing this. Overall, > I think that this is very good work. I can't really help with the > correctness of the code, since I don't know the bfin architecture. np. if there's any feedback i didnt respond to here, it's because i just d= id=20 as asked since it made perfect sense. > > + bfin --target=3Dbfin-elf ,-Werror > > + Mike Frysinger vapier@gentoo.org > > + >=20 > We usually wait to see a few contributions before inviting a contributor > to become the official maintainer. So, would you mind taking that hunk > out of your patch? i thought people would want to know how to find out who knows the Blackfin= =20 code in case they had a question. ive made contributions in the past to gd= b,=20 but more so to the binutils project. but if you want me to leave it out fo= r=20 now, that's obviously your call. > > +static int > > +bfin_linux_pc_in_sigtramp (struct frame_info *this_frame, CORE_ADDR pc) > > +{ > [...] > > + gdb_byte buf[10]; >=20 > The use of that buffer is most peculiar. You first start by reading > a 2-byte instruction and storing it at buf+4. Then, depending on > the instruction, you you extract the other instruction either at > buf or buf+6. If it works, then no need to change... i think the idea is for buf[] to reflect the actual code to make debugging= =20 this code easier. i'm sure the buffer could be compacted, but i dont think= =20 it'd have any sort of profound savings, so i'd prefer to leave it be. > I'm also wondering whether you know about tramp-frame.h, and whether > that might be sufficient for your needs... this port was originally written against an older version of gdb which didn= t=20 have tramp-frame stuff. i'll log this into our tracker to get this updated. > > +#ifdef _DEBUG > > + fprintf (stderr, "frameless pc case base %x\n", cache->base); > > +#endif >=20 > Can you avoid the #ifdef? I personally that debugging logs are a good > idea. But I don't think they should only be activated (or not) at > compile time. The typical way of doing this is to conditionalize this > on a global variable controlled by a setting. For instance, how about >=20 > (gdb) set debug bfin >=20 > (see `set debug infrun' for instance). i'll drop it for now, but i like the idea of runtime debug in this code, an= d i=20 have more stuff i could sprinkle throughout. > > +static int > > +is_minus_minus_sp (int op) >=20 > Just out of curiosity: Why two 'minus' in the name? the actual insn is "[--SP] =3D ...;" > > +enum gcc_regnum { > > + BFIN_GCC_R0_REGNUM =3D 0, >=20 > What is this used for? that's a good question. looking through the history, it doesnt seem to be= =20 referenced when it was first added over 5 years ago. i cant really ask the= =20 guy who committed it as it was a contractor who hasnt worked for us in a ve= ry=20 long time. considering the current dwarf/gdb map seems to work, i'll just= =20 punt it. > > +/* The ABIs for Blackfin. */ > > +enum bfin_abi > > +{ > > + BFIN_ABI_FLAT > > +}; > > + > > +/* Target-dependent structure in gdbarch. */ > > +struct gdbarch_tdep > > +{ > > + /* Which ABI is in use? */ > > + enum bfin_abi bfin_abi; > > +}; >=20 > Why is this used, since there is only one ABI? i have a follow up patch to add the FDPIC ABI, but it requires rework in th= e=20 FRV FDPIC, and the frame parsing doesnt work quite right when going through= =20 the PLT after we updated from our older gdb version. since these issues ar= e=20 taking time, i'd like to get the core Blackfin code merged as people are us= ing=20 that today for various targets. > > +/* Return the Blackfin ABI associated with GDBARCH. */ > > +static inline enum bfin_abi > > +bfin_abi (struct gdbarch *gdbarch) > > +{ > > + return gdbarch_tdep (gdbarch)->bfin_abi; > > +} >=20 > Please - no code in .h files. it's used by both the core Blackfin code and the FDPIC solib code (which is= nt=20 part of this patch per above). i could duplicate the function in two files= ,=20 but it seems to make more sense to have one shared instance. > I doubt that making that function inline is bringing any measurable benef= it > - is it? it avoids warnings when people include this header but dont use the func -mike --nextPart1340838.SXGPa4aENU Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part. Content-length: 836 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) iQIcBAABAgAGBQJNAE6AAAoJEEFjO5/oN/WBF38P/2dyBqWnJ7JX5jggxEBfT/Tj W2SNXWX2o2iuBt4AqB0FkS9+2WkZUAlqd1CwW5XWRgf4e+TsAtiIqx54JSr2nQ69 J4XZUWTwo+K43HetcViykUqJBkq46dvJTDbDjPRHF6kcGc6wGf8tLx/Q0RUHvNqF 4fFV3Adw+Q2US6xrwMm2a/A3IciDEz+2oJp/g5b2hV997DhzqGhepBlPe0Ga99Om Ae+YpmpQ9H9Bg3iJz6EDv7ZR5KCim28aWSmSZRY7tThdsZTYuRcs7gJcD/hfHc+w dGygkYJBQSnsqHI4qNBXyVMXphE6nNchOVgU5Tzgjw9dtCZlpV/Kb/c7GZyeKrnS 2GMLhp9wfVM3urizmB7TVnsZvzei+4IpqqR6NrrFYlj5jM5RLX4MYGCE2Ty1CcyV 7EuMpolLuDUqhl7n9btxgr9Qc5dul+ZgFSNXpcDL1VM0wSM3Dpcknj+oZs0aCl8L 4RlasspTHW0/9PLUsK91Tm71H8ZKy/pKu0PDCRe4e1vAp6eKDkSIyxlvDMPWD4Vp HzYGh+lCrL/dv5oYuGM/mqZU8iFSchQk2crfzDY+/3V+ftBPDq3gsSP+JLhPPoku VCtjjCvX31bJeCM8BDTeAq7SUdPFm5W+YouPbGABN0NON8PDRerkfc11EzvVXzFJ AD/Yai2ubVidbU461ppY =m2Ne -----END PGP SIGNATURE----- --nextPart1340838.SXGPa4aENU--