Re: [RFA] thread specific breakpoints and single stepping

[Emi SUZUKI <emi-suzuki at tjsys dot co dot jp>]

Hi Daniel, 

thank you for your comment.  

From: Daniel Jacobowitz <drow at false.org>
Subject: Re: [RFA] thread specific breakpoints and single stepping
Date: Fri, 25 Jul 2008 23:01:33 -0400

> On Wed, Jul 09, 2008 at 09:10:38PM +0900, Emi SUZUKI wrote:
> > It works better than mine.  But while I am concerning about
> > single-stepping for software watchpoints, I noticed that we should
> > also check whether a hardware watchpoint is triggered.  
> 
> As this condition gets more complicated, I'm getting worried about
> keeping it in sync with everything else.  Could it be that the logic
> is wrong - we should determine whether a thread hop is necessary later
> in the process?  It should be just like a breakpoint with a false
> condition.

Ah, yes.  I also feel like that I would eventually implement the same
procedures to two different places...

I investigated the condition again, and found that all the need for
thread hopping over a regular breakpoint can determine by calling
bpstat_stop_status as you suggested: even breakpoint_thread_match is
not necessary here.  
So I revised the patch to delete it.  handle_inferior_event was the
last caller of breakpoint_thread_match, but I just left it defined.  

And I found another problem: when a hardware watchpoint trigger
occured while doing software single-stepping, the trigger can be
ignored by a thread hop.  

I think it would be fixed by calling
single_step_breakpoint_inserted_here_p instead of checking
singlestep_breakpoints_inserted_p, to make sure a thread is really
trapped by a software single step breakpoint.  But I don't have a
environment where I can reproduce the situation.  
It may occur on a PPC box...?

And I doubt if the testcases are reasonable now, but at least the
tests fails without a patch, and succeeds with it, on x86-linux-gnu.  

Is that OK?

-- 
Emi SUZUKI / emi-suzuki at tjsys.co.jp

gdb:
2008-08-01  Emi Suzuki  <emi-suzuki@tjsys.co.jp>

	Organize the check for the need of thread hopping.  

	* infrun.c (handle_inferior_event): Remove the call of
	breakpoint_thread_match.  Call single_step_breakpoint_inserted_here_p
	instead of checking singlestep_breakpoint_inserted_p.  
	Update a comment.  
	* breakpoint.c (single_step_breakpoint_inserted_here_p): Make global.  
	Delete prototype.  
	* breakpoint.h (single_step_breakpoint_inserted_here_p): Declare.  

gdb/testsuite:

2008-07-09  Emi Suzuki  <emi-suzuki@tjsys.co.jp>

	* gdb.threads/thread-specific2.exp,
	gdb.threads/thread-specific2.c: New tests. 

Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.301
diff -u -r1.301 infrun.c
--- gdb/infrun.c	22 Jul 2008 02:10:14 -0000	1.301
+++ gdb/infrun.c	1 Aug 2008 11:20:03 -0000
@@ -2241,8 +2241,8 @@
       deferred_step_ptid = null_ptid;
     }
 
-  /* See if a thread hit a thread-specific breakpoint that was meant for
-     another thread.  If so, then step that thread past the breakpoint,
+  /* See if a thread hit a software single-step breakpoint that was set
+     on another thread.  If so, then step that thread past the breakpoint, 
      and continue it.  */
 
   if (stop_signal == TARGET_SIGNAL_TRAP)
@@ -2252,13 +2252,8 @@
       /* Check if a regular breakpoint has been hit before checking
          for a potential single step breakpoint. Otherwise, GDB will
          not see this breakpoint hit when stepping onto breakpoints.  */
