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

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

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