This is the mail archive of the gdb-patches@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]

Fix spurious x86 gdbserver failures in threaded tests


I was occasionaly seeing some threaded tests fail with random
SIGTRAPs against x86-64 gdbserver.  For example,

 $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-gdbserver watch_thread_num.exp"; done)
 ... eventually ...
 FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 1
 FAIL: gdb.base/watch_thread_num.exp: Check thread that triggered iteration 1

The gdb.log shows:
 Program received signal SIGTRAP, Trace/breakpoint trap.
 [Switching to Thread 14687]
 0x00000000004006f8 in thread_function (arg=0x3) at ../../../src/gdb/testsuite/gdb.base/watch_thread_num.c:55
 55          int my_number = (long) arg;
 (gdb) FAIL: gdb.base/watch_thread_num.exp: Watchpoint triggered iteration 1

Looking a bit closer, one sees GDB setting a breakpoint
at 0x4006f7, and the random SIGTRAP being reported for 0x4006f8.
This is usually indicative of PC after break adjustment going wrong
somewhere.  Indeed that's the problem --- cancel_breakpoint
isn't actually managing to cancel any breakpoint.  That trap was
left pending, but when it was reported to GDB, the breakpoint was
gone already...

There are three issues causing this problem.

First, cancel_breakpoint checks if a trap is inserted in memory
like so:

  if ((*the_low_target.breakpoint_at) (lwp->stop_pc))
    {

The problem is then in the breakpoint_at callback, in this
case, x86_breakpoint_at.  x86_breakpoint_at is using
read_inferior_memory to check if a trap is inserted
at a given address.  Problem is, read_inferior_memory cooks
the returned buffer to mask out gdbserver managed breakpoints.
This function should be reading raw memory instead.  The problem
became much more visible with Z0 breakpoints, as now
read_inferior_memory masks out GDB breakpoints as well.
This callback in most archs is already reading memory correctly.
According to:

 $ grep read_inferior_memory linux-*-low.c
 linux-m68k-low.c:  read_inferior_memory (pc, c, 2);
 linux-s390-low.c:  read_inferior_memory (pc, c, s390_breakpoint_len);

m68k and s390 should be fixed in the same way.


The second issue is, cancel_breakpoints_call does this
(amonst others) check before calling cancel_breakpoint:

  if (...
      && get_lwp_thread (lp)->last_status.kind == TARGET_WAITKIND_IGNORE
     ...
     && cancel_breakpoint (lp))
   /* Throw away the SIGTRAP */
   ...

But, I forgot to merge in a bit that clears new thread's
last_status.kind to TARGET_WAITKIND_IGNORE.  Instead, the field
was being left memset'ed as 0, which happens to be
TARGET_WAITKIND_EXITED.


The third issue I found by inspection.  linux_resume_one_thread had
a path that didn't clear last_status.kind to TARGET_WAITKIND_IGNORE.

This patch fixes all these issues, and cleans up a couple
bits I noticed while debugging this.

Tested against x86_64-linux gdbserver and applied.

-- 
Pedro Alves

2010-04-03  Pedro Alves  <pedro@codesourcery.com>

	gdb/gdbserver/
	* inferiors.c (add_thread): Set last_status kind to
	TARGET_WAITKIND_IGNORE.
	* linux-low.c (cancel_breakpoint): Remove unnecessary regcache
	fetch.  Use ptid_of.  Avoid unnecessary get_lwp_thread calls.
	(linux_wait_1): Move `thread' local definition to block that uses
	it.  Don't NULL initialize `event_child'.
	(linux_resume_one_thread): Avoid unnecessary get_lwp_thread calls.
	Alway set the thread's last_status to TARGET_WAITKIND_IGNORE.
	* linux-x86-low.c (x86_breakpoint_at): Read raw memory.

---
 gdb/gdbserver/inferiors.c     |    1 +
 gdb/gdbserver/linux-low.c     |   18 ++++++++----------
 gdb/gdbserver/linux-x86-low.c |    2 +-
 3 files changed, 10 insertions(+), 11 deletions(-)

Index: src/gdb/gdbserver/inferiors.c
===================================================================
--- src.orig/gdb/gdbserver/inferiors.c	2010-04-03 20:48:57.000000000 +0100
+++ src/gdb/gdbserver/inferiors.c	2010-04-03 20:49:35.000000000 +0100
@@ -171,6 +171,7 @@ add_thread (ptid_t thread_id, void *targ
   memset (new_thread, 0, sizeof (*new_thread));
 
   new_thread->entry.id = thread_id;
+  new_thread->last_status.kind = TARGET_WAITKIND_IGNORE;
 
   add_inferior_to_list (&all_threads, & new_thread->entry);
 
Index: src/gdb/gdbserver/linux-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-low.c	2010-04-03 19:54:09.000000000 +0100
+++ src/gdb/gdbserver/linux-low.c	2010-04-03 22:30:18.000000000 +0100
@@ -1125,14 +1125,11 @@ static int
 cancel_breakpoint (struct lwp_info *lwp)
 {
   struct thread_info *saved_inferior;
-  struct regcache *regcache;
 
   /* There's nothing to do if we don't support breakpoints.  */
   if (!supports_breakpoints ())
     return 0;
 
-  regcache = get_thread_regcache (get_lwp_thread (lwp), 1);
-
   /* breakpoint_at reads from current inferior.  */
   saved_inferior = current_inferior;
   current_inferior = get_lwp_thread (lwp);
@@ -1142,13 +1139,13 @@ cancel_breakpoint (struct lwp_info *lwp)
       if (debug_threads)
 	fprintf (stderr,
 		 "CB: Push back breakpoint for %s\n",
-		 target_pid_to_str (lwp->head.id));
+		 target_pid_to_str (ptid_of (lwp)));
 
       /* Back up the PC if necessary.  */
       if (the_low_target.decr_pc_after_break)
 	{
 	  struct regcache *regcache
-	    = get_thread_regcache (get_lwp_thread (lwp), 1);
+	    = get_thread_regcache (current_inferior, 1);
 	  (*the_low_target.set_pc) (regcache, lwp->stop_pc);
 	}
 
@@ -1161,7 +1158,7 @@ cancel_breakpoint (struct lwp_info *lwp)
 	fprintf (stderr,
 		 "CB: No breakpoint found at %s for [%s]\n",
 		 paddress (lwp->stop_pc),
-		 target_pid_to_str (lwp->head.id));
+		 target_pid_to_str (ptid_of (lwp)));
     }
 
   current_inferior = saved_inferior;
