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

[Paunovic, Aleksandar]

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

Actually now when I tested this further I saw that this check (the whole patch) is unnecessary because GDB recovers successfully.
Originally the regcache_read_pc function call (10 lines below my change) was throwing "PC register is not available" but it seems in the meantime this is fixed.
I will put only first patch for review.

>> +    }
>>  
>>    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
>>
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928