Re: [PATCH] Conditionally restore displaced stepping state after fork.

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-05-31 12:15 p.m., John Baldwin wrote:
> There is no default method for
> gdbarch_displaced_step_restore_all_in_ptid, so calling it
> unconditionally for fork events triggered an assertion failure on
> platforms that do not support displaced stepping.  To fix, only invoke
> the method if the gdbarch supports displaced stepping.
> 
> gdb/ChangeLog:
> 
> 	* infrun.c (handle_inferior_event): Only call
> 	gdbarch_displaced_step_restore_all_in_ptid if
> 	gdbarch_supports_displaced_stepping is true.
> ---
>  gdb/ChangeLog | 6 ++++++
>  gdb/infrun.c  | 3 ++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 03910c0634..b0f448a35e 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-05-30  John Baldwin  <jhb@FreeBSD.org>
> +
> +	* infrun.c (handle_inferior_event): Only call
> +	gdbarch_displaced_step_restore_all_in_ptid if
> +	gdbarch_supports_displaced_stepping is true.
> +
>  2021-05-27  Simon Marchi  <simon.marchi@polymtl.ca>
>  
>  	* Fix tab after space indentation issues throughout.
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index e9624d2a9b..6fd077796f 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -5496,7 +5496,8 @@ handle_inferior_event (struct execution_control_state *ecs)
>  	/* If this is a fork (child gets its own address space copy) and some
>  	   displaced step buffers were in use at the time of the fork, restore
>  	   the displaced step buffer bytes in the child process.  */
> -	if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
> +	if (ecs->ws.kind == TARGET_WAITKIND_FORKED
> +	    && gdbarch_supports_displaced_stepping (gdbarch))
>  	  gdbarch_displaced_step_restore_all_in_ptid
>  	    (gdbarch, parent_inf, ecs->ws.value.related_pid);

gdbarch_supports_displaced_stepping checks whether the gdbarch implement
gdbarch_displaced_step_prepare.  But a gdbarch could technically
implement gdbarch_displaced_step_prepare and not
gdbarch_displaced_step_restore_all_in_ptid.  Would it be useful though?
If not, we force that arches implementing gdbarch_displaced_step_prepare
also implement gdbarch_displaced_step_restore_all_in_ptid by using a
post-check on gdbarch creation (like we check that if an arch implements
gdbarch_displaced_step_prepare, it also implements
gdbarch_displaced_step_finish).  In that case, checking
gdbarch_supports_displaced_stepping here will be sufficient.

If there's a case for having gdbarch_displaced_step_prepare but not
gdbarch_displaced_step_restore_all_in_ptid, then we would probably need
to use gdbarch_displaced_step_restore_all_in_ptid_p.  But I don't see a
use case for that, so I'd go with the other solution.

Simon