Re: patch to add semihosting control for remote arm targets

[Richard Earnshaw <rearnsha@arm.com>]

> 
> When debugging remote arm targets using  devices such as JEENI  from epitools 
> the semihosting function slows execution by the target of SWI instruction 
> nearly 100-fold.  This is especially annoying when you try to debug  linux 
> kernel on the target as it uses SWI to implement system calls.  
> The attached patch adds a set rdisemihosting {on/off/1/0/true/false} command 
> to the gdb which allows one to enable or disable semihosting suppport.
> the command shoud be executed before target rdi ....  command
> The patch is against gdb 5.3 but i think will apply to older versions cleanly 
> too.
> 
> 

The idea is a good one, but there are several problems with this patch.

1) The formatting does not conform to GNU coding standards.  (use 
gdb_indent.sh)
2) I think we should have a "set rdi" command for the two rdi features (in 
the same way that we have set debug).  Heartbeat and semihosting should be 
sub-variables of that command.
3) Ideally, it should be possible to turn off semihosting after we have 
connected to the target.

Do you have a copyright assignment on file?

Other comments are interspersed below.

>  Wed Mar 12 13:00:00 2003  Vadim Lebedev  (vadim at 7chips.com)
> 
> 	* remote-rdi.c 		add set rdisemihosting command
> 

Formatting.  Please follow ChangeLog conventions.


> +/* target has semihosting enabled */

Formatting.  Capital letter at the start.  Full stop and two spaces at the 
end before the close-comment.

>    if (rslt != RDIError_NoError)
> -    {
> -      printf_filtered ("RDI_info: %s\n", rdi_error_message (rslt));
> -    }
> +  {
> +	  printf_filtered ("RDI_info: %s\n", rdi_error_message (rslt));
> +  }

Indentation -- why have you changed it?

> +
> +  arg1 = rdi_semihosting ? 1 : 0;
> +  
> +  rslt = angel_RDI_info(RDISemiHosting_SetState,  &arg1, &arg2);
> +  if (rslt != RDIError_NoError)
> +  {
> +	  printf_filtered ("RDI_info: %s\n", rdi_error_message (rslt));
> +  }

Indentation.

>  
> +  
>    arg1 = (unsigned long) "";
>    rslt = angel_RDI_info (RDISet_Cmdline, &arg1, &arg2);
>    if (rslt != RDIError_NoError)
> @@ -1051,14 +1064,24 @@ _initialize_remote_rdi (void)
>       &setlist, &showlist);
>  
>    add_setshow_boolean_cmd
> -    ("rdiheartbeat", no_class, &rdi_heartbeat,
> -     "Set enable for ADP heartbeat packets.\n"
> -     "I don't know why you would want this. If you enable them,\n"
> -     "it will confuse ARM and EPI JTAG interface boxes as well\n"
> -     "as the Angel Monitor.\n",
> -     "Show enable for ADP heartbeat packets.\n",
> -     NULL, NULL,
> -     &setlist, &showlist);
> +		  ("rdiheartbeat", no_class, &rdi_heartbeat,
> +		   "Set enable for ADP heartbeat packets.\n"
> +		   "I don't know why you would want this. If you enable them,\n"
> +		   "it will confuse ARM and EPI JTAG interface boxes as well\n"
> +		   "as the Angel Monitor.\n",
> +		   "Show enable for ADP heartbeat packets.\n",
> +		   NULL, NULL,
> +		   &setlist, &showlist);

Indentation.

> +
> +  add_setshow_boolean_cmd
> +		  ("rdisemihosting", no_class, &rdi_semihosting,
> +		   "Set semihosting support.\n"
> +		   "A true value activates semihosting false value deactivates it.\n",

Unnecessary repetition.  "A true value activates semihosting.\n" is 
sufficient.

> +		   "Show enable for semihosting.\n",
> +		   NULL, NULL,
> +		   &setlist, &showlist);
> +
> +  

Indetation -- delete unnecessary blank lines.

R.