This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: patch to add semihosting control for remote arm targets
- From: Richard Earnshaw <rearnsha at arm dot com>
- To: Vadim Lebedev <vadim at 7chips dot com>
- Cc: gdb-patches at sources dot redhat dot com, Richard dot Earnshaw at arm dot com
- Date: Thu, 13 Mar 2003 11:39:03 +0000
- Subject: Re: patch to add semihosting control for remote arm targets
- Organization: ARM Ltd.
- Reply-to: Richard dot Earnshaw at arm dot 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.