Re: [PATCH v2 3/5] gdb: fix assert when quitting GDB while a thread is stepping

[Simon Marchi <simon.marchi@polymtl.ca>]

On 11/28/22 07:11, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>> From: Andrew Burgess <aburgess@redhat.com>
>>
>> This commit addresses one of the issues identified in PR gdb/28275.
>>
>> Bug gdb/28275 identifies a number of situations in which this assert:
>>
>>   Assertion `!proc_target->commit_resumed_state' failed.
>>
>> could be triggered.  There's actually a number of similar places where
>> this assert is found in GDB, the two of interest in gdb/28275 are in
>> target_wait and target_stop.
>>
>> In one of the comments:
>>
>>   https://sourceware.org/bugzilla/show_bug.cgi?id=28275#c1
>>
>> steps to trigger the assertion within target_stop were identified when
>> using a modified version of the gdb.threads/detach-step-over.exp test
>> script.
>>
>> In the gdb.threads/detach-step-over.exp test, we attach to a
>> multi-threaded inferior, and continue the inferior in asynchronous
>> (background) mode.  Each thread is continuously hitting a conditional
>> breakpoint where the condition is always false.  While the inferior is
>> running we detach.  The goal is that we detach while GDB is performing a
>> step-over for the conditional breakpoint in at least one thread.
>>
>> While detaching, if a step-over is in progress, then GDB has to
>> complete the step over before we can detach.  This involves calling
>> target_stop and target_wait (see prepare_for_detach).
>>
>> As far as gdb/28275 is concerned, the interesting part here, is the
>> the process_stratum_target::commit_resumed_state variable must be
>> false when target_stop and target_wait are called.
>>
>> This is currently ensured because, in detach_command (infrun.c), we
>> create an instance of scoped_disable_commit_resumed, this ensures that
>> when target_detach is called, ::commit_resumed_state will be false.
>>
>> The modification to the test that I propose here, and which exposed
>> the bug, is that, instead of using "detach" to detach from the
>> inferior, we instead use "quit".  Quitting GDB after attaching to an
>> inferior will cause GDB to first detach, and then exit.
>>
>> When we quit GDB we end up calling target_detach via a different code
>> path, the stack looks like:
>>
>>   #0 target_detach
>>   #1 kill_or_detach
>>   #2 quit_force
>>   #3 quit_command
>>
>> Along this path there is no scoped_disable_commit_resumed created.
>> ::commit_resumed_state can be true when we reach prepare_for_detach,
>> which calls target_wait and target_stop, so the assertion will trigger.
>>
>> In this commit, I propose fixing this by adding the creation of a
>> scoped_disable_commit_resumed into target_detach.  This will ensure
>> that ::commit_resumed_state is false when calling prepare_for_detach
>> from within target_detach.
>>
>> I did consider placing the scoped_disable_commit_resumed in
>> prepare_for_detach, however, placing it in target_detach ensures that
>> the target's commit_resumed_state flag is left to false if the detached
>> inferior was the last one for that target.  It's the same rationale as
>> for patch "gdb: disable commit resumed in target_kill" that comes later
>> in this series, but for detach instead of kill.
>>
>> detach_command still includes a scoped_disable_commit_resumed too, but I
>> think it is still relevant to cover the resumption at the end of the
>> function.
>>
>> Co-Authored-By: Simon Marchi <simon.marchi@efficios.com>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28275
>> Change-Id: Ie128f7aba6ef0e018859275eca372e6ea738e96f
> 
> LGTM.  Thanks for taking this into your series.
> 
> Thanks,
> Andrew

Thanks, pushed up to here.

Simon