From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 807 invoked by alias); 14 Sep 2009 17:39:04 -0000 Received: (qmail 799 invoked by uid 22791); 14 Sep 2009 17:39:03 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from shell4.BAYAREA.NET (HELO shell4.bayarea.net) (209.128.82.1) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 14 Sep 2009 17:38:57 +0000 Received: (qmail 21555 invoked from network); 14 Sep 2009 10:38:55 -0700 Received: from 209-128-106-254.bayarea.net (HELO redwood.eagercon.com) (209.128.106.254) by shell4.bayarea.net with SMTP; 14 Sep 2009 10:38:55 -0700 Message-ID: <4AAE7FAE.30602@eagercon.com> Date: Mon, 14 Sep 2009 17:39:00 -0000 From: Michael Eager User-Agent: Thunderbird 2.0.0.23 (X11/20090825) MIME-Version: 1.0 To: Joel Brobecker CC: "gdb-patches@sourceware.org" Subject: Re: Support for Xilinx MicroBlaze architecture (1 of 3) References: <4AAAA1CF.6080403@eagercon.com> <20090914173413.GJ8327@adacore.com> In-Reply-To: <20090914173413.GJ8327@adacore.com> Content-Type: text/plain; charset=UTF-8; format=flowed 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: 2009-09/txt/msg00422.txt.bz2 Joel Brobecker wrote: >> 2009-09-11 Michael Eager >> >> * configure.tgt: Add targets microblaze*-linux-*, microblaze*-xilinx-*. >> * doc/gdb.texinfo: Add MicroBlaze. >> * MAINTAINERS: Add self as maintainer for MicroBlaze. >> * Makefile.in: Build microblaze-tdep.o, microblaze-linux-tdep.o. > [...] > > It looks like this patch is against an older version of the debugger. > It makes references to entities or files that no longer exist. For > instance, it's references solib-legacy.c which was removed in 2007, > or BREAKPOINT_FROM_PC (use gdbarch_breakpoint_from_pc instead, since > 2007 as well). Can you resync your patch against our CVS HEAD and > resubmit? > > I did notice a couple of things while looking at the code: > >> +/* See ppc_linux_memory_remove_breakpoints for description. */ >> +int >> +microblaze_linux_memory_remove_breakpoint (struct bp_target_info *bp_tgt) >> +{ > > Because this is a target-independent implementation of a gdbarch > routine, you don't need to explain what this function does. You might > want to explain why it is necessary in your particular target, but > the comments inside the implementation seem to be clear enough as > to why the default implementation is not sufficient. So I would > drop the comment. > > The other thing I noticed is that this function should be made static. > >> + const unsigned char *bp; > > You should use gdb_byte > > I only glanced at the rest of the code. I noticed commented out code > which should be removed (#if 0 ... #endif). There is also a commented > out printf. Formatting issues, such as missing double-space after a > period: > >> + then our frame has not yet been set up. */ > > (I am guilty of making these mistakes myself) > > We prefer it if you have an empty line between variable declarations > and the rest of the code. > > You don't need to protect your debugging traces against MICROBLAZE_DEBUG. > If these traces are useful to diagnose an issue, then have them available > through a "set/show debug " switch. > >> +struct gdbarch_tdep >> +{ >> + int dummy; /* declare something. */ >> +}; > > I don't think you need to have any element in your tdep structure. Thanks. I'll take a look at these issues. -- Michael Eager eager@eagercon.com 1960 Park Blvd., Palo Alto, CA 94306 650-325-8077