From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31387 invoked by alias); 7 Feb 2009 04:14:57 -0000 Received: (qmail 31377 invoked by uid 22791); 7 Feb 2009 04:14:54 -0000 X-SWARE-Spam-Status: No, hits=-0.9 required=5.0 tests=AWL,BAYES_40,J_CHICKENPOX_33 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, 07 Feb 2009 04:14:48 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 6D5A62A96E8; Fri, 6 Feb 2009 23:14:47 -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 PgmvC8BDjyEG; Fri, 6 Feb 2009 23:14:47 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id C8D542A96E7; Fri, 6 Feb 2009 23:14:46 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 9FDA2E7ACD; Fri, 6 Feb 2009 20:14:43 -0800 (PST) Date: Sat, 07 Feb 2009 04:14:00 -0000 From: Joel Brobecker To: Jon Beniston Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Add support for the Lattice Mico32 (LM32) architecture Message-ID: <20090207041443.GG3676@adacore.com> References: <266F97CB7CD14C6899877E7D69AA2030@bibi> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <266F97CB7CD14C6899877E7D69AA2030@bibi> User-Agent: Mutt/1.4.2.2i 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-02/txt/msg00173.txt.bz2 Jon, Sorry for the delay in reviewing this patch. This is a fairly big one, and it's not often that one has a big enough window to fit in this review :). On a side note, could you use "unified" diffs instead of using "context" diffs. Most of us here find the latter much harder to review (thanks!). First off, I'll say that I can't review the changes to the simulator. But I think we have a few fairly active maintainers for the simulator. I'd just resend this part as a separate patch with an explicit subject that says this is a simulator patch (with maybe a PING, this tends to wake people up). > gdb/ > 2008-12-15 Jon Beniston > > * MAINTAINERS: Add Jon Beniston as lm32 maintainer. I cannot approve this part yet, as I haven't had the change before this patch to review your code and make sure that you're able to follow the GDB conventions in your code. As you'll see, there are a few things that you didn't follow, so I think that it will be beneficial for you if we continue reviewing your patches before you commit. Given the nature of the issues I found, most are easy to learn and we should be able to nominate you soon. Also, I'll say it now: I had a lot of comments, and maybe it'll feel bad, but it should not feel that way. I don't think you're very far away from having something I can approve. A lot of it looks good to me in principle, and you're been commented on issues that should be easy to fix. > gdb/testsuite > 2008-12-15 Jon Beniston > > * gdb.asm/asm-source.exp: Add lm32 target. This part is fine. > include/gdb/ > 2008-12-15 Jon Beniston > > * sim-lm32.h: New file. Please make sure to ask the sim maintainer to approve this part... > ia64 --target=ia64-linux-gnu ,-Werror > (--target=ia64-elf broken) > > + lm32 --target=lm32-elf ,-Werror > + Jon Beniston, jon@beniston.com As much as it pains me to say this (I **HATE** tabs, I absolutely abhore tabs, I cannot believe that we're still using them. Aaaahhh, I feel better now): You need to use tabs in this case to be consistent with the rest of this section. Tabs before "lm32", and tabs between "lm32" and the --target. > --- 1299,1306 ---- > irix5-nat.c \ > libunwind-frame.c \ > linux-fork.c \ > ! lm32-linux-tdep.c lm32-tdep.c \ > ! m68hc11-tdep.c \ > m32r-tdep.c \ > m32r-linux-nat.c m32r-linux-tdep.c \ > m68k-tdep.c \ You should replace the spaces on the second line by a tab. > + lm32*-*-uclinux*) This is really nit-picking, but I looked at other GNU tools, and it looks like their configure scripts only recognize "lm32". I think we should be consistent and remove the first "*". Also, I'm wondering about only accepting "uclinux", as opposed to "*linux*". BFD, for instance, does not treat linux and uclinux differently. Unless you think that GDB will have to treat the two systems differently, perhaps it would make sense to follow BFD's example. > + gdb_target_obs="lm32-tdep.o lm32-linux-tdep.o" Tab instead of spaces... > + gdb_target_obs="lm32-tdep.o" Same here... I started reviewing this patch while reading it, and after reviewing the first file, I finally realized that this first file on the list is called lm32-linux-tdep.c. Purely from the contents of that file, I was convinced that I was reviewing lm32-tdep.c. I don't think there is anything in that file that is specific to linux. Is there? > + Copyright (C) 2008 Free Software Foundation, Inc. It's already 2009, so you'll need to add 2009 to the copyright date. > + #include Can you explain what this is for? Intuitively, I don't think this can be correct since you are including the signal.h from the host, which might be different from the one on the target... > + #include For portability reasons, GDB always includes "gdb_string.h". > + int lm32_software_single_step (struct frame_info *frame); I can't find the body for this function. Looks like your patch is incomplete??? I'm also a little bit surprised that this is unilaterally needed. Doesn't uclinux provide single-stepping, for instance? How about the simulator? > + enum { > + NORMAL_SIGTRAMP = 1, > + /* RT_SIGTRAMP = 2 */ > + }; As far as I can tell, this isn't really useful. I can only see this enum being used in one place, which is lm32_linux_pc_in_sigtramp, which is unused. It looks like either this is a left over that is no longer needed, or, as I suspect, there is a piece missing in your patch. > + #define LM32_ELF_NGREG 32 > + typedef unsigned char lm32_elf_greg_t[4]; I think that you might want to use gdb_byte instead of "unsigned char". > + #define LM32_UIMM16(insn) (insn & 0xffff) > + #define LM32_IMM26(insn) ((((long)insn & 0x3ffffff) << 6) >> 6) These two macros seem to be unused at the moment. It's OK to leave them if you think they might become useful at one point. Otherwise, let's delete them. > + struct lm32_unwind_cache To be consistent with GDB's usual naming conventions, would you mind renaming this structure to lm32_frame_cache, please? > + { > + /* The previous frame's inner most stack address. Used as this > + frame ID's stack_addr. */ > + CORE_ADDR prev_sp; This looked suspicious to me, and indeed, it looks like it's unused (apart from being computed). Contrary to what your comment implies, we usually use the cache->base as the frame's stack_addr... (which is why I found this field a little strange) > + /* Size of frame */ > + int size; Similarly, it looks like this information is only needed to compute the frame prev_sp. If this is the case, and assuming that prev_sp can go too, then this can been jettisoned as well. > + /* Whether the function uses fp as a frame pointer */ > + int uses_fp; Same here. This field is set, but doesn't seem to be ever used. > + static int > + lm32_linux_pc_in_sigtramp (CORE_ADDR pc, char *name) As said above, I suspect that you are missing a piece of code that uses this function. Otherwise, this function is unused and should be deleted. > + static void > + lm32_linux_supply_gregset (const struct regset *regset, > + struct regcache *regcache, > + int regnum, const void *gregs, size_t len) Tabs issue here again... > + fprintf(stderr, "%s:%d\n", __FILE__, __LINE__); I suspect that this trace wasn't supposed to be in this patch. If you find such traces occasionally useful, you might want to consider introducing traces conditional on a "debug" setting. See how we use "debug_infrun" inside infrun.c for instance. > + for (regi = 1; regi <= 32; regi++) > + { > + regcache_raw_supply (regcache, regi, &gregsetp->reg[regi]); > + } A minor style issue: The curly braces are not used when there is only one statement in the body of the loop. > + static void > + lm32_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > + { > + /* Set the sigtramp frame sniffer. */ > + //frame_unwind_append_sniffer (gdbarch, lm32_linux_sigtramp_frame_sniffer); This commented-out code should be removed. > + set_gdbarch_regset_from_core_section (gdbarch, > + lm32_linux_regset_from_core_section); Tabs vs space... (sorry about that, it annoys me at least as much as it must be annoying you right now) > + static enum gdb_osabi > + lm32_linux_elf_osabi_sniffer (bfd *abfd) > + { > + int elf_flags; > + > + elf_flags = elf_elfheader (abfd)->e_flags; > + > + if (elf_flags & EF_LM32_MACH) > + return GDB_OSABI_LINUX; > + else The "else" is incorrectly indented. > + static int > + lm32_register_reggroup_p (struct gdbarch *gdbarch, int regnum, > + struct reggroup *group) Indentation issue... > + if (group == general_reggroup) > + { Can you remove the unnecessary curly braces, please? > + return ((regnum >= LM32_R0_REGNUM) && (regnum <= LM32_RA_REGNUM)) > + || (regnum == LM32_PC_REGNUM); The formatting is strange, and will be destroyed if we run gdb_indent.sh. The GNU Coding Standards (GCS) recommend that this be formatted as follow: return ((regnum >= LM32_R0_REGNUM && regnum <= LM32_RA_REGNUM) || regnum == LM32_PC_REGNUM); (watchout for tabs, as you'll probably need them for the second line). > + else if (group == system_reggroup) > + { > + return ( (regnum >= LM32_EA_REGNUM) > + && (regnum <= LM32_BA_REGNUM) > + ); > + } Same here: else if (group == system_reggroup) return (regnum >= LM32_EA_REGNUM && regnum <= LM32_BA_REGNUM); > + /* Return a name that corresponds to the given register number */ Can you add a '.' at the end of your sentence, followed by two spaces, please? I realize this can be annoying to fix these, but we try to be consistent and follow the GCS. > + if ((reg_nr < 0) || (reg_nr >= sizeof (register_names) / sizeof (register_names[0]))) > + return NULL; Formatting: The condition needs to be split in two lines to fit in 80 characters. Or better yet, use the ARRAY_SIZE macro defined for us in libiberty.h (already included by defs.h). > + /* Parse a functions prologue */ Typo: No 's' at the end of "function". Also, a period needs to be added at the end of the sentence. However, when I looked at the implementation, the function turned out to be completely different from what I expected! Traditionally, we call this function "..._analyze_prologue" and this function usually performs two things: - analyze the prologue instructions to build a frame_cache object; - return the address where the last prologue instruction was found. > + static CORE_ADDR > + lm32_parse_prologue (CORE_ADDR pc) With this in mind, it's a shame that you have two separate functions for parsing and analyzing... You can have a look at i386-tdep.c to see how it uses the same function to perform both tasks. I'm not going to review the body of this function assuming that you'll be able to merge it with the function that analyzes the function, which you can extract from your "lm32_frame_unwind_cache" function, I think. > + /* If we have line debugging information, then the end of the > + prologue should the first assembly instruction of the first source line */ > + if (find_pc_partial_function (pc, NULL, &func_addr, &func_end)) > + { > + sal = find_pc_line (func_addr, 0); > + if (sal.end && sal.end < func_end) > + return sal.end; > + } This part of the code can be replaced by a call to skip_prologue_using_sal. > + /* Create a breakpoint instruction */ Period and two spaces at the end of the sentence... > + static const unsigned char * > + lm32_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr) > + { > + static const unsigned char breakpoint[4] = {OP_RAISE << 2, 0, 0, 2}; "unsigned char" should be replaced by gdb_byte (same for the function return type). > + /* Setup registers and stack for faking a call to a function in the inferior */ period and spaces at the end of the sentence... > + /* Set the return address */ > + /* If we're returning a large struct, a pointer to the address to > + store it at is passed as a first hidden parameter */ > + /* Setup parameters */ > + /* Promote small integer types to int */ > + /* FIXME: Handle structures */ > + /* Update stack pointer */ > + /* Extract return value after calling a function in the inferior */ > + /* Return value is returned in a single register */ Ditto. > + if (TYPE_LENGTH (arg_type) < 4) > + { > + arg_type = builtin_type_int32; > + arg = value_cast (arg_type, arg); The last two lines are incorrectly formatted. You're also mixing spaces and tabs, but I'm just about ready to give up on this one :-(. > + contents = (char *) value_contents (arg); I think that contents needs to be declared as a gdb_byte *, and that this will allow the cast to go away. > + /* First num_arg_regs parameters go in registers, rest go on stack */ I should say that I really appreciate it when people make an effort to comment their code as you did. I do personally follow a strict rule when it comes to comments, as I try to write them in real English as opposed to abbreviated English. The reason why I do this is because I find that abbreviated English often makes it harder to read a comment, thus reducing its effectiveness. I was ready to let the comment above go, except that I noticed that the second "go" should be "goes" and that you're missing the period at the end of your sentence... I suggest: /* The First num_arg_regs parameters are passed by registers, and the rest as passed on the stack. */ > + if (i < num_arg_regs) > + { > + regcache_cooked_write_unsigned (regcache, first_arg_reg + i, val); > + } Can you remove the unnecessary curly braces, please? > + static void > + lm32_extract_return_value (struct type *type, struct regcache *regcache, > + gdb_byte *valbuf) Tabs instead of spaces... > + if ( TYPE_CODE(type) != TYPE_CODE_STRUCT > + && TYPE_CODE(type) != TYPE_CODE_UNION > + && TYPE_CODE(type) != TYPE_CODE_ARRAY > + && TYPE_LENGTH (type) <= 4 > + ) Formatting... I think this is the last time I'm going to mention this. I'll let you fix the formatting everywhere else. > + /* 64-bit values are returned in a register pair */ Same for the missing periods at the end of your sentences. Can you fix them throughout your patch, please? > + /* Aggregate types greater than a single register are returned in memory: I think that a period instead of a colon is better in this case. But that's just a suggestion. > + static void > + lm32_store_return_value (struct type *type, struct regcache *regcache, > + const gdb_byte *valbuf) You're missing one space at the beginning of the second line. Otherwise, the argument is not aligned... > + val = extract_unsigned_integer ((char *)valbuf + 4, len - 4); I think the cast should be removed. > + static enum return_value_convention > + lm32_return_value (struct gdbarch *gdbarch, struct type *func_type, > + struct type *valtype, struct regcache *regcache, One space too many... > + gdb_byte *readbuf, const gdb_byte *writebuf) Tabs vs spaces... > + if ((code == TYPE_CODE_STRUCT > + || code == TYPE_CODE_UNION > + || code == TYPE_CODE_ARRAY > + || TYPE_LENGTH (valtype) > 8)) Can you remove the extra parens? > + static struct frame_id > + lm32_dummy_id (struct gdbarch *gdbarch, struct frame_info *this_frame) > + { > + CORE_ADDR sp = lm32_unwind_sp (gdbarch, this_frame); I don't think this is correct, but I often get confused. In this case, I think you're using the SP of the caller of THIS_FRAME. I think you want to use the SP of this frame, in which case you need to call get_frame_register_unsigned. > + struct lm32_unwind_cache * > + lm32_frame_unwind_cache (struct frame_info *this_frame, > + void **this_prologue_cache) I think that this function should be made static. Also as discussed earlier, you should extract out the prologue analyzing portion in order to factorize a little bit your code. One last thing, just to make it consistent across GDB, can you drop the "unwind" in the name of the function? > + /* The FUNC is easy. */ > + func = get_frame_func (this_frame); Usually, the frame_func address is saved in the frame cache to avoid having to recompute again. You've already needed to compute it while computing the frame cache, so might as well avoid the recomputing... For instance, the i386 frame code stores this address in a field called "pc". > + /* Hopefully the prologue analysis either correctly determined the > + frame's base (which is the SP from the previous frame), or set > + that base to "NULL". */ > + base = info->base; > + if (base == 0) > + return; > + > + id = frame_id_build (base, func); > + (*this_id) = id; You're also using a lot of local variables that we usually don't introduce for this function. I usually don't really care one way or the other, but this made your function implementation look different from the others. This is one of these functions that are implemented in pretty much every -tdep file, so consistency with the rest if we can would be nice. See i386-tdep.c... > + static struct value * > + lm32_frame_prev_register (struct frame_info *this_frame, > + void **this_prologue_cache, > + int regnum) > + { > + struct lm32_unwind_cache *info; > + info = lm32_frame_unwind_cache (this_frame, this_prologue_cache); Minor style nit: Can you add an empty line after your declaration? We like to have this empty line between the declaration block and the statements. > + /* None found, create a new architecture from the information provided. */ Missing space at the end... > Index: gdb/lm32-linux-tdep.h I think that this file should be renamed lm32-tdep.h. There doesn't seem to be anything that's specific to Linux in that file. For instance, I see: > + enum lm32_linux_regs This should only depend on the CPU, not the OS... > + /* Fetch the executable load address for the PIC/FDPIC ABI. > + Return 0 if successful, -1 if not. */ > + int lm32_load_address(struct gdbarch *gdbarch, > + CORE_ADDR *load_addr); Never actually implemented... > Index: gdb/lm32-tdep.c I'm not going to review that file. I assume that the contents should be very close to what lm32-linux-tdep.c contained. I'll review this file again once the duplication between lm32-linux-tdep.c and lm32-tdep is taken care of. -- Joel