From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31700 invoked by alias); 14 Sep 2009 17:34:24 -0000 Received: (qmail 31692 invoked by uid 22791); 14 Sep 2009 17:34:24 -0000 X-SWARE-Spam-Status: No, hits=-2.5 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; Mon, 14 Sep 2009 17:34:20 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id EA61D2BAAFA; Mon, 14 Sep 2009 13:34:18 -0400 (EDT) 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 PyOjzezapA98; Mon, 14 Sep 2009 13:34:18 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id AC15E2BAAC8; Mon, 14 Sep 2009 13:34:18 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id C29F1F589B; Mon, 14 Sep 2009 10:34:13 -0700 (PDT) Date: Mon, 14 Sep 2009 17:34:00 -0000 From: Joel Brobecker To: Michael Eager Cc: "gdb-patches@sourceware.org" Subject: Re: Support for Xilinx MicroBlaze architecture (1 of 3) Message-ID: <20090914173413.GJ8327@adacore.com> References: <4AAAA1CF.6080403@eagercon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4AAAA1CF.6080403@eagercon.com> User-Agent: Mutt/1.5.18 (2008-05-17) 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/msg00420.txt.bz2 > 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. -- Joel