Re: [PATCH v5 05/15] Don't enable async mode at the end of target ::resume methods.

[Joel Brobecker <brobecker@adacore.com>]

Hi John,

> Now that target_resume always enables async mode after target::resume
> returns, these calls are redundant.
> 
> The other place that target resume methods are invoked outside of
> target_resume are as the beneath target in record_full_wait_1.  In
> this case, async mode should already be enabled when supported by the
> target before the resume method is invoked due to the following:
> 
>   In general, targets which support async mode run as async until
>   ::wait returns TARGET_WAITKIND_NO_RESUMED to indicate that there are
>   no unwaited for children (either they have exited or are stopped).
>   When that occurs, the loop in wait_one disables async mode.  Later
>   if a stopped child is resumed, async mode is re-enabled in
>   do_target_resume before waiting for the next event.
> 
>   In the case of record_full_wait_1, this function is invoked from the
>   ::wait target method when fetching an event.  If the underlying
>   target supports async mode, then an earlier call to do_target_resume
>   to resume the child reporting an event in the loop in
>   record_full_wait_1 would have already enabled async mode before
>   ::wait was invoked.  In addition, nothing in the code executed in
>   the loop in record_full_wait_1 disables async mode.  Async mode is
>   only disabled higher in the call stack in wait_one after ::wait
>   returns.
> 
>   It is also true that async mode can be disabled by an
>   INF_EXEC_COMPLETE event passed to inferior_event_handle, but all of
>   the places that invoke that are in the gdb core which is "above" a
>   target ::wait method.
> 
> Note that there is an earlier call to enable async mode in
> linux_nat_target::resume.  That call also marks the async event pipe
> to report an existing event after enabling async mode, so it needs to
> stay.

Not sure if you expected a review of this patch or not, so I took
a look, and it looks OK to me.

> ---
>  gdb/linux-nat.c   |  3 ---
>  gdb/record-full.c | 10 ----------
>  gdb/remote.c      | 10 ----------
>  3 files changed, 23 deletions(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index db3585dfa23..621118ba862 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1764,9 +1764,6 @@ linux_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo)
>  			   ? strsignal (gdb_signal_to_host (signo)) : "0"));
>  
>    linux_resume_one_lwp (lp, step, signo);
> -
> -  if (target_can_async_p ())
> -    target_async (1);
>  }
>  
>  /* Send a signal to an LWP.  */
> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 76b21523853..bd8c49c1abe 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -1095,11 +1095,6 @@ record_full_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
>  
>        this->beneath ()->resume (ptid, step, signal);
>      }
> -
> -  /* We are about to start executing the inferior (or simulate it),
> -     let's register it with the event loop.  */
> -  if (target_can_async_p ())
> -    target_async (1);
>  }
>  
>  static int record_full_get_sig = 0;
> @@ -2062,11 +2057,6 @@ record_full_core_target::resume (ptid_t ptid, int step,
>    record_full_resume_step = step;
>    record_full_resumed = 1;
>    record_full_execution_dir = ::execution_direction;
> -
> -  /* We are about to start executing the inferior (or simulate it),
> -     let's register it with the event loop.  */
> -  if (target_can_async_p ())
> -    target_async (1);
>  }
>  
>  /* "kill" method for prec over corefile.  */
> diff --git a/gdb/remote.c b/gdb/remote.c
> index b093ad86675..2711e5a301c 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6571,16 +6571,6 @@ remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
>    for (thread_info *tp : all_non_exited_threads (this, ptid))
>      get_remote_thread_info (tp)->set_resumed ();
>  
> -  /* We are about to start executing the inferior, let's register it
> -     with the event loop.  NOTE: this is the one place where all the
> -     execution commands end up.  We could alternatively do this in each
> -     of the execution commands in infcmd.c.  */
> -  /* FIXME: ezannoni 1999-09-28: We may need to move this out of here
> -     into infcmd.c in order to allow inferior function calls to work
> -     NOT asynchronously.  */
> -  if (target_can_async_p ())
> -    target_async (1);
> -
>    /* We've just told the target to resume.  The remote server will
>       wait for the inferior to stop, and then send a stop reply.  In
>       the mean time, we can't start another command/query ourselves
> -- 
> 2.34.1
> 

-- 
Joel