Re: [RFA] Fix hand called function when another thread has hit a bp.

[Doug Evans <dje@google.com>]

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

On Wed, Dec 17, 2008 at 11:14 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Doug Evans wrote:
>
>> > The problem arises when scheduler locking is switched on.  Actually,
>> > I think there are really two problems.  First of all, after we've
>> > switched back and single-stepped over an already-hit breakpoint via
>> > the prepare_to_proceed logic, we'll continue only a single thread
>> > if scheduler-locking is on -- and that is the wrong thread.  The
>> > prepare_to_proceed logic only explicitly switches *back* to the
>> > user-selected thread if the user was *stepping* (that's the
>> > deferred_step_ptid logic).  For scheduler-locking, we should probably
>> > switch back always ...
>>
>> If scheduler locking is on, why is there any switching at all?  If
>> scheduler-locking is on and I switch threads I'd want gdb to defer
>> single-stepping the other thread over its breakpoint until the point
>> when I make that other thread runnable.
>>
>> Also, I think removing the notion of one previously stopped thread and
>> generalizing it to not caring, i.e. checking the status of every
>> stopped thread before resuming will simplify things and fix a few bugs
>> along the way.  IOW, make deferred_ptid go away.
>
> That may indeed be the best solution.  The simplest implementation
> might be to simply remember in a per-thread flag the fact that the
> last time this thread stopped, we reported a breakpoint at stop_pc
> (which would have to be made per-thread as well, but we'd already
> decided this should happen anyway).
>
> This information could then be consulted the next time the thread
> is made runnable again.
>
>> > The second problem is more a problem of definition: even if the
>> > first problem above were fixed, we've have to single-step the other
>> > thread at least once to get over the breakpoint.  This would seem
>> > to violate the definition of scheduler locking if interpreted
>> > absolutely strictly.  Now you could argue that as the user should
>> > never be aware of that single step, it doesn't really matter.
>>
>> I'm not sure how we necessarily have a violation of the definition of
>> scheduler locking.
>
> This is just saying the same you said in other words: "If scheduler-
> locking is on and I switch threads I'd want gdb to defer single-
> stepping the other thread over its breakpoint until the point when
> I make that other thread runnable."
>
> I.e. "definition of scheduler locking" meaning: no other thread but
> the one selected by the user runs, ever.  Today, this is not true,
> in the case of single-stepping over a breakpoint in another thread.

Hi.  Here's an updated version of the patch.
Handling the restart after several threads are all stopped at a
breakpoint (via scheduler-locking = on), is left for a later patch
(it's happens more rarely).

Ok to check in?

2009-02-23  Doug Evans  <dje@google.com>

        * infrun.c (prepare_to_proceed): Document.  Assert !non_stop.
        If scheduler-locking is enabled, we're not going to be singlestepping
        any other previously stopped thread.

        * gdb.threads/hand-call-in-threads.exp: New file.
        * gdb.threads/hand-call-in-threads.c: New file.

[-- Attachment #2: gdb-090223-schedlock-4.patch.txt --]
[-- Type: text/plain, Size: 10722 bytes --]

2009-02-23  Doug Evans  <dje@google.com>

	* infrun.c (prepare_to_proceed): Document.  Assert !non_stop.
	If scheduler-locking is enabled, we're not going to be singlestepping
	any other previously stopped thread.

	* gdb.threads/hand-call-in-threads.exp: New file.
	* gdb.threads/hand-call-in-threads.c: New file.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.359
diff -u -p -u -p -r1.359 infrun.c
--- infrun.c	21 Feb 2009 16:14:48 -0000	1.359
+++ infrun.c	24 Feb 2009 00:37:03 -0000
@@ -1239,13 +1239,21 @@ clear_proceed_status (void)
     }
 }
 
-/* This should be suitable for any targets that support threads. */
+/* Check the current thread against the thread that reported the most recent
+   event.  If a step-over is required return TRUE and set the current thread
+   to the old thread.  Otherwise return FALSE.
+
+   This should be suitable for any targets that support threads. */
 
 static int
 prepare_to_proceed (int step)
 {
   ptid_t wait_ptid;
   struct target_waitstatus wait_status;
+  int schedlock_enabled;
+
+  /* With non-stop mode on, threads are always handled individually.  */
+  gdb_assert (! non_stop);
 
   /* Get the last target status returned by target_wait().  */
   get_last_target_status (&wait_ptid, &wait_status);
@@ -1257,9 +1265,15 @@ prepare_to_proceed (int step)
       return 0;
     }
 
