Use is_stopped, instead of !is_running

Use is_stopped, instead of !is_running

[Pedro Alves]

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

This patches fixes a couple of left over instances of !is_running to use
is_stopped instead, and adds a missing check for is_exited in
get_selected_block, which I tripped on while development the yet
unsubmitted non-stop remote target.

I'm adding more comments to gdbthread.h to describe why.  Basically,
the thread state is not a boolean anymore.

I'll soon post a description of these states in the internal manual
under the currently empty section 3.5 - Thread Handling.  Let me
know if it should be placed somewhere else.

Tested on x86_64-unknown-linux-gnu sync and async.

OK?

-- 
Pedro Alves

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

2008-07-29  Pedro Alves  <pedro@codesourcery.com>

	* gdbthread.h: Add comments.
	* stack.c (get_selected_block): Return 0 on an exited thread.
	* top.c (execute_command): Check for is_stopped, not !is_running.
	* event-top.c (command_handler): Likewise.

---
 gdb/event-top.c |    2 +-
 gdb/gdbthread.h |   33 ++++++++++++++++++++++++++-------
 gdb/stack.c     |    3 +++
 gdb/top.c       |    2 +-
 4 files changed, 31 insertions(+), 9 deletions(-)

Index: src/gdb/gdbthread.h
===================================================================
--- src.orig/gdb/gdbthread.h	2008-07-29 13:25:21.000000000 +0100
+++ src/gdb/gdbthread.h	2008-07-29 13:25:42.000000000 +0100
@@ -210,21 +210,40 @@ extern void switch_to_thread (ptid_t pti
    If PIDGET (PTID) is -1, marks all threads.  */
 extern void set_running (ptid_t ptid, int running);
 
-/* Reports if thread PTID is known to be running right now.  */
-extern int is_running (ptid_t ptid);
+/* NOTE: Since the thread state is not a boolean, most times, you do
+   not want to check it with negation.  If you really want to check if
+   the thread is stopped,
 
-/* Reports if any thread is known to be running right now.  */
-extern int any_running (void);
+    use (good):
+
+     if (is_stopped (ptid))
+
+    instead of (bad):
+
+     if (!is_running (ptid))
+
+   The latter also returns true on exited threads, most likelly not
+   what you want.  */
+
+/* Reports if in the frontend's perpective, thread PTID is running.  */
+extern int is_running (ptid_t ptid);
 
 /* Is this thread listed, but known to have exited?  We keep it listed
    (but not visible) until it's safe to delete.  */
 extern int is_exited (ptid_t ptid);
 
-/* Is this thread stopped?  */
+/* In the frontend's perpective, is this thread stopped?  */
 extern int is_stopped (ptid_t ptid);
 
-/* Marks thread PTID as executing, or as stopped.
-   If PIDGET (PTID) is -1, marks all threads.  */
+/* In the frontend's perpective is there any thread running?  */
+extern int any_running (void);
+
+/* Marks thread PTID as executing, or not.  If PIDGET (PTID) is -1,
+   marks all threads.
+
+   Note that this is different from the running state.  See the
+   description of state_ and executing_ fields of struct
+   thread_info.  */
 extern void set_executing (ptid_t ptid, int executing);
 
 /* Reports if thread PTID is executing.  */
Index: src/gdb/stack.c
===================================================================
--- src.orig/gdb/stack.c	2008-07-29 13:25:21.000000000 +0100
+++ src/gdb/stack.c	2008-07-29 13:25:42.000000000 +0100
@@ -1644,6 +1644,9 @@ get_selected_block (CORE_ADDR *addr_in_b
   if (!target_has_stack)
     return 0;
 
+  if (is_exited (inferior_ptid))
+    return 0;
+
   if (is_executing (inferior_ptid))
     return 0;
 
Index: src/gdb/top.c
===================================================================
--- src.orig/gdb/top.c	2008-07-29 13:25:21.000000000 +0100
+++ src/gdb/top.c	2008-07-29 13:25:42.000000000 +0100
@@ -484,7 +484,7 @@ Cannot execute this command without a li
   /* FIXME:  This should be cacheing the frame and only running when
      the frame changes.  */
 
-  if (target_has_stack && !is_running (inferior_ptid))
+  if (target_has_stack && is_stopped (inferior_ptid))
     {
       flang = get_frame_language ();
       if (!warned
Index: src/gdb/event-top.c
===================================================================
--- src.orig/gdb/event-top.c	2008-07-29 13:25:21.000000000 +0100
+++ src/gdb/event-top.c	2008-07-29 13:25:42.000000000 +0100
@@ -518,7 +518,7 @@ command_handler (char *command)
   /* Do any commands attached to breakpoint we stopped at. Only if we
      are always running synchronously. Or if we have just executed a
      command that doesn't start the target. */
-  if (!target_can_async_p () || !is_running (inferior_ptid))
+  if (!target_can_async_p () || is_stopped (inferior_ptid))
     {
       bpstat_do_actions (&stop_bpstat);
       do_cleanups (old_chain);

Re: Use is_stopped, instead of !is_running

[Thiago Jung Bauermann]

On Tue, 2008-07-29 at 14:59 +0100, Pedro Alves wrote:
> I'm adding more comments to gdbthread.h to describe why.  Basically,
> the thread state is not a boolean anymore.

If the thread state is not a boolean, then IMHO offering convenient
functions which treat it as such and then putting a warning in the
comment is really setting up a subtle trap for GDB hackers... :-)

What about just nuking is_running and is_stopping, and making everybody
use is_thread_state directly? That would make this reality explicit.
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

Re: Use is_stopped, instead of !is_running

[Pedro Alves]

On Tuesday 29 July 2008 16:59:08, Thiago Jung Bauermann wrote:
> If the thread state is not a boolean, then IMHO offering convenient
> functions which treat it as such and then putting a warning in the
> comment is really setting up a subtle trap for GDB hackers... :-)
>
> What about just nuking is_running and is_stopping, and making everybody
> use is_thread_state directly? That would make this reality explicit.

It might not look like it, but I'm doing baby steps in that
direction.  :-)

There are target_has_execution checks inside the is_XXX
functions that I'd like to cleanly get rid of.

-- 
Pedro Alves

Re: Use is_stopped, instead of !is_running

[Joel Brobecker]

> 2008-07-29  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* gdbthread.h: Add comments.
> 	* stack.c (get_selected_block): Return 0 on an exited thread.
> 	* top.c (execute_command): Check for is_stopped, not !is_running.
> 	* event-top.c (command_handler): Likewise.

Looks OK to me. It took me a while to decide that it was OK to leave
one instance inside inf-loop.c, but I think the !is_running there is OK.

-- 
Joel

Re: Use is_stopped, instead of !is_running

[Pedro Alves]

On Saturday 09 August 2008 15:30:45, Joel Brobecker wrote:
> > 2008-07-29  Pedro Alves  <pedro@codesourcery.com>
> >
> > 	* gdbthread.h: Add comments.
> > 	* stack.c (get_selected_block): Return 0 on an exited thread.
> > 	* top.c (execute_command): Check for is_stopped, not !is_running.
> > 	* event-top.c (command_handler): Likewise.
>
> Looks OK to me. It took me a while to decide that it was OK to leave
> one instance inside inf-loop.c, but I think the !is_running there is OK.

Yeah, it actually also took me a while to decide what to do with that
check, and it's just for a log printf.

Thanks, Joel, I checked it in.  I'm posting an internal docs blurb
about these states next.

-- 
Pedro Alves