Re: [RFC] remote-mips.c: Add support for NEC rockhopper boards

[Joel Brobecker <brobecker@adacore.com>]

> 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