[BUG] Quit and "(running)" problem

[BUG] Quit and "(running)" problem

[Pierre Muller]

  I have troubles with CVS HEAD gdb on cygwin,
related to the "(running)" state. But I don't think that
this problem is windows specific...


  Easiest way to reproduce these problems
is to:
Run gdb with itself:
./gdb ./gdb
$ ./gdb ./gdb
GNU gdb (GDB) 6.8.50.20090115-cvs
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
<http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-cygwin".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Setting up the environment for debugging gdb.
During symbol reading, struct/union type gets multiply defined: struct type.
Breakpoint 1 at 0x40b8a3: file ../../purecvs/gdb/utils.c, line 972.
Breakpoint 2 at 0x419086: file ../../purecvs/gdb/cli/cli-cmds.c, line 199.
(top-gdb) start
Temporary breakpoint 3 at 0x40105c: file ../../purecvs/gdb/gdb.c, line 26.
Starting program: /usr/local/src/gdbcvs/build-bare/gdb/gdb.exe
[New Thread 3768.0xd98]
[New Thread 3768.0xb0]

Temporary breakpoint 3, main (argc=1, argv=0xf01f58)
    at ../../purecvs/gdb/gdb.c:26
26      {
(top-gdb) set height 1
(top-gdb) n
---Type <return> to continue, or q <return> to quit---q
Quit
(top-gdb) set height 80
(top-gdb) inf thr
  2 Thread 3768.0xb0  (running)
* 1 Thread 3768.0xd98  (running)
(top-gdb) cont
Continuing.
Cannot execute this command while the selected thread is running.
(top-gdb)

  The problem is the set_running function
introduced for the non-stop mode probably:
the fact on Quitting at the --Type <return> question
bypasses the
 set_running(..,0)
(I discovered that by adding an printout on
each set_running call)
and thus leaves gdb beleaaving that 
the threads are running while non-stop mode is
not even implemented yet on cygwin native gdb!

  This needs a fix!



Pierre Muller
Pascal language support maintainer for GDB

Re: [BUG] Quit and "(running)" problem

[Joel Brobecker]

> (top-gdb) set height 1
> (top-gdb) n
> ---Type <return> to continue, or q <return> to quit---q
> Quit
> (top-gdb) set height 80
> (top-gdb) inf thr
>   2 Thread 3768.0xb0  (running)
> * 1 Thread 3768.0xd98  (running)
> (top-gdb) cont
> Continuing.
> Cannot execute this command while the selected thread is running.

Ugh! :-(

>   This needs a fix!

Right, I agree, and I think we should try to fix it before
we release. Would you mind creating a PR in our database?
I'll reference it in our checklist for the next release.

Thanks,
-- 
Joel

RE: [BUG] Quit and "(running)" problem

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Thursday, January 15, 2009 2:59 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org; 'Pedro Alves'
> Objet : Re: [BUG] Quit and "(running)" problem
> 
> > (top-gdb) set height 1
> > (top-gdb) n
> > ---Type <return> to continue, or q <return> to quit---q
> > Quit
> > (top-gdb) set height 80
> > (top-gdb) inf thr
> >   2 Thread 3768.0xb0  (running)
> > * 1 Thread 3768.0xd98  (running)
> > (top-gdb) cont
> > Continuing.
> > Cannot execute this command while the selected thread is running.
> 
> Ugh! :-(
> 
> >   This needs a fix!
> 
> Right, I agree, and I think we should try to fix it before
> we release. Would you mind creating a PR in our database?
> I'll reference it in our checklist for the next release.
  I just submitted a bug report
http://sourceware.org/bugzilla/show_bug.cgi?id=9747

Hope this will indeed make sure this doesn't reach
the next release...


Pierre Muller
Pascal language support maintainer for GDB

Re: [BUG] Quit and "(running)" problem

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 106 bytes --]

Something like this should handle your case and similars.  Could you try it out, please?

-- 
Pedro Alves

[-- Attachment #2: pr9747.diff --]
[-- Type: text/x-diff, Size: 9202 bytes --]

2009-01-15  Pedro Alves  <pedro@codesourcery.com>

	PR gdb/9747:
	* gdbthread.h (finish_thread_state, finish_thread_state_cleanup):
	Declare.
	* thread.c (finish_thread_state, finish_thread_state_cleanup): New.
	* infrun.c (wait_for_inferior, fetch_inferior_event): If an error
	is thrown while handling an event, finish the thread state.
	* infcmd.c (run_command_1): If an error is thrown while starting
	the inferior, finish the thread state.

---
 gdb/gdbthread.h |   17 +++++++++++++++++
 gdb/infcmd.c    |   19 ++++++++++++++++++-
 gdb/infrun.c    |   51 +++++++++++++++++++++++++++++++++++++++------------
 gdb/thread.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 123 insertions(+), 13 deletions(-)

Index: src/gdb/gdbthread.h
===================================================================
--- src.orig/gdb/gdbthread.h	2009-01-15 21:37:10.000000000 +0000
+++ src/gdb/gdbthread.h	2009-01-15 21:38:06.000000000 +0000
@@ -288,6 +288,23 @@ extern void set_executing (ptid_t ptid, 
 /* Reports if thread PTID is executing.  */
 extern int is_executing (ptid_t ptid);
 
+/* Merge the executing property of thread PTID over to its thread
+   state property (frontend running/stopped view).
+
+   "not executing" -> "stopped"
+   "executing"     -> "running"
+   "exited"        -> "exited"
+
+   If PIDGET (PTID) is -1, go over all threads.
+
+   Notifications are only emitted if the thread state did change.  */
+extern void finish_thread_state (ptid_t ptid);
+
+/* Same as FINISH_THREAD_STATE, but with an interface suitable to be
+   registered as a cleanup.  PTID_P points to the ptid_t that is
+   passed to FINISH_THREAD_STATE.  */
+extern void finish_thread_state_cleanup (void *ptid_p);
+
 /* Commands with a prefix of `thread'.  */
 extern struct cmd_list_element *thread_cmd_list;
 
Index: src/gdb/thread.c
===================================================================
--- src.orig/gdb/thread.c	2009-01-15 21:37:10.000000000 +0000
+++ src/gdb/thread.c	2009-01-15 21:37:11.000000000 +0000
@@ -632,6 +632,55 @@ set_stop_requested (ptid_t ptid, int sto
     observer_notify_thread_stop_requested (ptid);
 }
 
+void
+finish_thread_state (ptid_t ptid)
+{
+  struct thread_info *tp;
+  int all;
+  int any_started = 0;
+
+  all = ptid_equal (ptid, minus_one_ptid);
+
+  if (all || ptid_is_pid (ptid))
+    {
+      for (tp = thread_list; tp; tp = tp->next)
+	{
+ 	  if (tp->state_ == THREAD_EXITED)
+  	    continue;
+	  if (all || ptid_get_pid (ptid) == ptid_get_pid (tp->ptid))
+	    {
+	      if (tp->executing_ && tp->state_ == THREAD_STOPPED)
+		any_started = 1;
+	      tp->state_ = tp->executing_ ? THREAD_RUNNING : THREAD_STOPPED;
+	    }
+	}
+    }
+  else
+    {
+      tp = find_thread_pid (ptid);
+      gdb_assert (tp);
+      if (tp->state_ != THREAD_EXITED)
+	{
+	  if (tp->executing_ && tp->state_ == THREAD_STOPPED)
+	    any_started = 1;
+	  tp->state_ = tp->executing_ ? THREAD_RUNNING : THREAD_STOPPED;
+	}
+    }
+
+  if (any_started)
+    observer_notify_target_resumed (ptid);
+}
+
+void
+finish_thread_state_cleanup (void *arg)
+{
+  ptid_t *ptid_p = arg;
+
+  gdb_assert (arg);
+
+  finish_thread_state (*ptid_p);
+}
+
 /* Prints the list of threads and their details on UIOUT.
    This is a version of 'info_thread_command' suitable for
    use from MI.  
Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2009-01-15 21:37:10.000000000 +0000
+++ src/gdb/infrun.c	2009-01-15 21:37:11.000000000 +0000
@@ -1783,6 +1783,8 @@ wait_for_inferior (int treat_exec_as_sig
 
   while (1)
     {
+      struct cleanup *old_chain;
+
       if (deprecated_target_wait_hook)
 	ecs->ptid = deprecated_target_wait_hook (waiton_ptid, &ecs->ws);
       else
@@ -1795,9 +1797,17 @@ wait_for_inferior (int treat_exec_as_sig
           ecs->ws.value.sig = TARGET_SIGNAL_TRAP;
         }
 
+      /* If an error happens while handling the event, propagate GDB's
+	 knowledge of the executing state to the frontend/user running
+	 state.  */
+      old_chain = make_cleanup (finish_thread_state_cleanup, &minus_one_ptid);
+
       /* Now figure out what to do with the result of the result.  */
       handle_inferior_event (ecs);
 
+      /* No error, don't finish the state yet.  */
+      discard_cleanups (old_chain);
+
       if (!ecs->wait_some_more)
 	break;
     }
@@ -1820,6 +1830,7 @@ fetch_inferior_event (void *client_data)
   struct execution_control_state ecss;
   struct execution_control_state *ecs = &ecss;
   struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
+  struct cleanup *ts_old_chain;
   int was_sync = sync_execution;
 
   memset (ecs, 0, sizeof (*ecs));
@@ -1863,6 +1874,14 @@ fetch_inferior_event (void *client_data)
        thread.  */
     context_switch (ecs->ptid);
 
+  /* If an error happens while handling the event, propagate GDB's
+     knowledge of the executing state to the frontend/user running
+     state.  */
+  if (!non_stop)
+    ts_old_chain = make_cleanup (finish_thread_state_cleanup, &minus_one_ptid);
+  else
+    ts_old_chain = make_cleanup (finish_thread_state_cleanup, &ecs->ptid);
+
   /* Now figure out what to do with the result of the result.  */
   handle_inferior_event (ecs);
 
@@ -1886,6 +1905,9 @@ fetch_inferior_event (void *client_data)
 	inferior_event_handler (INF_EXEC_COMPLETE, NULL);
     }
 
+  /* No error, don't finish the thread states yet.  */
+  discard_cleanups (ts_old_chain);
+
   /* Revert thread and frame.  */
   do_cleanups (old_chain);
 
@@ -4161,9 +4183,23 @@ normal_stop (void)
 {
   struct target_waitstatus last;
   ptid_t last_ptid;
+  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
 
   get_last_target_status (&last_ptid, &last);
 
+  /* If an exception is thrown from this point on, make sure to
+     propagate GDB's knowledge of the executing state to the
+     frontend/user running state.  A QUIT is an easy exception to see
+     here, so do this before any filtered output.  */
+  if (target_has_execution)
+    {
+      if (!non_stop)
+	old_chain = make_cleanup (finish_thread_state_cleanup, &minus_one_ptid);
+      else if (last.kind != TARGET_WAITKIND_SIGNALLED
+	       && last.kind != TARGET_WAITKIND_EXITED)
+	old_chain = make_cleanup (finish_thread_state_cleanup, &last_ptid);
+    }
+
   /* In non-stop mode, we don't want GDB to switch threads behind the
      user's back, to avoid races where the user is typing a command to
      apply to thread x, but GDB switches to thread y before the user
@@ -4383,20 +4419,11 @@ done:
 	/* Delete the breakpoint we stopped at, if it wants to be deleted.
 	   Delete any breakpoint that is to be deleted at the next stop.  */
 	breakpoint_auto_delete (inferior_thread ()->stop_bpstat);
-
-      /* Mark the stopped threads accordingly.  In all-stop, all
-	 threads of all processes are stopped when we get any event
-	 reported.  In non-stop mode, only the event thread stops.  If
-	 we're handling a process exit in non-stop mode, there's
-	 nothing to do, as threads of the dead process are gone, and
-	 threads of any other process were left running.  */
-      if (!non_stop)
-	set_running (minus_one_ptid, 0);
-      else if (last.kind != TARGET_WAITKIND_SIGNALLED
-	       && last.kind != TARGET_WAITKIND_EXITED)
-	set_running (inferior_ptid, 0);
     }
 
+  /* Tell the frontend about the new thread states.  */
+  do_cleanups (old_chain);
+
   /* Look up the hook_stop and run it (CLI internally handles problem
      of stop_command's pre-hook not existing).  */
   if (stop_command)
Index: src/gdb/infcmd.c
===================================================================
--- src.orig/gdb/infcmd.c	2009-01-15 21:37:10.000000000 +0000
+++ src/gdb/infcmd.c	2009-01-15 21:37:11.000000000 +0000
@@ -456,6 +456,8 @@ static void
 run_command_1 (char *args, int from_tty, int tbreak_at_main)
 {
   char *exec_file;
+  struct cleanup *old_chain;
+  ptid_t ptid;
 
   dont_repeat ();
 
@@ -544,14 +546,29 @@ run_command_1 (char *args, int from_tty,
   target_create_inferior (exec_file, get_inferior_args (),
 			  environ_vector (inferior_environ), from_tty);
 
+  /* We're starting off a new process.  When we get out of here, in
+     non-stop mode, finish the state of all threads of that process,
+     but leave other threads alone, as they may be stopped in internal
+     events --- the frontend shouldn't seem them as stopped.  In
+     all-stop, always finish the state of all threads, as we may be
+     resuming more than just the new process.  */
+  if (non_stop)
+    ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
+  else
+    ptid = minus_one_ptid;
+  old_chain = make_cleanup (finish_thread_state_cleanup, &ptid);
+
   /* Pass zero for FROM_TTY, because at this point the "run" command
      has done its thing; now we are setting up the running program.  */
   post_create_inferior (&current_target, 0);
 
   /* Start the target running.  */
   proceed ((CORE_ADDR) -1, TARGET_SIGNAL_0, 0);
-}
 
+  /* Since there was no error, there's no need to finish the thread
+     states here.  */
+  discard_cleanups (old_chain);
+}
 
 static void
 run_command (char *args, int from_tty)

RE: [BUG] Quit and "(running)" problem

[Pierre Muller]

  I can confirm that your patch fixed the simple
test case that I submitted in PR9747.
As such I of course 

  Reading the patch, I was wondering about the
utility of the old_chain cleanup in fetch_inferior_event
function. But this is probably due to my
lack of comprehension of the cleanup chain mechanisms.

  Is it really possible to reach
  do_cleanups (old_chain)
with something else that old_chain
as the top item on the cleanup list?
  I thought that all the cleanups where stored
as local variables, so that all cleanups
that were set in functions called while running any
code called from within the fetch_inferior_event
would be invalid data anyhow at that point,
as the stack might have been overwritten by calls to other functions.


Pierre Muller
Pascal language support maintainer for GDB

Re: [BUG] Quit and "(running)" problem

[Pedro Alves]

A Friday 16 January 2009 08:54:21, Pierre Muller wrote:
>   I can confirm that your patch fixed the simple
> test case that I submitted in PR9747.

Thank you.

>   Reading the patch, I was wondering about the
> utility of the old_chain cleanup in fetch_inferior_event
> function. But this is probably due to my
> lack of comprehension of the cleanup chain mechanisms.
> 
>   Is it really possible to reach
>   do_cleanups (old_chain)
> with something else that old_chain
> as the top item on the cleanup list?

Yes, there's a make_cleanup_restore_current_thread call
there that adds a new cleanup to the chain.

>   I thought that all the cleanups where stored
> as local variables, so that all cleanups
> that were set in functions called while running any
> code called from within the fetch_inferior_event
> would be invalid data anyhow at that point,
> as the stack might have been overwritten by calls to other functions.

Some confusion here.  Instead of trying to explain the basic mechanism
and doing a lousy job at it, I suggest taking a look at the Cleanups section
in internals manual, if you haven't already, which I think explains
it quite nicelly:

 http://sourceware.org/gdb/current/onlinedocs/gdbint_15.html#SEC118

Another way to really understand cleanups is to step through
the make_cleanup, do_cleanups and discard_cleanups functions, it looks
scarrier than it is.


In this particular case, we have:

old_chain +----+- <null_cleanup>
               |
               +- <restore_current_thread> (always run this, wether leaving with an exception or leaving succesfully)
               |
ts_old_chain +-+- <finish_thread_state> (only run if there's an exception)
               
                  [do something that can throw]
             
                  [ if we got pass it sucessfully, discard the <finish_thread_state> cleanup chain ]

                  /* No error, don't finish the thread states yet.  */
                  discard_cleanups (ts_old_chain);

So, at this point we have something like:

old_chain +----+- <null_cleanup>
               |
               +- <restore_current_thread> (always run this, wether leaving with an exception or leaving succesfully)

ts_old_chain (invalid, dangling pointer)

                  /* Revert thread and frame.  */
                  do_cleanups (old_chain);
                    
                  This statement runs the <restore_current_thread> cleanup.

( I mentioned doing a lousy job explaining it.  )

-- 
Pedro Alves

RE: [BUG] Quit and "(running)" problem

[Pierre Muller]

  Thanks for the explanations...
  
  By the way, this also explains why I
got troubles when I tried some code 
with windows OS, where I added some
debug informations using printf_unfiltered into the windows
function that handles console control events.
  The problem was that even printf_unfiltered
adds some cleanups, but this control function
is executed in a newly created thread, which 
leads to big troubles, if a second control event
is generated...

Pierre Muller
Pascal language support maintainer for GDB

  
> -----Message d'origine-----
> De : Pedro Alves [mailto:pedro@codesourcery.com]
> Envoyé : Friday, January 16, 2009 4:13 PM
> À : Pierre Muller
> Cc : 'Joel Brobecker'; gdb-patches@sourceware.org
> Objet : Re: [BUG] Quit and "(running)" problem
> 
> A Friday 16 January 2009 08:54:21, Pierre Muller wrote:
> >   I can confirm that your patch fixed the simple
> > test case that I submitted in PR9747.
> 
> Thank you.
> 
> >   Reading the patch, I was wondering about the
> > utility of the old_chain cleanup in fetch_inferior_event
> > function. But this is probably due to my
> > lack of comprehension of the cleanup chain mechanisms.
> >
> >   Is it really possible to reach
> >   do_cleanups (old_chain)
> > with something else that old_chain
> > as the top item on the cleanup list?
> 
> Yes, there's a make_cleanup_restore_current_thread call
> there that adds a new cleanup to the chain.
> 
> >   I thought that all the cleanups where stored
> > as local variables, so that all cleanups
> > that were set in functions called while running any
> > code called from within the fetch_inferior_event
> > would be invalid data anyhow at that point,
> > as the stack might have been overwritten by calls to other functions.
> 
> Some confusion here.  Instead of trying to explain the basic mechanism
> and doing a lousy job at it, I suggest taking a look at the Cleanups
> section
> in internals manual, if you haven't already, which I think explains
> it quite nicelly:
> 
>  http://sourceware.org/gdb/current/onlinedocs/gdbint_15.html#SEC118
> 
> Another way to really understand cleanups is to step through
> the make_cleanup, do_cleanups and discard_cleanups functions, it looks
> scarrier than it is.
> 
> 
> In this particular case, we have:
> 
> old_chain +----+- <null_cleanup>
>                |
>                +- <restore_current_thread> (always run this, wether
> leaving with an exception or leaving succesfully)
>                |
> ts_old_chain +-+- <finish_thread_state> (only run if there's an
> exception)
> 
>                   [do something that can throw]
> 
>                   [ if we got pass it sucessfully, discard the
> <finish_thread_state> cleanup chain ]
> 
>                   /* No error, don't finish the thread states yet.  */
>                   discard_cleanups (ts_old_chain);
> 
> So, at this point we have something like:
> 
> old_chain +----+- <null_cleanup>
>                |
>                +- <restore_current_thread> (always run this, wether
> leaving with an exception or leaving succesfully)
> 
> ts_old_chain (invalid, dangling pointer)
> 
>                   /* Revert thread and frame.  */
>                   do_cleanups (old_chain);
> 
>                   This statement runs the <restore_current_thread>
> cleanup.
> 
> ( I mentioned doing a lousy job explaining it.  )
> 
> --
> Pedro Alves

Re: [BUG] Quit and "(running)" problem

[Pedro Alves]

On Thursday 15 January 2009 12:39:39, Pierre Muller wrote:
>   I have troubles with CVS HEAD gdb on cygwin,
> related to the "(running)" state. But I don't think that
> this problem is windows specific...

It isn't.

> (top-gdb) set height 1
> (top-gdb) n
> ---Type <return> to continue, or q <return> to quit---q
> Quit

This is one of the reasons I tell people that a pagination
request is an easy way to get exceptions in unexpected
places.  In this case, this is comming from normal_stop, I believe.

As a general guideline, we should try to do *_filtered output only
after having done all the side effects we wanted.

> (top-gdb) set height 80
> (top-gdb) inf thr
>   2 Thread 3768.0xb0  (running)
> * 1 Thread 3768.0xd98  (running)
> (top-gdb) cont
> Continuing.
> Cannot execute this command while the selected thread is running.
> (top-gdb)
> 

>   The problem is the set_running function
> introduced for the non-stop mode probably:
> the fact on Quitting at the --Type <return> question
> bypasses the
>  set_running(..,0)
> (I discovered that by adding an printout on
> each set_running call)
> and thus leaves gdb beleaaving that 
> the threads are running while non-stop mode is
> not even implemented yet on cygwin native gdb!

Yep.

>   This needs a fix!

Believe it or not, I zapped this morning a source tree by mistake
where I had most of the fix for this.  :-(

I'm working on it again.

-- 
Pedro Alves