From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18113 invoked by alias); 8 Dec 2010 21:25:51 -0000 Received: (qmail 18066 invoked by uid 22791); 8 Dec 2010 21:25:50 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 08 Dec 2010 21:25:44 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id B30D52BAACF; Wed, 8 Dec 2010 16:25:42 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id FHzCPvdS8J12; Wed, 8 Dec 2010 16:25:42 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 18A9D2BAAC0; Wed, 8 Dec 2010 16:25:41 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 92C43145B58; Wed, 8 Dec 2010 13:25:38 -0800 (PST) Date: Wed, 08 Dec 2010 21:25:00 -0000 From: Joel Brobecker To: Mike Frysinger Cc: gdb-patches@sourceware.org, Jie Zhang Subject: Re: [PATCH 1/2] gdb: bfin: new port Message-ID: <20101208212538.GA18023@adacore.com> References: <1289867547-19134-1-git-send-email-vapier@gentoo.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1289867547-19134-1-git-send-email-vapier@gentoo.org> User-Agent: Mutt/1.5.20 (2009-06-14) 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/msg00094.txt.bz2 Mike, 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. > 2010-11-16 Jie Zhang > > * configure.ac (bfin-*-*): Remove gdb from noconfigdirs. > * configure: Regenerate. This should be sent to gcc-patches.... > gdb/: > 2010-11-16 Jie Zhang > > * MAINTAINERS: Add Blackfin port. > * Makefile.in (ALLDEPFILES): Add bfin-tdep.c. > (HFILES_NO_SRCDIR): Add bfin-tdep.h. > * NEWS: Mention new Blackfin port. > * bfin-tdep.c, bfin-tdep.h, config/bfin/bfin.mt: New files. > * configure.tgt (bfin-*-*): Handle bfin targets. Minor comments below... > + bfin --target=bfin-elf ,-Werror > + Mike Frysinger vapier@gentoo.org > + 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? > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in I think you will also want to add bfin-tdep.o to ALL_TARGET_OBS. Otherwise, bfin gets left out when you build with --enable-targets=all. I don't see a reason why we shouldn't include it. > --- a/gdb/NEWS > +++ b/gdb/NEWS This part gets approved by Eli Zaretski, our documentation maintainer. Can you maybe ping him about it? He has always been extremely responsive, so he probably just missed the fact that this patch had some documentation changes for him to review. > +#define NUM_BFIN_REGNAMES \ > + (sizeof (bfin_register_name_strings) / sizeof (char *)) There is a macro called ARRAY_SIZE that you can use... > +int map_gcc_gdb[] = Can this be made static? > +/* Return non-zero if PC points into the signal trampoline. For the > + sake of bfin_linux_get_sigtramp_info. */ This is a detail, but `nonzero' is spelled without the dash in the middle. It's a detail because it's spelled your way everywhere... > +static int > +bfin_linux_pc_in_sigtramp (struct frame_info *this_frame, CORE_ADDR pc) > +{ [...] > + gdb_byte buf[10]; 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'm also wondering whether you know about tramp-frame.h, and whether that might be sufficient for your needs... > + if (ret == 1) > + { > + /* Get sigcontext address. */ > + info.context_addr = sp + SIGCONTEXT_OFFSET; > + } > + else > + internal_error (__FILE__, __LINE__, _("not a sigtramp\n")); You should use a gdb_assert(), in this case. > + /* This would come after the LINK instruction in the ret_from_signal () > + function, hence the frame id would be SP + 8. */ Please remove the `()' - it is used only when talking about the function call... > + /* Either there is no prologue (frameless function) or we are at > + the start of a function. In short we do not have a frame. > + PC is stored in rets register. FP points to previous frame. */ Missing space after a period (2 instances). > + cache->saved_regs[BFIN_PC_REGNUM] = get_frame_register_unsigned (this_frame, BFIN_RETS_REGNUM); This line is too long (over 78 characters). > +#ifdef _DEBUG > + fprintf (stderr, "frameless pc case base %x\n", cache->base); > +#endif 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 (gdb) set debug bfin (see `set debug infrun' for instance). > +/* The following functions are for function prologue length calculations. */ Each function needs to be documented. > +static int > +is_minus_minus_sp (int op) Just out of curiosity: Why two 'minus' in the name? > +CORE_ADDR > +bfin_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc) This function should be made static. > + fprintf (stderr, "Function Prologue not recognised. pc will point to ENTRY_POINT of the function\n"); No call to fprintf. In this case, I think you can use the `warning' function. > +/* We currently only support passing parameters in integer registers. This > + conforms with GCC's default model. Several other variants exist and > + we should probably support some of them based on the selected ABI. */ I think that this comment will be less surprising if you place it at the start of the function, inside the function body... > + for (i = nargs - 1; i >= 0; i--) > + { > + struct type *value_type = value_enclosing_type (args[i]); > + int len = TYPE_LENGTH (value_type); > + total_len += (len + 3) & ~3; Empty line after local variable declarations. > + for (i = nargs - 1; i >= 0; i--) > + { > + struct type *value_type = value_enclosing_type (args[i]); > + struct type *arg_type = check_typedef (value_type); > + int len = TYPE_LENGTH (value_type); > + int container_len = (len + 3) & ~3; > + sp -= container_len; > + write_memory (sp, value_contents_writeable (args[i]), container_len); Likewise. > + if (reg > sizeof (map_gcc_gdb) / sizeof (int)) You can use ARRAY_SIZE... > +/* This function implements the BREAKPOINT_FROM_PC macro. It returns ^^^^ missing space > +const unsigned char * > +bfin_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr) Should be made static. > + and copy it into READBUF. If WRITEBUF is non-zero, write the value ^^^^^^^^ > +CORE_ADDR > +bfin_frame_align (struct gdbarch *gdbarch, CORE_ADDR address) Make static? > +enum gcc_regnum { > + BFIN_GCC_R0_REGNUM = 0, What is this used for? > +/* 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; > +}; Why is this used, since there is only one ABI? > +/* Return the Blackfin ABI associated with GDBARCH. */ > +static inline enum bfin_abi > +bfin_abi (struct gdbarch *gdbarch) > +{ > + return gdbarch_tdep (gdbarch)->bfin_abi; > +} Please - no code in .h files. I doubt that making that function inline is bringing any measurable benefit - is it? -- Joel