Re: Inadvertently run inferior threads

Re: Inadvertently run inferior threads

[Eli Zaretskii]

[I'm taking this to gdb-patches@, as I'm suggesting a patch.]

> Date: Wed, 10 Jun 2015 18:50:13 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gdb@sourceware.org
> 
> So 'set_running' is called, and it is called with minus_one_ptid,
> which then has the effect of marking all the threads as running.
> 
> What I don't understand is why doesn't the breakpoint we set at exit
> from the inferior function countermand that.  I do see the effect of
> that breakpoint if I turn on infrun debugging:
> 
>   infrun: target_wait (-1, status) =
>   infrun:   4608 [Thread 4608.0x4900],
>   infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
>   infrun: TARGET_WAITKIND_STOPPED
>   infrun: stop_pc = 0x88ba9f
>   infrun: BPSTAT_WHAT_STOP_SILENT
>   infrun: stop_waiting
> 
> Why don't we mark all threads as stopped when we hit the breakpoint?
> is that because of #3 above?

Answering myself: yes.

> Any ideas how to solve this annoying problem?

One idea is implemented in the patch below.  I tested it in a
MinGW-compiled GDB doing native debugging, and it completely solved
the problem for me: I see additional threads started during an
infcall, but none of those dreaded "the selected thread is running"
error messages.

OK to commit (with a suitable ChangeLog entry)?

--- gdb/infrun.c~0	2015-01-13 15:14:47 +0200
+++ gdb/infrun.c	2015-06-11 07:13:54 +0300
@@ -6508,8 +6508,14 @@ normal_stop (void)
      running, all without informing the user/frontend about state
      transition changes.  If this is actually a call command, then the
      thread was originally already stopped, so there's no state to
-     finish either.  */
-  if (target_has_execution && inferior_thread ()->control.in_infcall)
+     finish either.  Howevever, if target doesn't support asynchronous
+     execution, we must mark all of its threads as stopped, because
+     that's what such targets do when the thread running in an infcall
+     stops.  (The cleanup will call finish_thread_state with
+     minus_one_ptid in that case.)  */
+  if (target_has_execution
+      && inferior_thread ()->control.in_infcall
+      && target_can_async_p ())
     discard_cleanups (old_chain);
   else
     do_cleanups (old_chain);

Re: Inadvertently run inferior threads

[Eli Zaretskii]

Ping!  With the GDB 7.10 branching date looming, could we please
decide ASAP if the patch I proposed for this annoying problem is good
to go in?

TIA.

> Date: Thu, 11 Jun 2015 16:26:10 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gdb-patches@sourceware.org
> 
> [I'm taking this to gdb-patches@, as I'm suggesting a patch.]
> 
> > Date: Wed, 10 Jun 2015 18:50:13 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: gdb@sourceware.org
> > 
> > So 'set_running' is called, and it is called with minus_one_ptid,
> > which then has the effect of marking all the threads as running.
> > 
> > What I don't understand is why doesn't the breakpoint we set at exit
> > from the inferior function countermand that.  I do see the effect of
> > that breakpoint if I turn on infrun debugging:
> > 
> >   infrun: target_wait (-1, status) =
> >   infrun:   4608 [Thread 4608.0x4900],
> >   infrun:   status->kind = stopped, signal = GDB_SIGNAL_TRAP
> >   infrun: TARGET_WAITKIND_STOPPED
> >   infrun: stop_pc = 0x88ba9f
> >   infrun: BPSTAT_WHAT_STOP_SILENT
> >   infrun: stop_waiting
> > 
> > Why don't we mark all threads as stopped when we hit the breakpoint?
> > is that because of #3 above?
> 
> Answering myself: yes.
> 
> > Any ideas how to solve this annoying problem?
> 
> One idea is implemented in the patch below.  I tested it in a
> MinGW-compiled GDB doing native debugging, and it completely solved
> the problem for me: I see additional threads started during an
> infcall, but none of those dreaded "the selected thread is running"
> error messages.
> 
> OK to commit (with a suitable ChangeLog entry)?
> 
> --- gdb/infrun.c~0	2015-01-13 15:14:47 +0200
> +++ gdb/infrun.c	2015-06-11 07:13:54 +0300
> @@ -6508,8 +6508,14 @@ normal_stop (void)
>       running, all without informing the user/frontend about state
>       transition changes.  If this is actually a call command, then the
>       thread was originally already stopped, so there's no state to
> -     finish either.  */
> -  if (target_has_execution && inferior_thread ()->control.in_infcall)
> +     finish either.  Howevever, if target doesn't support asynchronous
> +     execution, we must mark all of its threads as stopped, because
> +     that's what such targets do when the thread running in an infcall
> +     stops.  (The cleanup will call finish_thread_state with
> +     minus_one_ptid in that case.)  */
> +  if (target_has_execution
> +      && inferior_thread ()->control.in_infcall
> +      && target_can_async_p ())
>      discard_cleanups (old_chain);
>    else
>      do_cleanups (old_chain);
> 

Re: Inadvertently run inferior threads

[Pedro Alves]

On 06/13/2015 12:00 PM, Eli Zaretskii wrote:
> Ping!  With the GDB 7.10 branching date looming, could we please
> decide ASAP if the patch I proposed for this annoying problem is good
> to go in?

Off hand, I don't think the patch is correct.  I'll need a bit more
to think this through and give a better response.  I'm not seeing why
target_can_async targets would be special here: target_can_async targets
also stop all threads when they report an event out of target_wait.

I'll go respond to some of your other mails.

Meanwhile, the way to make sure that we don't forget
considering fixing something for the release, is to list it
in the wiki's release page:

 https://sourceware.org/gdb/wiki/GDB_7.10_Release

Thanks,
Pedro Alves

Re: Inadvertently run inferior threads

[Eli Zaretskii]

> Date: Mon, 15 Jun 2015 13:58:13 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> On 06/13/2015 12:00 PM, Eli Zaretskii wrote:
> > Ping!  With the GDB 7.10 branching date looming, could we please
> > decide ASAP if the patch I proposed for this annoying problem is good
> > to go in?
> 
> Off hand, I don't think the patch is correct.  I'll need a bit more
> to think this through and give a better response.  I'm not seeing why
> target_can_async targets would be special here: target_can_async targets
> also stop all threads when they report an event out of target_wait.

One difference is that in the former case, the call to set_running
marks _all_ the threads running.

But I'm okay with making this a MinGW-specific change, since other
platforms need to do something extraordinary to get into this
situation, while on Windows this happens out of my or the inferior's
control, and its effect on the debugging session is so devastating.

> Meanwhile, the way to make sure that we don't forget
> considering fixing something for the release, is to list it
> in the wiki's release page:
> 
>  https://sourceware.org/gdb/wiki/GDB_7.10_Release

Done.  I hope I formatted the entry according to expectations.