[v8,5/6] gdb/infrun: enable/disable thread events of all targets in stop_all_threads

Message ID 20200513205338.14233-6-palves@redhat.com
State New
Headers show
Series
  • Handle already-exited threads in 'stop_all_threads'
Related show

Commit Message

Lancelot SIX via Gdb-patches May 13, 2020, 8:53 p.m.
From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>


In stop_all_threads, the thread events of the current top target are
enabled at the beginning of the function and then disabled at the end
(at scope exit time).  Because there may be multiple targets whose
thread lists will be updated and whose threads are stopped,
enable/disable thread events for all targets.

This update caused a change in the annotations.  In particular, a
"frames-invalid" annotation is printed one more time due to switching
the current inferior.  Hence, gdb.base/annota1.exp and
gdb.cp/annota2.exp tests are also updated.

Regression-tested on X86_64 Linux using the default board file and the
native-extended-gdbserver board file.

gdb/ChangeLog:
2020-04-30  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* infrun.c (stop_all_threads): Enable/disable thread events of all
	targets.

gdb/testsuite/ChangeLog:
2020-04-30  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.base/annota1.exp: Update the expected output.
	* gdb.cp/annota2.exp: Ditto.
---
 gdb/infrun.c                       | 15 +++++++++++++--
 gdb/testsuite/gdb.base/annota1.exp |  2 +-
 gdb/testsuite/gdb.cp/annota2.exp   |  2 +-
 3 files changed, 15 insertions(+), 4 deletions(-)

-- 
2.14.5

Comments

Lancelot SIX via Gdb-patches May 14, 2020, 8:44 a.m. | #1
On Wednesday, May 13, 2020 10:54 PM, Pedro Alves wrote:
> diff --git a/gdb/infrun.c b/gdb/infrun.c

> index c5bf2d0ad74..6602bc28d5e 100644

> --- a/gdb/infrun.c

> +++ b/gdb/infrun.c

> @@ -4769,8 +4769,12 @@ stop_all_threads (void)

> 

>    scoped_restore_current_thread restore_thread;

> 

> -  target_thread_events (1);

> -  SCOPE_EXIT { target_thread_events (0); };

> +  /* Enable thread events of all targets.  */

> +  for (auto *target : all_non_exited_process_targets ())

> +    {

> +      switch_to_target_no_thread (target);

> +      target_thread_events (true);

> +    }

> 

>    /* Request threads to stop, and then wait for the stops.  Because

>       threads we already know about can spawn more threads while we're

> @@ -4962,6 +4966,13 @@ stop_all_threads (void)

>  	}

>      }

> 

> +  /* Disable thread events of all targets.  */

> +  for (auto *target : all_non_exited_process_targets ())

> +    {

> +      switch_to_target_no_thread (target);

> +      target_thread_events (false);

> +    }

> +

>    if (debug_infrun)

>      fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");

>  }


In internal communication, Markus pointed out that while the original
SCOPED_EXIT block would run also in the case of exceptions, the new code
does not.  Here is an update that uses SCOPED_EXIT for the thread event
disabling loop, too:

diff --git a/gdb/infrun.c b/gdb/infrun.c
index c5bf2d0ad74..04fcc390d17 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4769,8 +4769,22 @@ stop_all_threads (void)

   scoped_restore_current_thread restore_thread;

-  target_thread_events (1);
-  SCOPE_EXIT { target_thread_events (0); };
+  /* Enable thread events of all targets.  */
+  for (auto *target : all_non_exited_process_targets ())
+    {
+      switch_to_target_no_thread (target);
+      target_thread_events (true);
+    }
+
+  SCOPE_EXIT
+    {
+      /* Disable thread events of all targets.  */
+      for (auto *target : all_non_exited_process_targets ())
+       {
+         switch_to_target_no_thread (target);
+         target_thread_events (false);
+       }
+    };

   /* Request threads to stop, and then wait for the stops.  Because
      threads we already know about can spawn more threads while we're

Thanks,
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Lancelot SIX via Gdb-patches May 14, 2020, 11:16 a.m. | #2
On 5/14/20 9:44 AM, Aktemur, Tankut Baris wrote:
> On Wednesday, May 13, 2020 10:54 PM, Pedro Alves wrote:

>> diff --git a/gdb/infrun.c b/gdb/infrun.c

>> index c5bf2d0ad74..6602bc28d5e 100644

>> --- a/gdb/infrun.c

>> +++ b/gdb/infrun.c

>> @@ -4769,8 +4769,12 @@ stop_all_threads (void)

>>

>>    scoped_restore_current_thread restore_thread;

>>

>> -  target_thread_events (1);

>> -  SCOPE_EXIT { target_thread_events (0); };

>> +  /* Enable thread events of all targets.  */

