Re: [PATCH, RFC] Changes to sim_store_register API

[Joel Brobecker <brobecker at adacore dot com>]

Andrew,

Thanks for doing this work.

Just a small request: Can you generate your diffs using the -p option?
This allows us to see the name of the function being modified by each
hunk, thus making review a little easier...

> 2010-11-16  Andrew Burgess <aburgess@broadcom.com>
> 
> 	* remote-sim.c (gdbsim_store_register): Update API to 
> 	  sim_store_register to check more error conditions.

This change needs a corresponding documentation update in
include/gdb/remote-sim.h.

> 2010-11-16  Andrew Burgess <aburgess@broadcom.com>
> 
> 	* interf.c (sim_store_register): Update return value to 
> 	  match new API. 
> 
> sim/h8300/ChangeLog :
> 
> 2010-11-16  Andrew Burgess <aburgess@broadcom.com>
> 
> 	* compile.c (sim_store_register): Update return value to 
> 	  match new API. 
> 
> sim/m32c/ChangesLog : 
> 
> 2010-11-16  Andrew Burgess <aburgess@broadcom.com>
> 
> 	* gdb-if.c (sim_store_register): Update return value to 
> 	  match new API. 

I think that this function should return -1 if the check_regno() check
fails.

> sim/mn10300/ChangeLog : 
> 
> 2010-11-16  Andrew Burgess <aburgess@broadcom.com>
> 
> 	* interp.c (sim_store_register): Update return value to 
> 	  match new API. 
> 
> sim/ppc/ChangeLog : 
> 
> 2010-11-16  Andrew Burgess <aburgess@broadcom.com>
> 
> 	* gdb-sim.c (sim_store_register): Update return value to 
> 	  match new API. 
> 
> sim/rx/ChangeLog :
> 
> 2010-11-16  Andrew Burgess  <aburgess@broadcom.com>
> 
> 	* gdb-if.c (sim_store_register): Update return value to
> 	  match new API.

Same as above, here.

> sim/v850/ChangeLog : 
> 
> 2010-11-16  Andrew Burgess  <aburgess@broadcom.com>
> 
> 	* interp.c (sim_store_register): Update return value to
> 	  match new API.

Overall, this seems OK to me.  Can you make the adjustments requested
above, and then resubmit?

>  	default:
>  	  fprintf (stderr, "m32c minisim: unrecognized register number: %d\n",
>  		   regno);
> -	  return -1;
> +	  return 0;

I had to think twice about this one - on the one hand, reaching this
code means an implementation error/hole somewhere.  But then, assuming
that the register number cannot be off-limits (because of the check_regno()
test above), it's probably because of the sim code missing the handling
for a given register. Which would be best in that case? A warning and
continue, or an internal-error? I'll follow your suggestion on a
warning, hoping that the failed operation does not change the behavior
too much (the warnings might help reduce confusion).

-- 
Joel