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

[John Baldwin <jhb@FreeBSD.org>]

On 5/31/21 5:59 PM, Simon Marchi wrote:
> 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.

I should have been clearer that I wasn't fully sure this was the right fix.
I had read the comment for gdbarch_supports_displaced_stepping as meaning
it only checked the one method as a check for the group.

/* Return true if the gdbarch implements the required methods to use
    displaced stepping.  */

static bool
gdbarch_supports_displaced_stepping (gdbarch *arch)
{
   /* Only check for the presence of step_copy_insn.  Other required methods
      are checked by the gdbarch validation.  */
   return gdbarch_displaced_step_copy_insn_p (arch);
}

If I'm understanding you correctly, we don't currently enforce a check
for this particular method in gdbarch validation so I need to add that?

-- 
John Baldwin