[PATCH] sim: watch: add basic default handler that traps

[Andrew Burgess]

* Mike Frysinger via Gdb-patches <gdb-patches@sourceware.org> [2021-01-13 06:05:19 -0500]:

> The default watchpoint handler is NULL.  That means any port that
> sets the STATE_WATCHPOINTS->pc field will crash if you try to use
> the --watch options but don't configure the interrupt handler.  In
> the past, you had to setup STATE_WATCHPOINTS->pc if you wanted to
> support PC profiling, and while that was fixed a while ago, we have
> a lot of ports who still configure it.
> 
> We already add a default set of interrupts (just "int") if the port
> doesn't define any.  Let's also add a default handler that raises a
> SIGTRAP.  When connected to gdb, this is a breakpoint which is what
> people would expect.  When running standalone, it'll abort the sim,
> but it's unclear whether there's anything better to do there.  This
> really is just to make the watchpoint module more usable out of the
> box for most ports with very little setup, at least inside of gdb.
> ---
>  sim/common/ChangeLog   |  6 ++++++
>  sim/common/sim-watch.c | 13 ++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)

LGTM.

Thanks,
Andrewx

> 
> diff --git a/sim/common/ChangeLog b/sim/common/ChangeLog
> index 76d86ea55ef8..3571c95e918b 100644
> --- a/sim/common/ChangeLog
> +++ b/sim/common/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-01-13  Mike Frysinger  <vapier@gentoo.org>
> +
> +	* sim-watch.c (default_interrupt_handler): Define.
> +	(sim_watchpoint_install): Set default interrupt_handler to new
> +	default_interrupt_handler.
> +
>  2021-01-13  Mike Frysinger  <vapier@gentoo.org>
>  
>  	* sim-watch.c (do_watchpoint_create): Parse arg+1 and assign to arg1.
> diff --git a/sim/common/sim-watch.c b/sim/common/sim-watch.c
> index 29ac982b0d2a..6d1772985971 100644
> --- a/sim/common/sim-watch.c
> +++ b/sim/common/sim-watch.c
> @@ -376,7 +376,16 @@ static const OPTION watchpoint_options[] =
>  
>  static const char *default_interrupt_names[] = { "int", 0, };
>  
> -
> +/* This default handler is "good enough" for targets that just want to trap into
> +   gdb when watchpoints are hit, and have only configured STATE_WATCHPOINTS pc &
> +   sizeof_pc fields.  */
> +static void
> +default_interrupt_handler (SIM_DESC sd, void *data)
> +{
> +  sim_cpu *cpu = STATE_CPU (sd, 0);
> +  address_word cia = CPU_PC_GET (cpu);
> +  sim_engine_halt (sd, cpu, NULL, cia, sim_stopped, SIM_SIGTRAP);
> +}
>  
>  SIM_RC
>  sim_watchpoint_install (SIM_DESC sd)
> @@ -389,6 +398,8 @@ sim_watchpoint_install (SIM_DESC sd)
>    /* fill in some details */
>    if (watch->interrupt_names == NULL)
>      watch->interrupt_names = default_interrupt_names;
> +  if (watch->interrupt_handler == NULL)
> +    watch->interrupt_handler = default_interrupt_handler;
>    watch->nr_interrupts = 0;
>    while (watch->interrupt_names[watch->nr_interrupts] != NULL)
>      watch->nr_interrupts++;
> -- 
> 2.28.0
>