>> +  for (auto *target : all_non_exited_process_targets ())

>> +    {

>> +      switch_to_target_no_thread (target);

>> +      target_thread_events (true);

>> +    }

>>

>>    /* Request threads to stop, and then wait for the stops.  Because

>>       threads we already know about can spawn more threads while we're

>> @@ -4962,6 +4966,13 @@ stop_all_threads (void)

>>  	}

>>      }

>>

>> +  /* Disable thread events of all targets.  */

>> +  for (auto *target : all_non_exited_process_targets ())

>> +    {

>> +      switch_to_target_no_thread (target);

>> +      target_thread_events (false);

>> +    }

>> +

>>    if (debug_infrun)

>>      fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");

>>  }

> 

> In internal communication, Markus pointed out that while the original

> SCOPED_EXIT block would run also in the case of exceptions, the new code

> does not.  Here is an update that uses SCOPED_EXIT for the thread event

> disabling loop, too:

> 

> diff --git a/gdb/infrun.c b/gdb/infrun.c

> index c5bf2d0ad74..04fcc390d17 100644

> --- a/gdb/infrun.c

> +++ b/gdb/infrun.c

> @@ -4769,8 +4769,22 @@ stop_all_threads (void)

> 

>    scoped_restore_current_thread restore_thread;

> 

> -  target_thread_events (1);

> -  SCOPE_EXIT { target_thread_events (0); };

> +  /* Enable thread events of all targets.  */

> +  for (auto *target : all_non_exited_process_targets ())

> +    {

> +      switch_to_target_no_thread (target);

> +      target_thread_events (true);

> +    }

> +

> +  SCOPE_EXIT

> +    {

> +      /* Disable thread events of all targets.  */

> +      for (auto *target : all_non_exited_process_targets ())

> +       {

> +         switch_to_target_no_thread (target);

> +         target_thread_events (false);

> +       }

> +    };

> 


Yes, I noticed it too, but forgot to address it.  Thanks for bringing
it up.

I think the:

  if (debug_infrun)
    fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");

bit should also move to within SCOPE_EXIT, since we currently
print that before the destructors are run.  But that could be seen
as an unrelated change.

Thanks,
Pedro Alves
Lancelot SIX via Gdb-patches May 14, 2020, 11:30 a.m. | #3
On Thursday, May 14, 2020 1:17 PM, Pedro Alves wrote:
> On 5/14/20 9:44 AM, Aktemur, Tankut Baris wrote:

> > On Wednesday, May 13, 2020 10:54 PM, Pedro Alves wrote:

> >> diff --git a/gdb/infrun.c b/gdb/infrun.c

> >> index c5bf2d0ad74..6602bc28d5e 100644

> >> --- a/gdb/infrun.c

> >> +++ b/gdb/infrun.c

> >> @@ -4769,8 +4769,12 @@ stop_all_threads (void)

> >>

> >>    scoped_restore_current_thread restore_thread;

> >>

> >> -  target_thread_events (1);

> >> -  SCOPE_EXIT { target_thread_events (0); };

> >> +  /* Enable thread events of all targets.  */

> >> +  for (auto *target : all_non_exited_process_targets ())

> >> +    {

> >> +      switch_to_target_no_thread (target);

> >> +      target_thread_events (true);

> >> +    }

> >>

> >>    /* Request threads to stop, and then wait for the stops.  Because

> >>       threads we already know about can spawn more threads while we're

> >> @@ -4962,6 +4966,13 @@ stop_all_threads (void)

> >>  	}

> >>      }

> >>

> >> +  /* Disable thread events of all targets.  */

> >> +  for (auto *target : all_non_exited_process_targets ())

> >> +    {

> >> +      switch_to_target_no_thread (target);

> >> +      target_thread_events (false);

> >> +    }

> >> +

> >>    if (debug_infrun)

> >>      fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");

> >>  }

> >

> > In internal communication, Markus pointed out that while the original

> > SCOPED_EXIT block would run also in the case of exceptions, the new code

> > does not.  Here is an update that uses SCOPED_EXIT for the thread event

> > disabling loop, too:

> >

> > diff --git a/gdb/infrun.c b/gdb/infrun.c