-      if (regular_breakpoint_inserted_here_p (stop_pc))
-	{
-	  ecs->random_signal = 0;
-	  if (!breakpoint_thread_match (stop_pc, ecs->ptid))
-	    thread_hop_needed = 1;
-	}
-      else if (singlestep_breakpoints_inserted_p)
+      if (!regular_breakpoint_inserted_here_p (stop_pc)
+	  && single_step_breakpoint_inserted_here_p (stop_pc))
 	{
 	  /* We have not context switched yet, so this should be true
 	     no matter which thread hit the singlestep breakpoint.  */
Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.338
diff -u -r1.338 breakpoint.c
--- gdb/breakpoint.c	28 Jul 2008 17:53:52 -0000	1.338
+++ gdb/breakpoint.c	1 Aug 2008 11:19:37 -0000
@@ -180,8 +180,6 @@
 
 static void ep_skip_leading_whitespace (char **s);
 
-static int single_step_breakpoint_inserted_here_p (CORE_ADDR pc);
-
 static void free_bp_location (struct bp_location *loc);
 
 static struct bp_location *
@@ -8185,7 +8183,7 @@
 
 /* Check whether a software single-step breakpoint is inserted at PC.  */
 
-static int
+int
 single_step_breakpoint_inserted_here_p (CORE_ADDR pc)
 {
   int i;
Index: gdb/breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.76
diff -u -r1.76 breakpoint.h
--- gdb/breakpoint.h	25 Jul 2008 16:12:03 -0000	1.76
+++ gdb/breakpoint.h	1 Aug 2008 11:19:40 -0000
@@ -685,6 +685,8 @@
 
 extern int software_breakpoint_inserted_here_p (CORE_ADDR);
 
+extern int single_step_breakpoint_inserted_here_p (CORE_ADDR pc);
+
 extern int breakpoint_thread_match (CORE_ADDR, ptid_t);
 
 extern void until_break_command (char *, int, int);
Index: gdb/testsuite/gdb.threads/thread-specific2.exp
===================================================================
RCS file: gdb/testsuite/gdb.threads/thread-specific2.exp
diff -N gdb/testsuite/gdb.threads/thread-specific2.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.threads/thread-specific2.exp	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,97 @@
+# Copyright 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/>.
+
+# It tests that a thread-specific breakpoint would not be hopped when 
+# the trapped thread is currently single-stepping or a watchpoint 
+# triggered on the breakpoint.  
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "thread-specific2"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "incdir=${objdir}"]] != "" } {
+    return -1
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+gdb_load ${binfile}
+
+gdb_test "set print sevenbit-strings" ""
+gdb_test "set width 0" ""
+
+if { ! [ runto "thread_entry" ] } then {
+    untested thread-specific2.exp
+    return -1
+}
+
+# Test 1: the target will get stopped on a thread specific breakpoint
+# when it is singile-stepping.  
+set line1 [gdb_get_line_number "thread-specific2.exp: hop over 1"]
+
+gdb_test_multiple "break $line1 thread 1" \
+    "breakpoint $line1 main thread" {
+    -re "Breakpoint (\[0-9\]*) at.* file .*$srcfile, line.*$gdb_prompt $" {
+	set thread_break1 $expect_out(1,string)
+	pass "breakpoint $line1 main thread"
+    }
+}
+
+gdb_test_multiple "step" \
+    "step onto thread-specific breakpoint" {
+    -re "$line1 *.*\r\n$gdb_prompt $" {
+	pass "step onto breakpoint $thread_break1"
+    }
+}
+
+# Test 2: the target will get stopped on a thread specific breakpoint
+# when a watchpoint triggered.  
+set line2 [gdb_get_line_number "thread-specific2.exp: hop over 2"]
+
+gdb_test_multiple "break $line2 thread 1" \
+    "breakpoint $line2 main thread" {
+    -re "Breakpoint (\[0-9\]*) at.* file .*$srcfile, line.*$gdb_prompt $" {
+	set thread_break2 $expect_out(1,string)
+	pass "breakpoint $line2 main thread"
+    }
+}
+
+gdb_test_multiple "watch i" \
+    "watch i" {
+      -re "\[Ww\]atchpoint (\[0-9\]*): i\r\n$gdb_prompt $" {
+      set watch_num $expect_out(1,string)
+      pass "watch i"
+    }
+}
+
+gdb_test_multiple "continue" \
+    "watchpoint $watch_num triggered on thread-specific breakpoint" {
+    -re "\[Ww\]atchpoint $watch_num: i\r\n.*\r\n$gdb_prompt $" {
+	pass "watchpoint $watch_num triggered on breakpoint $thread_break2"
+    }
+}
+
+return 0
Index: gdb/testsuite/gdb.threads/thread-specific2.c
===================================================================
RCS file: gdb/testsuite/gdb.threads/thread-specific2.c
diff -N gdb/testsuite/gdb.threads/thread-specific2.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.threads/thread-specific2.c	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,45 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+void *
+thread_entry (void* args)
+{
+  int i = 0;
+  i += 1;  /* thread-specific2.exp: hop over 1 */
+  i -= 1;  /* thread-specific2.exp: hop over 2 */
+
+  pthread_exit(NULL);
+}
+
+int
+main (void)
+{
+  int res;
+  pthread_t tid;
+  void *thread_status;
+
+  res = pthread_create (&tid, NULL, (void *) thread_entry, NULL);
+
+  /* wait for the thread */
+  pthread_join (tid, &thread_status);
+
+  exit (EXIT_SUCCESS);
+}