This is the mail archive of the gdb@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Inadvertently run inferior threads


On 06/23/2015 01:19 PM, Pedro Alves wrote:
> On 06/23/2015 05:07 AM, Doug Evans wrote:
> 
>> Maybe we could give up on trying to cover up the stopped/running state
>> of the thread and just let info threads report something closer to
>> what's actually going on?
>> An asterisk or some such accompanying the output of threads in
>> intermediate states may be a sufficient clue to the the user.
> 
> I strongly disagree.
> 
> Plus, frontends also don't want to be flooded with useless
> *running -> *stopped -> *running transitions.
> 
> Hiding internal stops isn't the complicated part.  It's the opposite
> that is causing problems.  That is, the hiding that threads run at all
> when doing an infcall:
> 
>  (gdb)
>  p malloc (0)
>  &"p malloc (0)\n"
>  ~"$1 = (void *) 0x602010\n"
>  ^done
>  (gdb)
> 
> Note no *running/*stopped above.
> 
> The next question should be: if you do "print sleep (10000)", then
> since the thread / threads was/were never marked running, does that mean that
> the user/frontend can end up issuing another execution command
> that corrupts the ongoing infcall?  The answer is no, but just because
> infcalls are always synchronous, so there's no way to issue any command
> while an infcall is ongoing anyway.  More about it here:
> 
>  https://sourceware.org/ml/gdb-patches/2014-05/msg00273.html
> 
> But fixing this bug may require removing the infcall-specific
> (pretend-it-doesnt-run) suppressions, resulting in something like:
> 
>  (gdb)
>  p malloc (0)
>  *running,thread-id="all"
>  &"p malloc (0)\n"
>  ~"$1 = (void *) 0x602010\n"
>  *stopped,frame={addr="0x000000000040071c",func="main",args=[{name="argc",value="1"}, (...) ,thread-id="1",stopped-threads="all",core="3" ^done
>  (gdb)
> 
> I'm playing with tests and potential solutions.

This makes GDB mark threads as running while doing an infcall,
and sets them back to stopped when the call finishes.  The MI
event suppression is done elsewhere and isn't really affected.
So I'm leaving MI untouched for now.

Eli, could you give it try to check if this fixes your
case too, please?

(I'll audit for related comments that might need updating and
add an alarm call to the test.)

>From 356f189ff985fa53a7ab347590b6ab3ad05c59e2 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 23 Jun 2015 20:06:26 +0100
Subject: [PATCH] Fix for stale "running" threads.

---
 gdb/infcall.c                                 | 22 ++++++++++
 gdb/infrun.c                                  | 32 +++++++--------
 gdb/testsuite/gdb.threads/threads-infcall.c   | 58 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.threads/threads-infcall.exp | 56 ++++++++++++++++++++++++++
 4 files changed, 151 insertions(+), 17 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/threads-infcall.c
 create mode 100644 gdb/testsuite/gdb.threads/threads-infcall.exp

diff --git a/gdb/infcall.c b/gdb/infcall.c
index f79afea..cfdfd3e 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -387,6 +387,7 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc)
   int saved_in_infcall = call_thread->control.in_infcall;
   ptid_t call_thread_ptid = call_thread->ptid;
   int saved_sync_execution = sync_execution;
+  int was_running = call_thread->state == THREAD_RUNNING;
 
   /* Infcalls run synchronously, in the foreground.  */
   if (target_can_async_p ())
@@ -433,6 +434,27 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc)
      CALL_THREAD as it could be invalid if its thread has exited.  */
   call_thread = find_thread_ptid (call_thread_ptid);
 
+  /* If the infcall does NOT succeed, normal_stop will have already
+     finished the thread states.  However, on success, normal_stop
+     defers here, so that we can set back the thread states to what
+     they were before the call.  We use finish_thread_state instead of
+     set_running to also handle new threads that might have spawned
+     while the call was running -- if stopping, must mark them stopped
+     too.  The main cases to handle are:
+
+     - "(gdb) print foo ()", or any other command that evaluates an
+     expression at the prompt.  (The thread was marked stopped before.)
+
+     - "(gdb) break foo if return_false()" or similar cases where we
+     do an infcall while handling an event (while the thread is still
+     marked running).  In this example, whether the condition
+     evaluates true and thus we'll present a user-visible stop is
+     decided elsewhere.  */
+  if (!was_running
+      && ptid_equal (call_thread_ptid, inferior_ptid)
+      && stop_stack_dummy == STOP_STACK_DUMMY)
+    finish_thread_state (user_visible_resume_ptid (0));
+
   enable_watchpoints_after_interactive_call_stop ();
 
   /* Call breakpoint_auto_delete on the current contents of the bpstat
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d8eb0b0..ca2e066 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2264,11 +2264,8 @@ resume (enum gdb_signal sig)
 	     requests finish.  The thread is not executing at this
 	     point, and the call to set_executing will be made later.
 	     But we need to call set_running here, since from the
-	     user/frontend's point of view, threads were set running.
-	     Unless we're calling an inferior function, as in that
-	     case we pretend the inferior doesn't run at all.  */
-	  if (!tp->control.in_infcall)
-	    set_running (user_visible_resume_ptid (user_step), 1);
+	     user/frontend's point of view, threads were set running.  */
+	  set_running (user_visible_resume_ptid (user_step), 1);
 	  discard_cleanups (old_cleanups);
 	  return;
 	}
