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

[Joel Brobecker <brobecker at adacore dot com>]

> 	* 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