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

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

[Guinevere Larsen]

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
threads have a wait status of "no_resumed", and the "ignore" status
would make us iterate over them and wait more.

This commit fixes that by setting the status correctly to not_resumed.
---
 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 ();
       return minus_one_ptid;
     }
 

base-commit: ca0665335ff67af94f9ded026d7ef2405a68c453
-- 
2.55.0

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

[Andrew Burgess]

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..."

> 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.

>
> 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.

> ---
>  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?

Thanks,
Andrew

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

[Guinevere Larsen]

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