Re: [PATCH 2/2] gdb: Improve the resuming of the stepped thread

[Andrew Burgess <andrew.burgess@embecosm.com>]

* aleksandar.paunovic via Gdb-patches <gdb-patches@sourceware.org> [2021-06-07 10:42:03 +0200]:

> From: Aleksandar Paunovic <aleksandar.paunovic@intel.com>
> 
> Stepped thread should not be resumed (again) if the next stopping point
> (next stopping PC) cannot be determined.  Do not resume the stepped thread
> in this case.  Resuming would lead to an inconsistent state where the
> stepped thread would be running forever and GDB would never get to
> breakpoints of the other threads.  */
> 
> gdb/ChangeLog:
> 2021-04-30  Aleksandar Paunovic  <aleksandar.paunovic@intel.com>
> 
>         * infrun.c (keep_going_stepped_thread): Do not resume an
> 	already executing thread.
> 
> gdb/testsuite/ChangeLog:
> 2021-04-30  Aleksandar Paunovic  <aleksandar.paunovic@intel.com>
> 
> 	* gdb.base/breakpoint-running-inferior.exp: Add a start_step
> 	function check.
> 
> 2021-06-07 Aleksandar Paunovic <aleksandar.paunovic@intel.com>
> ---
>  gdb/infrun.c                                      | 15 ++++++++++++++-
>  .../gdb.base/breakpoint-running-inferior.exp      |  5 +++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 78da1f0e72..459a679ee6 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7529,7 +7529,7 @@ restart_after_all_stop_detach (process_stratum_target *proc_target)
>  
>  /* Set a previously stepped thread back to stepping.  Returns true on
>     success, false if the resume is not possible (e.g., the thread
> -   vanished).  */
> +   vanished, or the thread breakpoint position cannot be determined).  */
>  
>  static bool
>  keep_going_stepped_thread (struct thread_info *tp)
> @@ -7566,6 +7566,19 @@ keep_going_stepped_thread (struct thread_info *tp)
>        delete_thread (tp);
>        return false;
>      }
> +  /* Step start function determines the location of the next stopping
> +     point (next PC where the stepped thread should stop).  In case that
> +     this position cannot be determined the step_start_function will not
> +     be set.  Do not resume the stepped thread in this case.  Resuming
> +     would lead to an inconsistent state where the stepped thread would be
> +     running forever and GDB would never get to breakpoints of the other
> +     threads.  */
> +  if (tp->control.step_start_function == nullptr)
> +    {
> +      infrun_debug_printf ("not resuming previously stepped thread, it is "
> +			   "already executing");
> +      return 0;

I don't understand why the condition you're checking means that the
thread is already executing.

Also, your comment says step_start_function is the pc where we should
next stop, but it is set with find_pc_function, which just gives the
symbol for the function containing the $pc when the step/next was
started, so I don't understand why this tells GDB where to stop.

Also 'return false'.

Thanks,
Andrew


> +    }
>  
>    infrun_debug_printf ("resuming previously stepped thread");
>  
> diff --git a/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp b/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp
> index bb0406bc65..b95f2ec012 100644
> --- a/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp
> +++ b/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp
> @@ -74,10 +74,15 @@ gdb_test "next" ".*Thread 2.*hit Breakpoint.*" "next while in inferior 1"
>  
>  # We should be able to normally switch to thread 1.1.
>  # In case of a bad GDB flow the GDB was losing the thread.
> +# The thread should also not be in a "running" state because it is
> +# stopped.
>  gdb_test_multiple "thread 1.1" "Switching to thread 1.1" {
>      -re "\\\[Switching to thread 1.1 \\(Thread .*\\)\\\]" {
>          pass $gdb_test_name
>      }
> +    -re "\\\[Switching to thread 1.1.*\\(running\\)" {
> +        fail $gdb_test_name
> +    }
>      -re ".*Thread ID 1.1 has terminated.*" {
>          fail $gdb_test_name
>      }
> -- 
> 2.17.1
>