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

Message ID 1623055323-3331-3-git-send-email-aleksandar.paunovic@intel.com
State New
Headers show
Series
  • Fix next command in the running inferior
Related show

Commit Message

Mike Frysinger via Gdb-patches June 7, 2021, 8:42 a.m.
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(-)

-- 
2.17.1

Comments

Andrew Burgess June 7, 2021, 1:48 p.m. | #1
* 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

>
Mike Frysinger via Gdb-patches June 14, 2021, 10:47 a.m. | #2
>> 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

Patch

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;
+    }
 
   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
     }