@@ -2346,10 +2343,8 @@ resume (enum gdb_signal sig)
   /* Even if RESUME_PTID is a wildcard, and we end up resuming less
      (e.g., we might need to step over a breakpoint), from the
      user/frontend's point of view, all threads in RESUME_PTID are now
-     running.  Unless we're calling an inferior function, as in that
-     case pretend we inferior doesn't run at all.  */
-  if (!tp->control.in_infcall)
-    set_running (resume_ptid, 1);
+     running.  */
+  set_running (resume_ptid, 1);
 
   /* Maybe resume a single thread after all.  */
   if ((step || thread_has_single_step_breakpoints_set (tp))
@@ -6672,7 +6667,9 @@ normal_stop (void)
      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)
+  if (target_has_execution
+      && inferior_thread ()->control.in_infcall
+      && stop_stack_dummy == STOP_STACK_DUMMY)
     discard_cleanups (old_chain);
   else
     do_cleanups (old_chain);
@@ -6730,16 +6727,17 @@ normal_stop (void)
 done:
   annotate_stopped ();
 
-  /* Suppress the stop observer if we're in the middle of:
+  /* Suppress the stop observer if an outer layer has special
+     processing for the event.  The possible cases are:
 
-     - a step n (n > 1), as there still more steps to be done.
+     - step n (n > 1), as there still more steps to be done.
 
-     - a "finish" command, as the observer will be called in
-       finish_command_continuation, so it can include the inferior
-       function's return value.
+     - The "finish" command.  The normal_stop observer will be called
+       in finish_command_continuation instead, so it can include the
+       inferior function's return value.
 
-     - calling an inferior function, as we pretend we inferior didn't
-       run at all.  The return value of the call is handled by the
+     - When calling an inferior function, which is always done
+       synchronously.  The return value of the call is handled by the
        expression evaluator, through call_function_by_hand.  */
 
   if (!target_has_execution
diff --git a/gdb/testsuite/gdb.threads/threads-infcall.c b/gdb/testsuite/gdb.threads/threads-infcall.c
new file mode 100644
index 0000000..62fd73b
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/threads-infcall.c
@@ -0,0 +1,58 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <pthread.h>
+
+volatile int count;
+
+static int
+foo (void)
+{
+  usleep (1);
+}
+
+static void *
+thread_function (void *arg)
+{
+  pthread_t thread;
+
+  printf ("created thread %d\n", ++count);
+  while (1)
+    {
+      foo ();
+    }
+}
+
+void
+new_thread (void)
+{
+  pthread_t thread;
+
+  pthread_create (&thread, NULL, thread_function, NULL);
+}
+
+int
+main (int argc, char **argv)
+{
+  while (1)
+    {
+      usleep (1);
+    }
+}
diff --git a/gdb/testsuite/gdb.threads/threads-infcall.exp b/gdb/testsuite/gdb.threads/threads-infcall.exp
new file mode 100644
index 0000000..d10c9bb
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/threads-infcall.exp
@@ -0,0 +1,56 @@
+# Copyright (C) 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+set executable ${testfile}
+
+# This test verifies that a watchpoint is detected in a multithreaded
+# program so the test is only meaningful on a system with hardware
+# watchpoints.
+if {[skip_hw_watchpoint_tests]} {
+    return 0
+}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+	 executable [list debug "incdir=${objdir}"]] != "" } {
+    return -1
+}
+
+clean_restart $executable
+
+if ![runto_main] {
+    continue
+}
+
+# Set a thread-specific breakpoint that the wrong thread trips on
+# while running the infcall.  Check that no thread ends up in stale
+# "running" state.
+gdb_test "b foo thread 1" "Breakpoint .*$srcfile.*"
+
+for {set i 0} { $i < 3} {incr i} {
+    with_test_prefix "iter $i" {
+	gdb_test "p new_thread ()" ".*"
+
+	set message "no thread marked running"
+	gdb_test_multiple "info threads" $message {
+	    -re ".*running.*$gdb_prompt $" {
+		fail $message
+	    }
+	    -re "$gdb_prompt $" {
+		pass $message
+	    }
+	}
+    }
+}
-- 
1.9.3



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]