From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3376 invoked by alias); 14 Oct 2009 01:47:14 -0000 Received: (qmail 3364 invoked by uid 22791); 14 Oct 2009 01:47:12 -0000 X-SWARE-Spam-Status: No, hits=-2.4 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, 14 Oct 2009 01:47:06 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 998092BAC33; Tue, 13 Oct 2009 21:47:04 -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 eRCpLurUX1nE; Tue, 13 Oct 2009 21:47:04 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 324532BAAAD; Tue, 13 Oct 2009 21:47:04 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 1B567F58A0; Tue, 13 Oct 2009 18:46:55 -0700 (PDT) Date: Wed, 14 Oct 2009 01:47:00 -0000 From: Joel Brobecker To: Michael Eager Cc: "gdb-patches@sourceware.org" , Eli Zaretskii Subject: Re: [PATCH] Support for Xilinx MicroBlaze Message-ID: <20091014014655.GL5272@adacore.com> References: <4ACA9EE8.1040007@eagercon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4ACA9EE8.1040007@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-10/txt/msg00291.txt.bz2 > 2009-10-05 Michael Eager > > * config/djgpp/fnchange.lst: Add translations for cpu-microblaze.c, > elf32-microblaze.c, microblaze-rom.c, microblaze-linux-tdep.c, > microblaze-tdep.h, microblaze-tdep.c, microblaze-opc.h, microblaze-opcm.h, > microblaze-dis.c, microblaze-dis.h, sim/microblaze, microblaze.h, and > microblaze.isa. > * 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. > HFILES_NO_SRCDIR: Add microblaze-tdep.h. > * microblaze-linux-tdep.c: New. > * microblaze-rom.c: New. > * microblaze-tdep.c: New. > * microblaze-tdep.h: New. > * NEWS: Announce Xilinx MicroBlaze support. Overall, this looks like really good work. I just have a few comments, and I think that the next iteration will be ready to be checked in. I'll try to review the next revision faster, althought I will be away for 10 days on a business trip next week. The indentation for the ChangeLog entry needs to be 8 character, and that will cause some of the lines to exceed 80 chars (I even prefer limiting this to something less, such as 78 characters for instance). As discussed privately, can you hold off on the following change in MAINTAINERS: > + Michael Eager eager@eagercon.com For the record, this is not because we refuse to acknowledge Michael's contribution of commitment to the port, but simply because this file is used to list the port maintainers that have been nominated by the GDB Global Maintainers. See for instance the recent nomination of Jan Kratochvil and Tristan Gingold. > + if (picobug_regnames[i] > + && (strcmp (picobug_regnames[i], name) == 0)) Just a nit, you have some unnecessary parens around strcmp (...) == 0. > +static CORE_ADDR > +microblaze_push_dummy_code (struct gdbarch *gdbarch, CORE_ADDR sp, > + CORE_ADDR funcaddr, > + struct value **args, int nargs, > + struct type *value_type, > + CORE_ADDR *real_pc, CORE_ADDR *bp_addr, > + struct regcache *regcache) The formatting of the last line is slightly off... > + microblaze_insn_debug (("MICROBLAZE: Scanning prologue: name=%s, " > + "func_addr=0x%x, stop=0x%x\n", > + name, (unsigned int) func_addr, > + (unsigned int) stop)); The extra parens are because the macro is defined without the parens in the call to printf: +#define microblaze_insn_debug(args) \ + { if (microblaze_debug) { printf_filtered args; } } I would add the parens in the macro definition in order to make the call to the macro more natural. I also recommend, for debugging traces, that you use printf_unfiltered. Having to hit enter at every new page when possibly generating tons of debugging traces can drive someone crazy :). Speaking of these debug traces, the macro name seems unnecessarily specialized, as it really is nothing more than a printf. In fact, you do not use it just a few lines below, but you still output the same type of trace with the same "MICROBLAZE: ..." format. What I personally do is define a ..._debug routine with a printf format such as: void wtx_opt_objfiles_debug (const char *format, ...) ATTR_FORMAT (printf, 1, 2); And with the following implementation: void wtx_opt_objfiles_debug (const char *format, ...) { if (debug_objfiles) { va_list args; va_start (args, format); printf_filtered ("DEBUG (objfiles): "); vprintf_filtered (format, args); printf_filtered ("\n"); va_end (args); } } (Gaaahhh, what did I just say about using printf_filtered). I like this approach, even though it's slightly more code, simply because it allows me to have consistent debug traces without having to test my debug flag. For instance, the following would become: > + if (microblaze_debug) > + printf_filtered ("MICROBLAZE: %08x %08lx\n", (unsigned int) pc, insn); microblaze_debug ("%08x %08lx\n", (unsigned int) pc, insn); Actually, there is a routine to print CORE_ADDR values: paddress. So it should be: microblaze_debug ("%s %08lx\n", paddress (pc), insn); > + cache->register_offsets[rd] = imm - cache->framesize; > + /* reg spilled after updating. */ Comments in GDB are placed before the associated code (unfortunately, if you ask me). > + microblaze_insn_debug ( > + ("MICROBLAZE: swi %d %d %d, continuing\n", rd, ra, imm)); It would look better if the parens on the first line did not terminate the line. This is just a general remark, as you won't need 2 parens if you fix the macro or adopt the ..._debug function or macro as I suggest above. > + if (!(op == 0x26 || op == 0x27 || op == 0x2d || op == 0x2e || op == 0x2f)) You may prefer to use if (op != 0x26 && op != 0x27 && ..) instead of "if (!(op == 0x26 || op == 0x27 || ...))" which I find slightly confusing because you have to remember to keep reading until you find the end of the expression to which the "!" applies. But this is a matter of personal taste, so feel free to chose the version you prefer. > + /* For sentinel frame, return address is actual PC. For other frames, > + return address is pc+8. This is a workaround because gcc does not > + generate correct return address in CIE. */ You do what you have to do, but this is *BAD* idea (IMO). Unless you can detect the case when GCC generates bad return addresses or not in CIE, you'll end up having a broken debugger as soon as the compiler gets fixed. Introducing work arounds for compiler deficiencies is often fine, but I don't think that this should be done at the cost of proper operation. > + case 1: /* return last byte in the register. */ > + regcache_cooked_read (regcache, MICROBLAZE_RETVAL_REGNUM, buf); > + memcpy(valbuf, buf + MICROBLAZE_REGISTER_SIZE - 1, 1); > + return; > + case 2: /* return last 2 bytes in register. */ > + memcpy(valbuf, buf + MICROBLAZE_REGISTER_SIZE - 2, 2); > + return; > + case 4: /* for sizes 4 or 8, copy the required length. */ > + case 8: > + regcache_cooked_read (regcache, MICROBLAZE_RETVAL_REGNUM, buf); > + regcache_cooked_read (regcache, MICROBLAZE_RETVAL_REGNUM+1, buf+4); > + memcpy (valbuf, buf, TYPE_LENGTH (type)); > + return; > + default: > + fatal (_("Unsupported return value size requested")); fatal is verbotten except in a few specific cases. Can you use internal_error instead? > +int > +microblaze_can_use_hardware_watchpoints (enum bptype type, int count, int ot) This function appears to be unused? Let's delete it if that is the case. > +int > +microblaze_software_single_step (struct frame_info *frame) Can this function be made static? > + struct regcache *regcache = get_current_regcache (); This one raised a red flag, as we try to avoid depending on global variables. But I'm not sure what the kosher way of getting the regcache would be. I thought there would be method to get the regcache from a frame, but apparently not. Perhaps the right way is to use get_thread_arch_regcache (inferior_ptid, gdbarch), but I'm not sure. I'll ask Ulrich, who knows this area a lot better. > +int > +microblaze_frame_num_args_p (struct gdbarch *gdbarch) Looks unused... > + /* Debug this files internals. */ Tiny nit: Missing second space after the period. > +extern CORE_ADDR microblaze_analyze_prologue (CORE_ADDR, CORE_ADDR, > + struct microblaze_frame_cache *); Let's make that function static and remove that declaration. -- Joel