From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24399 invoked by alias); 26 Sep 2009 00:08:20 -0000 Received: (qmail 24384 invoked by uid 22791); 26 Sep 2009 00:08:19 -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; Sat, 26 Sep 2009 00:08:13 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id E70382BAB53; Fri, 25 Sep 2009 20:08:11 -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 0S6dWvP6oluF; Fri, 25 Sep 2009 20:08:11 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 6E8982BAB2F; Fri, 25 Sep 2009 20:08:11 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 40AC7F593C; Fri, 25 Sep 2009 17:08:08 -0700 (PDT) Date: Sat, 26 Sep 2009 00:08:00 -0000 From: Joel Brobecker To: Michael Eager Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH] Add support for Xilinx MicroBlaze Message-ID: <20090926000808.GI5077@adacore.com> References: <4ABA69CE.5040707@eagercon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4ABA69CE.5040707@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/msg00828.txt.bz2 > * 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. > * microblaze-linux-tdep.c: New. > * microblaze-rom.c: New. > * microblaze-tdep.c: New. > * microblaze-tdep.h: New. > * NEWS: Announce Xilinx MicroBlaze support. In the makefile, you'll need to add microblaze-tdep.h to the HFILES_NO_SRCDIR list. Not strictly needed in order to build GDB, but this is used by the developers who use ctags/etags. > + Copyright 2009 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 2 of the License, or > + (at your option) any later version. The copyright header is out of date. It should refer to version 3 of the GPL. Any C file in the GDB sources should have the correct copyright header. Can you update all the new files that you are submitting? > + if ((val == 0) && (memcmp (bp, old_contents, bplen) == 0)) The extra parens should be removed. We discussed this with other Global Maintainers, and we all felt that this difference in style made the code harder to read, and thus should be avoided. So it should be: if (val == 0 && memcmp (bp, old_contents, bplen) == 0) Can you fix the rest of the code accordingly? > +static struct tramp_frame microblaze_linux_sighandler_tramp_frame = { Just to be consistent, this curly brace should be on the next line. > + gdbarch_register_osabi (bfd_arch_microblaze, 0, > + GDB_OSABI_LINUX, microblaze_linux_init_abi); Nit: formatting is wrong on the second line. > +/* Picobug only supports a subset of registers from MCore. In reality, > + it doesn't support ss1, either. */ Formatting nit: 2 spaces after the period. > +void > +picobug_open (char *args, int from_tty) Can this be made static? > +/* FIXME - this shouldn't be here */ > +#include "../opcodes/microblaze-opcm.h" FIXME? If you remove the "../", I *think* it's fine to include opcodes headers files from GDB code. > +extern enum microblaze_instr get_insn_microblaze (long, bfd_boolean *, > + enum microblaze_instr_type *, short *); > +extern unsigned long microblaze_get_target_address (long, bfd_boolean, int, long, long, > + long, bfd_boolean *, bfd_boolean *); > +extern enum microblaze_instr microblaze_decode_insn (long, int *, int *, > + int *, int *); Can we avoid these extern declarations, though? It would be nice if we could get a warning if the implementation of these functions changed. This won't happen with these locally-defined externs. > +#define mb_error(msg) internal_error (__FILE__, __LINE__, _(msg)) I don't like this define much, because an outsider inspecting part of your code might not realize that mb_error is actually an internal error. But if you prefer it that way... > +static void > +microblaze_dump_insn (char *commnt, CORE_ADDR pc, int insn) > +{ > + if (microblaze_debug) > + { > + printf_filtered ("MICROBLAZE: %s %08x %08x ", > + commnt, (unsigned int) pc, (unsigned int) insn); > + /* print_insn_microblaze (pc, &tm_print_insn_info); */ > + printf_filtered ("\n"); > + } > +} Er, this function does nothing functional, whereas I was expecting it to do something. But then, looking at the usage, it's just a debug function. This reminds me that all new functions should be documented with a comment at the beginning of their implementation. There are exceptions, however; All the routines that implement gdbarch methods or target methods are already documented as part of either gdbarch or target. Same for solib vectors, etc. So you don't need to provide the overal documentation for the functions used in your particular target. In this case, the general style is usually to conditionalize the call to that function to microblaze_debug, rather than having the condition inside the function. Some purists will say that this avoids a function call if debug is off, but I'll just say that it makes the naming of that function a littler easier :). If you prefer to keep the condition inside the function, not a problem, but please consider choosing a name that expresses this fact. > +#define microblaze_insn_debug(args) \ > + { if (microblaze_debug) printf_filtered args; } Perhaps if you added the braces around args in your call to printf_filtered, you wouldn't need the extra parens when using his macro? > +#define mb_warn(msg) \ > + if (microblaze_debug) printf ("mb_warning: %s:%d %s\n", __FILE__, __LINE__, _(msg)) > + Let's not use this home-made warning. See below. > +unsigned long > +microblaze_fetch_instruction (CORE_ADDR pc) [...] > + insn = 0; > + for (i = 0; i < sizeof (buf); i++) > + insn = (insn << 8) | buf[i]; > + return insn; You can replace all this by a call to extract_unsigned_integer... > + mb_warn ("push_dummy_code not implemented"); > + mb_warn ("store_arguments not implemented"); Please call "error" instead: The user will then be told that this feature is not implemented, and the action that required this function call will be automatically aborted as a result. > +/* Return a pointer to a string of bytes that encode a breakpoint > + instruction, store the length of the string in *LEN. */ As explained above, the comment is unnecessary; but if you decide to keep it, there is just one nit: you need an extra space after the period (I can reassure you that I make these mistakes myself, so you're not the only one commented on these). > +static const gdb_byte * > +microblaze_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pc, int *len) Formatting: This line is a little too long... > + The prologue ends when an instruction fails to meet either of > + these criteria. */ > +/* Analyze the prologue to determine where registers are saved, > + the end of the prologue, etc. Return the address of the first line > + of "real" code (i.e., the end of the prologue). */ > + /* Find the start of this function. */ Missing spaces after periods. There are other instances of this, but I stopped pasting them here. Can you make sure you take care of them too? > + bfd_boolean save_hidden_pointer_found = FALSE; > + bfd_boolean non_stack_instruction_found = FALSE; We tend to prefer using integers for predicates. > + cache->framesize = -1 * imm; /* stack grows towards low memory */ Can't we just do -imm? > + /* FIXME: Rework this code and add comments. */ Please consider doing this now :) > + default: > + printf_filtered("Fatal error: unsupported return value size " > + "requested (%s @ %d)\n", __FILE__, __LINE__); This should be replaced by a call to "error". > + Values less than 32 bits (short, boolean) are stored in r2, right > + justified and sign or zero extended. FIXME It would be nice to have the FIXME addressed now. > +static int > +microblaze_stabs_argument_has_addr (struct gdbarch *gdbarch, struct type *type) > +{ > + return (TYPE_LENGTH (type) == 16); /* FIXME */ Can you take care of this FIXME as well? If you don't know whether you have anything to fix or not, just put a note saying that, as far as you know, the only matching case is when TYPE_LENGTH is 16. I'm surprised you even need this at all. Are you using stabs? > +microblaze_can_use_hardware_watchpoints (enum bptype type, int len, int ot) I assume you have unlimited number of hardware breakpoint/watchpoints? How nice. > + bfd_boolean isunsignednum; Can you rename this variable to is_unsingned_num? It's so much more readable that way... > + bfd_boolean unconditionalbranch; > + microblaze_decode_insn(insn, &lrd, &lra, &lrb, &limm); Can you use an empty line between the last declaration and the first line of code? > + { > + stepbreaks[1].valid = TRUE; > + } Another small nit. We prefer not to use the curly braces if the if block contains only one statement. > +static int > +microblaze_dwarf2_reg_to_regnum (struct gdbarch *gdbarch, int reg) > +{ > + return dwarf2_to_reg_map[reg]; Can you add a call to gdb_assert that verifies that REG is within the bounds of dwarf2_to_reg_map? > +#undef MICROBLAZE_DEBUG > +#ifdef MICROBLAZE_DEBUG > +int microblaze_debug = 1; > +#endif You probably meant to remove this part. > +#define RWSDP_REGNUM 13 > +#define LINK_REGNUM 15 > +#define PR_REGNUM 15 > +#define FIRST_ARGREG 5 > +#define LAST_ARGREG 10 > +#define RETVAL_REGNUM 3 Just a thought: Why not use the register-name litterals that you just defined above? Wouldn't that be clearer? > +/* All registers are 32 bits. */ > +#define REGISTER_SIZE 4 This is slightly paranoin, but we used to have a REGISTER_SIZE macro that corresponded to a call to gdbarch_register_size. Since you are defining this macro in a header file, can you rename it to MICROBLAZE_REGISTER_SIZE? I just want to avoid the confusion as well as avoid a clash with other parts of the code. > +#define MY_FRAME_IN_SP 0x1 > +#define MY_FRAME_IN_FP 0x2 Can you either move these macros to microblaze-tdep.c or prefix them with MICROBLAZE_? Note that you have a second identical definition of these macros just a few lines below. The compiler usually emits a warning about that... > +#define NO_MORE_FRAMES 0x4 This one does not appear to be used anywhere. It also appears to be duplicated. > +#define IS_RETURN(op) (op == rtsd || op == rtid) > +#define IS_UPDATE_SP(op, rd, ra) \ > + ((op == addik || op == addi) && rd == REG_SP && ra == REG_SP) > +#define IS_SPILL_SP(op, rd, ra) \ > + ((op == swi || op == sw) && rd == REG_SP && ra == REG_SP) > +#define IS_SPILL_REG(op, rd, ra) \ > + ((op == swi || op == sw) && rd != REG_SP && ra == REG_SP) > +#define IS_ALSO_SPILL_REG(op, rd, ra, rb) \ > + ((op == swi || op == sw) && rd != REG_SP && ra == 0 && rb == REG_SP) > +#define IS_SETUP_FP(op, ra, rb) \ > + ((op == add || op == addik || op == addk) && ra == REG_SP && rb == 0) > +#define IS_SPILL_REG_FP(op, rd, ra, fpregnum) \ > + ((op == swi || op == sw) && rd != REG_SP && ra == fpregnum && ra != 0) > +#define IS_SAVE_HIDDEN_PTR(op, rd, ra, rb) \ > + ((op == add || op == addik) && ra == FIRST_ARGREG && rb == 0) Same for these, I think they should be moved to a .c file or prefixed by MICROBLAZE_ > +/* BREAKPOINT defines the breakpoint that should be used. */ > +#ifdef __MICROBLAZE_UCLINUX__ > +#define BREAKPOINT {0xb9, 0xcc, 0x00, 0x60} > +#else > +#define BREAKPOINT {0x98, 0x0c, 0x00, 0x00} > +#endif Same remark about the name of the macro. But a bigger concern is the fact that the macro is statically defined based on what I am guessing is a host-specific macro. This is a target-specific file. This goes against what we call our "multi-arch" design. Is there no way for you to determine that at run time? For instance, is there any microblaze-linux other than uclinux? If the breakpoint instruction on linux is always {0xb9, 0xcc, 0x00, 0x60} while it is {0x98, 0x0c, 0x00, 0x00} on the rest of the targets, that's easy to fix. Otherwise, we'll have to come up with another form of detection. -- Joel