From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5153 invoked by alias); 6 Mar 2010 04:42:41 -0000 Received: (qmail 5142 invoked by uid 22791); 6 Mar 2010 04:42:40 -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, 06 Mar 2010 04:42:36 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 320A82BAAE7; Fri, 5 Mar 2010 23:42:35 -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 senYTloOxbyR; Fri, 5 Mar 2010 23:42:35 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 9C12A2BAADF; Fri, 5 Mar 2010 23:42:34 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 9CBE7F5894; Sat, 6 Mar 2010 08:42:23 +0400 (RET) Date: Sat, 06 Mar 2010 04:42: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: <20100306044223.GP2832@adacore.com> References: <20100305161522.735aadec@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100305161522.735aadec@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/msg00255.txt.bz2 > * 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. I have a few questions on a few things I don't understand. > @@ -1196,11 +1227,12 @@ mips_request (int cmd, > { > int addr_size = gdbarch_addr_bit (target_gdbarch) / 8; > char myBuff[DATA_MAXLEN + 1]; > + char response_string[17]; [...] > - if (sscanf (buff, "0x%x %c 0x%x 0x%lx", > - &rpid, &rcmd, &rerrflg, &rresponse) != 4 > + if (sscanf (buff, "0x%x %c 0x%x 0x%16s", > + &rpid, &rcmd, &rerrflg, response_string) != 4 > + || !read_hex_value (response_string, &rresponse) I don't understand the need to change the sscanf format for the last item from %lx to %16s. It definitely makes the code more complex... > +static void > +mips_set_register (int regno, ULONGEST value) [...] > + if (mips_monitor != MON_ROCKHOPPER > + && (regno == mips_regnum (gdbarch)->pc || regno < 32)) > + /* Some 64-bit boards have monitors that only send the bottom 32 bits. > + In such cases we can only really debug 32-bit code properly so, > + when reading a GPR or the PC, assume that the full 64-bit > + value is the sign extension of the lower 32 bits. */ > + store_signed_integer (buf, register_size (gdbarch, regno), byte_order, > + value); > + else > + store_unsigned_integer (buf, register_size (gdbarch, regno), byte_order, > + value); I just wanted to make sure that you are sure that this is correct (you must be - otherwise I think it'd have shown up in testing). But it's significantly different from the code that it replaces, so it attracted my attention. I guess the initial code had a bug that's unrelated to the rockhopper, which this hunk fixes. > @@ -1942,6 +1998,9 @@ mips_fetch_registers (struct target_ops > if (mips_monitor == MON_DDB) > val = (unsigned) mips_request ('t', pmon_reg, 0, > &err, mips_receive_wait, NULL); > + else if (mips_monitor == MON_ROCKHOPPER) > + val = mips_request ('t', pmon_reg, 0, > + &err, mips_receive_wait, NULL); In this function, val is declared as "unsigned LONGEST" (first time I ever see this!). Can you simplify this to "ULONGEST"? And also remove the cast to unsigned in the call to mips_request when mips_monitor == MON_DDB. And finally, since the call to mips_request seems to be identical if mips_monitor == MON_DDB or mips_monitor == MON_ROCKHOPPER, how about merging them the two branches into one block: if (mips_monitor == MON_DDB || mips_monitor == MON_ROCKHOPPER) val = mips_request ('t', pmon_reg, 0, [...]); ? -- Joel