Re: [PATCH] gdb/record-full: fix stop status when inferior isn't resumed

[Guinevere Larsen <guinevere@redhat.com>]

On 10/9/26 7:44 AM, Andrew Burgess wrote:
> Guinevere Larsen <guinevere@redhat.com> writes:
>
>> The wait function for the record-full target has a check for whether
>> or not the inferior should be resumed. However, when we correctly
>> identified that it shouldn't be resumed, we incorrectly set the exit
>> status as "ignore", as though the inferior had resumed and had an
>> uninteresting stop.
>>
>> This would cause a problem when - for example - GDB has to wait for
>> all threads to actually be stopped, because we do that by waiting all
> Missing word: "...we do that by waiting UNTIL all threads..."

Fixed.

>
>> threads have a wait status of "no_resumed", and the "ignore" status
>> would make us iterate over them and wait more.
> Your excellent "this would cause a problem when ..." description makes
> me ask why there's no test included with this patch.  I know that
> multi-threaded recording is not working well (or at all?) in upstream
> GDB, so maybe it's just not possible to test this right now.  If this is
> the case then it might be worth a sentence to explain this in the commit
> message.

I am pretty sure that this can't really be tested. Besides, once my 
updates on multi-threaded recording land, this use-case will be tested 
multiple times, most likely (and at least once), so it feels unnecessary 
to add a specific test for this.

I'll add a comment about that for v2

>
>> This commit fixes that by setting the status correctly to not_resumed.
> There is a global variable `record_full_resumed` which is documented
> like this:
>
>    /* True if we've been resumed, and so each record_full_wait call should
>       advance execution.  If this is false, record_full_wait will return a
>       TARGET_WAITKIND_IGNORE.  */
>    static int record_full_resumed = 0;
>
> With this change I think that this comment now needs to be updated.
Fixed
>
>> ---
>>   gdb/record-full.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gdb/record-full.c b/gdb/record-full.c
>> index 26bd85d34f6..f56b3dfc7fd 100644
>> --- a/gdb/record-full.c
>> +++ b/gdb/record-full.c
>> @@ -1188,7 +1188,7 @@ record_full_wait_1 (struct target_ops *ops,
>>         gdb_assert ((options & TARGET_WNOHANG) != 0);
>>   
>>         /* No interesting event.  */
>> -      status->set_ignore ();
>> +      status->set_no_resumed ();
> I think the comment "No interesting event" could be improved, maybe "No
> resumed threads" or something similar?
Fixed

-- 
Cheers,
Guinevere Larsen
it/its
she/her (deprecated)

>
> Thanks,
> Andrew