@@ -1584,8 +1581,7 @@ linux_wait_1 (ptid_t ptid,
 	      struct target_waitstatus *ourstatus, int target_options)
 {
   int w;
-  struct thread_info *thread = NULL;
-  struct lwp_info *event_child = NULL;
+  struct lwp_info *event_child;
   int options;
   int pid;
   int step_over_finished;
@@ -1611,6 +1607,8 @@ retry:
       && !ptid_equal (cont_thread, null_ptid)
       && !ptid_equal (cont_thread, minus_one_ptid))
     {
+      struct thread_info *thread;
+
       thread = (struct thread_info *) find_inferior_id (&all_threads,
 							cont_thread);
 
@@ -2615,7 +2613,7 @@ linux_resume_one_thread (struct inferior
 
       /* For stop requests, we're done.  */
       lwp->resume = NULL;
-      get_lwp_thread (lwp)->last_status.kind = TARGET_WAITKIND_IGNORE;
+      thread->last_status.kind = TARGET_WAITKIND_IGNORE;
       return 0;
     }
 
@@ -2635,7 +2633,6 @@ linux_resume_one_thread (struct inferior
 
       step = (lwp->resume->kind == resume_step);
       linux_resume_one_lwp (lwp, step, lwp->resume->sig, NULL);
-      get_lwp_thread (lwp)->last_status.kind = TARGET_WAITKIND_IGNORE;
     }
   else
     {
@@ -2663,6 +2660,7 @@ linux_resume_one_thread (struct inferior
 	}
     }
 
+  thread->last_status.kind = TARGET_WAITKIND_IGNORE;
   lwp->resume = NULL;
   return 0;
 }
Index: src/gdb/gdbserver/linux-x86-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-x86-low.c	2010-04-03 22:18:33.000000000 +0100
+++ src/gdb/gdbserver/linux-x86-low.c	2010-04-03 22:31:39.000000000 +0100
@@ -325,7 +325,7 @@ x86_breakpoint_at (CORE_ADDR pc)
 {
   unsigned char c;
 
-  read_inferior_memory (pc, &c, 1);
+  (*the_target->read_memory) (pc, &c, 1);
   if (c == 0xCC)
     return 1;
 


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