From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8259 invoked by alias); 7 Mar 2010 05:33:38 -0000 Received: (qmail 8250 invoked by uid 22791); 7 Mar 2010 05:33:37 -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; Sun, 07 Mar 2010 05:33:32 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id AAF532BAB7D; Sun, 7 Mar 2010 00:33:30 -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 92FfFMGjfNQr; Sun, 7 Mar 2010 00:33:30 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 14A552BAB78; Sun, 7 Mar 2010 00:33:30 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 8C344F5894; Sun, 7 Mar 2010 09:33:17 +0400 (RET) Date: Sun, 07 Mar 2010 05:33:00 -0000 From: Joel Brobecker To: Kevin Buettner Cc: gdb-patches@sourceware.org Subject: Re: [RFC] remote-mips.c: Add support for NEC rockhopper boards Message-ID: <20100307053317.GR2832@adacore.com> References: <20100305161522.735aadec@redhat.com> <20100306044223.GP2832@adacore.com> <20100306000443.06e081be@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100306000443.06e081be@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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: 2010-03/txt/msg00269.txt.bz2 > I think this change was made to accomodate the 64-bit rockhopper > boards. The concern is that sscanf() using %lx might not be able to > handle a 64-bit value. Hmmm, that's a good point. The most obvious case is when the host is a 32bit arch, where long will most likely be too small to hold a 64bit value. But even on 64bit machines, long might still be 32bits (this is the case on x64 Windows). > I agree this hunk seems to be fixing a 32-bit -> 64-bit bug instead of > only adding rockhopper support. I can, if you'd like, revise this > patch, the rockhopper support patch, to use store_unsigned_integer() > (in the non-rockhopper case) and then submit a separate patch which > changes store_unsigned_integer() to store_signed_integer(). I don't think that this will be necessary - it would be creating extra work for little gain, IMO. It makes things easier on the reviewer if things are split, but once it's reviewed, the only advantage in splitting the patch that I know of is that it makes it easier to revert one piece or the other, in case of a revert is needed. > From Richard Sandiford, Martin M. Hunt, Corinna Vinschen, > and Kevin Buettner: > > * remote-mips.c (rockhopper_ops): New target_ops struct. > (MON_ROCKHOPPER): New mips_monitor_type. > (read_hex_value): New function. > (mips_request): Send 8-byte values with a 'T' packet. Read the > packet argument as a string and use read_hex_value to parse it. > (mips_exit_debug): Wait for response when using MON_ROCKHOPPER. > (rockhopper_open): New function. > (mips_wait): Read the PC, FP and SP fields as strings. Use > read_hex_value to parse them and mips_set_register to commit them. > (mips_set_register): New function. > (mips_fetch_registers): Do not cast register value to "unsigned" > when reading a MON_ROCKHOPPER 't' packet. Use mips_set_register. > (mips_store_registers): Use a 'T' packet to set registers when > using MON_ROCKHOPPER. > (pmon_end_download): Don't run initEther if using MON_ROCKHOPPER > and expect the total to be printed before the entry address. > (_initialize_remote_mips): Initialize and add rockhopper_ops. Overall, the patch looks good to me. However, watch out that some hunks of your patch contain hunks for another earlier patch (about changing memory reads so as not to throw an exception). The rest looks about the same, with the mods we discussed. Just one minor nit: +static void +mips_set_register (int regno, ULONGEST value) Would you mind adding a short description of what this function does. It's sort of obvious from the name, but we're trying to document every function, and I'd rather we document all functions, including the obvious ones, rather than having to decide which ones deserve a descriptions and which ones don't. I don't think I will have much else to add, so you shouldn't wait for me to approve a final version. -- Joel