> > index c5bf2d0ad74..04fcc390d17 100644

> > --- a/gdb/infrun.c

> > +++ b/gdb/infrun.c

> > @@ -4769,8 +4769,22 @@ stop_all_threads (void)

> >

> >    scoped_restore_current_thread restore_thread;

> >

> > -  target_thread_events (1);

> > -  SCOPE_EXIT { target_thread_events (0); };

> > +  /* Enable thread events of all targets.  */

> > +  for (auto *target : all_non_exited_process_targets ())

> > +    {

> > +      switch_to_target_no_thread (target);

> > +      target_thread_events (true);

> > +    }

> > +

> > +  SCOPE_EXIT

> > +    {

> > +      /* Disable thread events of all targets.  */

> > +      for (auto *target : all_non_exited_process_targets ())

> > +       {

> > +         switch_to_target_no_thread (target);

> > +         target_thread_events (false);

> > +       }

> > +    };

> >

> 

> Yes, I noticed it too, but forgot to address it.  Thanks for bringing

> it up.

> 

> I think the:

> 

>   if (debug_infrun)

>     fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");

> 

> bit should also move to within SCOPE_EXIT, since we currently

> print that before the destructors are run.  But that could be seen

> as an unrelated change.


OK, I'll move that piece into SCOPED_EXIT, too.

> 

> Thanks,

> Pedro Alves


-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
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 c5bf2d0ad74..6602bc28d5e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4769,8 +4769,12 @@  stop_all_threads (void)
 
   scoped_restore_current_thread restore_thread;
 
-  target_thread_events (1);
-  SCOPE_EXIT { target_thread_events (0); };
+  /* Enable thread events of all targets.  */
+  for (auto *target : all_non_exited_process_targets ())
+    {
+      switch_to_target_no_thread (target);
+      target_thread_events (true);
+    }
 
   /* Request threads to stop, and then wait for the stops.  Because
      threads we already know about can spawn more threads while we're
@@ -4962,6 +4966,13 @@  stop_all_threads (void)
 	}
     }
 
+  /* Disable thread events of all targets.  */
+  for (auto *target : all_non_exited_process_targets ())
+    {
+      switch_to_target_no_thread (target);
+      target_thread_events (false);
+    }
+
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads done\n");
 }
diff --git a/gdb/testsuite/gdb.base/annota1.exp b/gdb/testsuite/gdb.base/annota1.exp
index 9d3bf73431c..829d144cc20 100644
--- a/gdb/testsuite/gdb.base/annota1.exp
+++ b/gdb/testsuite/gdb.base/annota1.exp
@@ -223,7 +223,7 @@  gdb_test_multiple "break printf" "break printf" {
 #
 # get to printf
 #
-set pat_begin "\r\n\032\032post-prompt\r\nContinuing.\r\n\r\n\032\032starting\r\n\r\n\032\032frames-invalid\r\n${breakpoints_invalid}"
+set pat_begin "\r\n\032\032post-prompt\r\nContinuing.\r\n\r\n\032\032starting\r\n\r\n\032\032frames-invalid\r\n${breakpoints_invalid}\r\n\032\032frames-invalid\r\n"
 set pat_adjust "warning: Breakpoint 3 address previously adjusted from $hex to $hex.\r\n"
 set pat_end "\r\n\032\032breakpoint 3\r\n\r\nBreakpoint 3, \r\n\032\032frame-begin 0 $hex\r\n\r\n(\032\032frame-address\r\n$hex\r\n\032\032frame-address-end\r\n in \r\n)*.*\032\032frame-function-name\r\n.*printf(@.*)?\r\n\032\032frame-args\r\n.*\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$"
 
diff --git a/gdb/testsuite/gdb.cp/annota2.exp b/gdb/testsuite/gdb.cp/annota2.exp
index dd3a0a5d6de..1b4f04bb445 100644
--- a/gdb/testsuite/gdb.cp/annota2.exp
+++ b/gdb/testsuite/gdb.cp/annota2.exp
@@ -218,7 +218,7 @@  set pat [multi_line "" \
 	     "\032\032post-prompt" \
 	     "" \
 	     "\032\032starting" \
-	     "\(${frames_invalid}\)*${breakpoints_invalid}" \
+	     "\(${frames_invalid}\)*${breakpoints_invalid}\(${frames_invalid}\)*" \
 	     "\032\032watchpoint 3" \
 	     ".*atchpoint 3: a.x" \
 	     "" \