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

[Andrew Burgess <aburgess@redhat.com>]

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