+  schedlock_enabled = (scheduler_mode == schedlock_on
+		       || (scheduler_mode == schedlock_step
+			   && step));
+
   /* Switched over from WAIT_PID.  */
   if (!ptid_equal (wait_ptid, minus_one_ptid)
-      && !ptid_equal (inferior_ptid, wait_ptid))
+      && !ptid_equal (inferior_ptid, wait_ptid)
+      /* Don't single step WAIT_PID if scheduler locking is on.  */
+      && !schedlock_enabled)
     {
       struct regcache *regcache = get_thread_regcache (wait_ptid);
 
Index: testsuite/gdb.threads/hand-call-in-threads.c
===================================================================
RCS file: testsuite/gdb.threads/hand-call-in-threads.c
diff -N testsuite/gdb.threads/hand-call-in-threads.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.threads/hand-call-in-threads.c	24 Feb 2009 00:37:04 -0000
@@ -0,0 +1,127 @@
+/* Test case for hand function calls in multi-threaded program.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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 <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <time.h>
+#include <unistd.h>
+
+#ifndef NR_THREADS
+#define NR_THREADS 4
+#endif
+
+int thread_count;
+
+pthread_mutex_t thread_count_mutex;
+
+pthread_cond_t thread_count_condvar;
+
+void
+incr_thread_count (void)
+{
+  pthread_mutex_lock (&thread_count_mutex);
+  ++thread_count;
+  if (thread_count == NR_THREADS)
+    pthread_cond_signal (&thread_count_condvar);
+  pthread_mutex_unlock (&thread_count_mutex);
+}
+
+void
+cond_wait (pthread_cond_t *cond, pthread_mutex_t *mut)
+{
+  pthread_mutex_lock (mut);
+  pthread_cond_wait (cond, mut);
+  pthread_mutex_unlock (mut);
+}
+
+void
+noreturn (void)
+{
+  pthread_mutex_t mut;
+  pthread_cond_t cond;
+
+  pthread_mutex_init (&mut, NULL);
+  pthread_cond_init (&cond, NULL);
+
+  /* Wait for a condition that will never be signaled, so we effectively
+     block the thread here.  */
+  cond_wait (&cond, &mut);
+}
+
+void *
+forever_pthread (void *unused)
+{
+  incr_thread_count ();
+  noreturn ();
+}
+
+void
+hand_call (void)
+{
+}
+
+/* Wait until all threads are running.  */
+
+void
+wait_all_threads_running (void)
+{
+  pthread_mutex_lock (&thread_count_mutex);
+  if (thread_count == NR_THREADS)
+    {
+      pthread_mutex_unlock (&thread_count_mutex);
+      return;
+    }
+  pthread_cond_wait (&thread_count_condvar, &thread_count_mutex);
+  if (thread_count == NR_THREADS)
+    {
+      pthread_mutex_unlock (&thread_count_mutex);
+      return;
+    }
+  pthread_mutex_unlock (&thread_count_mutex);
+  printf ("failed waiting for all threads to start\n");
+  abort ();
+}
+
+/* Called when all threads are running.
+   Easy place for a breakpoint.  */
+
+void
+all_threads_running (void)
+{
+}
+
+int
+main (void)
+{
+  pthread_t forever[NR_THREADS];
+  int i;
+
+  pthread_mutex_init (&thread_count_mutex, NULL);
+  pthread_cond_init (&thread_count_condvar, NULL);
+
+  for (i = 0; i < NR_THREADS; ++i)
+    pthread_create (&forever[i], NULL, forever_pthread, NULL);
+
+  wait_all_threads_running ();
+  all_threads_running ();
+
+  return 0;
+}
+
Index: testsuite/gdb.threads/hand-call-in-threads.exp
===================================================================
RCS file: testsuite/gdb.threads/hand-call-in-threads.exp
diff -N testsuite/gdb.threads/hand-call-in-threads.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.threads/hand-call-in-threads.exp	24 Feb 2009 00:37:04 -0000
@@ -0,0 +1,160 @@
+# Copyright (C) 2004, 2007, 2008 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/>.
+
+# Test making hand function calls in multiple threads.
+
+set NR_THREADS 4
+
+if $tracelevel then {
+	strace $tracelevel
+}
+
+set testfile "hand-call-in-threads"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "incdir=${objdir}" "additional_flags=-DNR_THREADS=$NR_THREADS"]] != "" } {
+    return -1
+}
+
+# Some targets can't do function calls, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    setup_xfail "*-*-*" 2416
+    fail "This target can not call functions"
+    continue
+}
+
+proc get_dummy_frame_number { } {
+  global gdb_prompt
+
+  send_gdb "bt\n"
+  gdb_expect {
+    -re "#(\[0-9\]*) *<function called from gdb>.*$gdb_prompt $"
+      {
+	return $expect_out(1,string)
+      }
+    -re "$gdb_prompt $"
+      {
+	return ""
+      }
+    timeout
+      {
+	return ""
+      }
+  }
+  return ""
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if { ![runto_main] } {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "break all_threads_running" \
+         "Breakpoint 2 at .*: file .*${srcfile}, line .*" \
+         "breakpoint on all_threads_running"
+
+gdb_test "break hand_call" \
+         "Breakpoint 3 at .*: file .*${srcfile}, line .*" \
+         "breakpoint on hand_call"
+
+# Run the program and make sure GDB reports that we stopped after
+# hitting breakpoint 2 in all_threads_running().
+
+gdb_test "continue" \
+         ".*Breakpoint 2, all_threads_running ().*" \
+         "run to all_threads_running"
+
+# Before we start making hand function calls, turn on scheduler locking.
+
+gdb_test "set scheduler-locking on" "" "enable scheduler locking"
+gdb_test "show scheduler-locking" ".* locking scheduler .* is \"on\"." "show scheduler locking on"
+
+# Now hand-call a function in each thread, having the function
+# stop without returning.
+
+# Add one for the main thread.
+set total_nr_threads [expr $NR_THREADS + 1]
+
+# Thread numbering in gdb is origin-1, so begin numbering at 1.
+for { set i 1 } { $i <= $total_nr_threads } { incr i } {
+    set thread_nr $i
+    gdb_test "thread $thread_nr" "" "prepare to make hand call, thread $thread_nr"
+    gdb_test "call hand_call()" "Breakpoint 3, .*" "hand call, thread $thread_nr"
+}
+
+# Now have each hand-called function return.
+
+# Turn confirmation off for the "return" command.
+gdb_test "set confirm off" ""
+
+clear_xfail "*-*-*"
+
+for { set i 1 } { $i <= $total_nr_threads } { incr i } {
+    set thread_nr $i
+    gdb_test "thread $thread_nr" "" "prepare to discard hand call, thread $thread_nr"
+    set frame_number [get_dummy_frame_number]
+    if { "$frame_number" == "" } {
+	fail "dummy stack frame number, thread $thread_nr"
+	setup_xfail "*-*-*"
+	# Need something.
+	set frame_number 0
+    } else {
+	pass "dummy stack frame number, thread $thread_nr"
+    }
+    # Pop the dummy frame.
+    gdb_test "frame $frame_number" "" "setting frame, thread $thread_nr"
+    gdb_test "return" "" "discard hand call, thread $thread_nr"
+    # In case getting the dummy frame number failed, re-enable for next iter.
+    clear_xfail "*-*-*"
+}
+
+# Make sure all dummy frames got popped.
+
+gdb_test_multiple "maint print dummy-frames" "all dummies popped" {
+    -re ".*stack=.*$gdb_prompt $" {
+	fail "all dummies popped"
+    }
+    -re ".*$gdb_prompt $" {
+	pass "all dummies popped"
+    }
+}
+
+# Before we resume the full program, turn of scheduler locking.
+gdb_test "set scheduler-locking off" "" "disable scheduler locking"
+gdb_test "show scheduler-locking" ".* locking scheduler .* is \"off\"." "show scheduler locking off"
+
+# Continue one last time, the program should exit normally.
+#
+# ??? This currently doesn't work because gdb doesn't know how to singlestep
+# over reported breakpoints that weren't in the last thread to run.
+# Fixing this first requires defining what the correct behaviour is.
+# Commented out until then.
+#
+# Manually set the thread back to the first thread: the program is still at
+# the all_threads_running breakpoint, which wasn't the last thread to run,
+# and gdb doesn't know how to singlestep over reported breakpoints that
+# weren't in the last thread to run.
+#gdb_test "thread 1" "" "set thread to 1, prepare to resume"
+#
+#gdb_continue_to_end "hand-call-in-threads"
+
+return 0