From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8397 invoked by alias); 5 Jan 2006 18:51:01 -0000 Received: (qmail 8390 invoked by uid 22791); 5 Jan 2006 18:51:00 -0000 X-Spam-Check-By: sourceware.org Received: from zproxy.gmail.com (HELO zproxy.gmail.com) (64.233.162.197) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 05 Jan 2006 18:50:59 +0000 Received: by zproxy.gmail.com with SMTP id l1so3293164nzf for ; Thu, 05 Jan 2006 10:50:57 -0800 (PST) Received: by 10.36.24.7 with SMTP id 7mr7292733nzx; Thu, 05 Jan 2006 10:50:56 -0800 (PST) Received: by 10.37.2.42 with HTTP; Thu, 5 Jan 2006 10:50:56 -0800 (PST) Message-ID: <8f2776cb0601051050o7b1bbe09xa346220e0b526cbe@mail.gmail.com> Date: Thu, 05 Jan 2006 18:51:00 -0000 From: Jim Blandy To: Jie Zhang Subject: Re: [PATCH] Add support for Analog Devices Blackfin processor (part 1/6: gdb) Cc: gdb-patches@sources.redhat.com In-Reply-To: <43BD4C00.70608@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline References: <43B15F54.5040903@gmail.com> <8f2776cb0601032128o3f4ef886lfde9e1fcca2e3202@mail.gmail.com> <43BD4C00.70608@gmail.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-01/txt/msg00062.txt.bz2 On 1/5/06, Jie Zhang wrote: > Here the a new patch with the following changes according to your > suggestions. Wonderful! > > Am I reading bfin_frame_prev_register correctly when I conclude that > > the only saved registers it can find are the PC and the FP? How are > > It can also find other registers besides the PC and the FP. It looks like: > > if(regnum =3D=3D BFIN_PC_REGNUM) > { > ... > } > else if (regnum =3D=3D BFIN_FP_REGNUM) > { > ... > } > else > read_memory (...); Right, but that's all under the control of: if (regnum < BFIN_NUM_REGS && cache->saved_regs[regnum] !=3D -1) and if I'm reading the rest of the code right, cache->saved_regs[R] can only be !=3D -1 when R is BFIN_PC_REGNUM or BFIN_FP_REGNUM. But those test suite results look pretty good. Does the Blackfin GCC port produce Dwarf CFI? If so, then you're probably not exercising the tdep unwinding code much. Either way, this is a quality of implementation issue, not a coding convention or interface usage issue, so if it's not a problem for you (or someone willing to pay you), I don't think it needs to be addressed for the port to go in. So, now we can get to picky stuff: /* ??? */ #define UPPER_LIMIT (40) #define RETS_OFFSET 4 That comment should probably be filled in. /* This would come after the LINK instruction in the ret_from_signal () function, hence the frame id would be SP + 8. */ this_id =3D frame_id_build (sp + 8, frame_pc_unwind(next_frame)); GNU coding standards call for a space before the opening parenthesis of a function call argument list. /* Read the value in from memory. */ if(regnum =3D=3D BFIN_PC_REGNUM) Similarly for 'if', 'while', and 'for' conditions. I just searched in Emacs for the regexp '[a-z]('. static int bfin_sim_regno (int regno) { switch (regno) { case SIM_BFIN_ASTAT_REGNUM : case SIM_BFIN_CYCLES_REGNUM : case SIM_BFIN_CYCLES2_REGNUM : case SIM_BFIN_USP_REGNUM : The GNU coding standards have 'case' labels un-indented two spaces relative to the code they label, like this. static int bfin_sim_regno (int regno) { switch (regno) { case SIM_BFIN_ASTAT_REGNUM : case SIM_BFIN_CYCLES_REGNUM : case SIM_BFIN_CYCLES2_REGNUM : case SIM_BFIN_USP_REGNUM : case SIM_BFIN_SEQSTAT_REGNUM : Looking at: static void bfin_linux_sigtramp_frame_prev_register (struct frame_info *next_frame, void **this_cache, int regnum, int *optimizedp, enum lval_type *lvalp, CORE_ADDR *addrp, int *realnump, gdb_byte *valuep) When we have a long argument list like this, I think it's more legible to have each argument on its own line, or at least have line groupings reflect some sort of semantic grouping (an array and its length, for example). The bfin_frame_prev_register case is similar. This is up to you, though. /* Integral values greater than one word are stored in consecutive registers starting with R0. This will always be a multiple of the regiser size. */ Typo. Overall, though, it looks pretty clean to me.