[patch] Fix linux-ia64 on SIGILL for deleted breakpoint

Jan Kratochvil jan.kratochvil@redhat.com
Sun Jul 25 18:52:00 GMT 2010


On Sun, 25 Jul 2010 00:26:04 +0200, Pedro Alves wrote:
> On Friday 23 July 2010 23:19:35, Jan Kratochvil wrote:
> 
> > This SIGTRAP->SIGILL case happens only on ia64 and ia64 does not use any
> > set_gdbarch_decr_pc_after_break at all, PC stays on the breakpoint bundle+slot
> > in both the SIGTRAP and SIGILL case.
> > 
> > You are right it is arch-specific.  On i386 I checked SIGILL is never
> > generated (only in some fpu-emulated code).  So I checked s390x-linux-gnu::
> > 	SIGILL on opcode 0xb29e
> > 	si_addr = 0x800009a4
> > 	.psw.addr = 0x800009a8
> > 	instr at = 0x800009a4
> > 	.psw.addr - instr == 4
> 
> Oh?  Does this mean the PC is left pointing _after_ the instruction
> that caused the SIGILL?

Yes.  It probably corresponds to:

390 - z/Architecture Principles of Operation (assembly manual)
http://publibz.boulder.ibm.com/epubs/pdf/a2278325.pdf
6-6 Instruction-Length Code
The instruction-length code (ILC) occupies two bit positions and provides the 
length of the last instruction executed.  It permits identifying the
instruction causing the interruption when the instruction address in the old
PSW designates the next sequential instruction.
                   ^^^^

set_gdbarch_decr_pc_after_break = 2 is still OK there:

gdbarch_decr_pc_after_break:
This function shall return the amount by which to decrement the PC after the
program encounters a breakpoint.
                   ^^^^^^^^^^^^

As I see it means GDB-placed-breakpoint.  When GDB hits x86_64 random "int3"
it does not decrement PC (which may be for a discussion if it should but
currently it does not by design).  GDB does not intentionally place SIGILL
generating instruction on s390x so it does not need to recognize ILC.


> I think you still need to audit other bits in linux-nat.c for SIGTRAP bkpts
> handling.  E.g., see stop_wait_callback.  (For extra correctness,
> count_events_callback, and the select_event_lwp_callback functions would
> be relaxed too.)  I wasn't previously suggesting to make this ia64 arch
> specific, which made fixing these other places too easier.  Notice how
> gdbserver/linux-low.c also considers non-sigtrap bkpts (and it was a recent
> change, needed for ARM thumb2 kernels that hadn't learned about the
> breakpoint insns yet, IIRC).

Are we talking about?
	RFC: Updates support for breakpoints that generate SIGILL
	http://sourceware.org/ml/gdb-patches/2010-01/msg00619.html

As it is for arm*.c which does not use set_gdbarch_decr_pc_after_break I do
not want to predict how SIGILL vs. gdbarch_decr_pc_after_break behaves.

Which arch uses gdbarch_decr_pc_after_break && generates a non-SIGTRAP signal
on a GDB-placed breakpoint?  It is probably none of the arches I target (i686,
x86_64, ppc{,64}, s390{,x}, ia64, possibly arm).


> If this stays, please add a comment above this call mentioning that
> we can safely call this function even for SIGILL, since
> decr_pc_after_break is 0 on ia64.

Done.


> (Alternatively, recode to avoid the assumption)

Sorry I do not try to code anything without an iron to test it.


> The point I'm making is that PC adjustment is always only necessary for
> sigtraps, never other signals (and is in fact wrong for other signals).

I believe you mean the adjustment is necessary for GDB-placed instructions
only.

No regressions on {x86_64,x86_64-m32,i686}-fedora13-linux-gnu.
Tested the new testcase on ia64-rhel55-linux-gnu.

OK to check-in?


Thanks,
Jan


gdb/
2010-07-25  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* ia64-linux-nat.c (ia64_linux_status_is_event): New function.
	(_initialize_ia64_linux_nat): Install it.
	* linux-nat.c (sigtrap_is_event, linux_nat_status_is_event)
	(linux_nat_set_status_is_event): New.
	(stop_wait_callback, count_events_callback, select_event_lwp_callback)
	cancel_breakpoints_callback, linux_nat_filter_event)
	(linux_nat_wait_1): Use linux_nat_status_is_event.
	* linux-nat.h (linux_nat_set_status_is_event): New prototype.

gdb/testsuite/
2010-07-25  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.threads/ia64-sigill.exp: New file.
	* gdb.threads/ia64-sigill.c: New file.

--- a/gdb/ia64-linux-nat.c
+++ b/gdb/ia64-linux-nat.c
@@ -809,6 +809,18 @@ ia64_linux_xfer_partial (struct target_ops *ops,
 			     offset, len);
 }
 
+/* For break.b instruction ia64 CPU forgets the immediate value and generates
+   SIGILL with ILL_ILLOPC instead of more common SIGTRAP with TRAP_BRKPT.
+   ia64 does not use gdbarch_decr_pc_after_break so we do not have to make any
+   difference for the signals here.  */
+
+static int
+ia64_linux_status_is_event (int status)
+{
+  return WIFSTOPPED (status) && (WSTOPSIG (status) == SIGTRAP
+				 || WSTOPSIG (status) == SIGILL);
+}
+
 void _initialize_ia64_linux_nat (void);
 
 void
@@ -848,4 +860,5 @@ _initialize_ia64_linux_nat (void)
   /* Register the target.  */
   linux_nat_add_target (t);
   linux_nat_set_new_thread (t, ia64_linux_new_thread);
+  linux_nat_set_status_is_event (t, ia64_linux_status_is_event);
 }
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2605,6 +2605,29 @@ linux_nat_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p)
   return lp->stopped_data_address_p;
 }
 
+/* Commonly any breakpoint / watchpoint generate only SIGTRAP.  */
+
+static int
+sigtrap_is_event (int status)
+{
+  return WIFSTOPPED (status) && WSTOPSIG (status) == SIGTRAP;
+}
+
+/* SIGTRAP-like events recognizer.  */
+
+static int (*linux_nat_status_is_event) (int status) = sigtrap_is_event;
+
+/* Set altarnative SIGTRAP-like events recognizer.  If
+   breakpoint_inserted_here_p there then gdbarch_decr_pc_after_break will be
+   applied.  */
+
+void
+linux_nat_set_status_is_event (struct target_ops *t,
+			       int (*status_is_event) (int status))
+{
+  linux_nat_status_is_event = status_is_event;
+}
+
 /* Wait until LP is stopped.  */
 
 static int
@@ -2645,7 +2668,7 @@ stop_wait_callback (struct lwp_info *lp, void *data)
 
       if (WSTOPSIG (status) != SIGSTOP)
 	{
-	  if (WSTOPSIG (status) == SIGTRAP)
+	  if (linux_nat_status_is_event (status))
 	    {
 	      /* If a LWP other than the LWP that we're reporting an
 	         event for has hit a GDB breakpoint (as opposed to
@@ -2801,7 +2824,7 @@ count_events_callback (struct lwp_info *lp, void *data)
 
   /* Count only resumed LWPs that have a SIGTRAP event pending.  */
   if (lp->status != 0 && lp->resumed
-      && WIFSTOPPED (lp->status) && WSTOPSIG (lp->status) == SIGTRAP)
+      && linux_nat_status_is_event (lp->status))
     (*count)++;
 
   return 0;
@@ -2829,7 +2852,7 @@ select_event_lwp_callback (struct lwp_info *lp, void *data)
 
   /* Select only resumed LWPs that have a SIGTRAP event pending. */
   if (lp->status != 0 && lp->resumed
-      && WIFSTOPPED (lp->status) && WSTOPSIG (lp->status) == SIGTRAP)
+      && linux_nat_status_is_event (lp->status))
     if ((*selector)-- == 0)
       return 1;
 
@@ -2891,7 +2914,7 @@ cancel_breakpoints_callback (struct lwp_info *lp, void *data)
 
   if (lp->waitstatus.kind == TARGET_WAITKIND_IGNORE
       && lp->status != 0
-      && WIFSTOPPED (lp->status) && WSTOPSIG (lp->status) == SIGTRAP
+      && linux_nat_status_is_event (lp->status)
       && cancel_breakpoint (lp))
     /* Throw away the SIGTRAP.  */
     lp->status = 0;
@@ -3064,7 +3087,7 @@ linux_nat_filter_event (int lwpid, int status, int options)
 	return NULL;
     }
 
-  if (WIFSTOPPED (status) && WSTOPSIG (status) == SIGTRAP)
+  if (linux_nat_status_is_event (status))
     {
       /* Save the trap's siginfo in case we need it later.  */
       save_siginfo (lp);
@@ -3411,7 +3434,7 @@ retry:
 			 threads.  */
 		      if (non_stop
 			  && lp->waitstatus.kind == TARGET_WAITKIND_IGNORE
-			  && WSTOPSIG (lp->status) == SIGTRAP
+			  && linux_nat_status_is_event (lp->status)
 			  && cancel_breakpoint (lp))
 			{
 			  /* Throw away the SIGTRAP.  */
@@ -3627,7 +3650,7 @@ retry:
   else
     lp->resumed = 0;
 
-  if (WIFSTOPPED (status) && WSTOPSIG (status) == SIGTRAP)
+  if (linux_nat_status_is_event (status))
     {
       if (debug_linux_nat)
 	fprintf_unfiltered (gdb_stdlog,
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -172,3 +172,7 @@ struct siginfo *linux_nat_get_siginfo (ptid_t ptid);
 
 /* Compute and return the processor core of a given thread.  */
 int linux_nat_core_of_thread_1 (ptid_t ptid);
+
+/* Set altarnative SIGTRAP-like events recognizer.  */
+void linux_nat_set_status_is_event (struct target_ops *t,
+				    int (*status_is_event) (int status));
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/ia64-sigill.c
@@ -0,0 +1,360 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.  */
+
+#define _GNU_SOURCE
+#include <pthread.h>
+#include <stdio.h>
+#include <limits.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+#include <assert.h>
+#include <sys/types.h>
+#include <signal.h>
+#include <unistd.h>
+#include <asm/unistd.h>
+
+#define gettid() syscall (__NR_gettid)
+
+/* Terminate always in the main task, it can lock up with SIGSTOPped GDB
+   otherwise.  */
+#define TIMEOUT (gettid () == getpid() ? 10 : 15)
+
+static pid_t thread1_tid;
+static pthread_cond_t thread1_tid_cond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t thread1_tid_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+
+static pid_t thread2_tid;
+static pthread_cond_t thread2_tid_cond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t thread2_tid_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+
+static pthread_mutex_t terminate_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+
+/* Do not use alarm as it would create a ptrace event which would hang up us if
+   we are being traced by GDB which we stopped ourselves.  */
+
+static void timed_mutex_lock (pthread_mutex_t *mutex)
+{
+  int i;
+  struct timespec start, now;
+
+  i = clock_gettime (CLOCK_MONOTONIC, &start);
+  assert (i == 0);
+
+  do
+    {
+      i = pthread_mutex_trylock (mutex);
+      if (i == 0)
+	return;
+      assert (i == EBUSY);
+
+      i = clock_gettime (CLOCK_MONOTONIC, &now);
+      assert (i == 0);
+      assert (now.tv_sec >= start.tv_sec);
+    }
+  while (now.tv_sec - start.tv_sec < TIMEOUT);
+
+  fprintf (stderr, "Timed out waiting for internal lock!\n");
+  exit (EXIT_FAILURE);
+}
+
+static void *
+thread_func (void *threadno_voidp)
+{
+  int threadno = (intptr_t) threadno_voidp;
+  int i;
+
+  switch (threadno)
+  {
+    case 1:
+      timed_mutex_lock (&thread1_tid_mutex);
+
+      /* THREAD1_TID_MUTEX must be already locked to avoid race.  */
+      thread1_tid = gettid ();
+
+      i = pthread_cond_signal (&thread1_tid_cond);
+      assert (i == 0);
+      i = pthread_mutex_unlock (&thread1_tid_mutex);
+      assert (i == 0);
+
+      break;
+
+    case 2:
+      timed_mutex_lock (&thread2_tid_mutex);
+
+      /* THREAD2_TID_MUTEX must be already locked to avoid race.  */
+      thread2_tid = gettid ();
+
+      i = pthread_cond_signal (&thread2_tid_cond);
+      assert (i == 0);
+      i = pthread_mutex_unlock (&thread2_tid_mutex);
+      assert (i == 0);
+
+      break;
+
+    default:
+      assert (0);
+  }
+
+#ifdef __ia64__
+  asm volatile ("label:\n"
+		"nop.m 0\n"
+		"nop.i 0\n"
+		"nop.b 0\n");
+#endif
+  /* break-here */
+
+  /* Be sure the "t (tracing stop)" test can proceed for both threads.  */
+  timed_mutex_lock (&terminate_mutex);
+  i = pthread_mutex_unlock (&terminate_mutex);
+  assert (i == 0);
+
+  return NULL;
+}
+
+static const char *
+proc_string (const char *filename, const char *line)
+{
+  FILE *f;
+  static char buf[LINE_MAX];
+  size_t line_len = strlen (line);
+
+  f = fopen (filename, "r");
+  if (f == NULL)
+    {
+      fprintf (stderr, "fopen (\"%s\") for \"%s\": %s\n", filename, line,
+	       strerror (errno));
+      exit (EXIT_FAILURE);
+    }
+  while (errno = 0, fgets (buf, sizeof (buf), f))
+    {
+      char *s;
+
+      s = strchr (buf, '\n');
+      assert (s != NULL);
+      *s = 0;
+
+      if (strncmp (buf, line, line_len) != 0)
+	continue;
+
+      if (fclose (f))
+	{
+	  fprintf (stderr, "fclose (\"%s\") for \"%s\": %s\n", filename, line,
+		   strerror (errno));
+	  exit (EXIT_FAILURE);
+	}
+
+      return &buf[line_len];
+    }
+  if (errno != 0)
+    {
+      fprintf (stderr, "fgets (\"%s\": %s\n", filename, strerror (errno));
+      exit (EXIT_FAILURE);
+    }
+  fprintf (stderr, "\"%s\": No line \"%s\" found.\n", filename, line);
+  exit (EXIT_FAILURE);
+}
+
+static unsigned long
+proc_ulong (const char *filename, const char *line)
+{
+  const char *s = proc_string (filename, line);
+  long retval;
+  char *end;
+
+  errno = 0;
+  retval = strtol (s, &end, 10);
+  if (retval < 0 || retval >= LONG_MAX || (end && *end))
+    {
+      fprintf (stderr, "\"%s\":\"%s\": %ld, %s\n", filename, line, retval,
+	       strerror (errno));
+      exit (EXIT_FAILURE);
+    }
+  return retval;
+}
+
+static void
+state_wait (pid_t process, const char *wanted)
+{
+  char *filename;
+  int i;
+  struct timespec start, now;
+  const char *state;
+
+  i = asprintf (&filename, "/proc/%lu/status", (unsigned long) process);
+  assert (i > 0);
+
+  i = clock_gettime (CLOCK_MONOTONIC, &start);
+  assert (i == 0);
+
+  do
+    {
+      state = proc_string (filename, "State:\t");
+
+      /* torvalds/linux-2.6.git 464763cf1c6df632dccc8f2f4c7e50163154a2c0
+	 has changed "T (tracing stop)" to "t (tracing stop)".  Make the GDB
+	 testcase backward compatible with older Linux kernels.  */
+      if (strcmp (state, "T (tracing stop)") == 0)
+	state = "t (tracing stop)";
+
+      if (strcmp (state, wanted) == 0)
+	{
+	  free (filename);
+	  return;
+	}
+
+      if (sched_yield ())
+	{
+	  perror ("sched_yield()");
+	  exit (EXIT_FAILURE);
+	}
+
+      i = clock_gettime (CLOCK_MONOTONIC, &now);
+      assert (i == 0);
+      assert (now.tv_sec >= start.tv_sec);
+    }
+  while (now.tv_sec - start.tv_sec < TIMEOUT);
+
+  fprintf (stderr, "Timed out waiting for PID %lu \"%s\" (now it is \"%s\")!\n",
+	   (unsigned long) process, wanted, state);
+  exit (EXIT_FAILURE);
+}
+
+static volatile pid_t tracer = 0;
+static pthread_t thread1, thread2;
+
+static void
+cleanup (void)
+{
+  printf ("Resuming GDB PID %lu.\n", (unsigned long) tracer);
+
+  if (tracer)
+    {
+      int i;
+      int tracer_save = tracer;
+
+      tracer = 0;
+
+      i = kill (tracer_save, SIGCONT);
+      assert (i == 0);
+    }
+}
+
+int
+main (int argc, char **argv)
+{
+  int i;
+  int standalone = 0;
+
+  if (argc == 2 && strcmp (argv[1], "-s") == 0)
+    standalone = 1;
+  else
+    assert (argc == 1);
+
+  setbuf (stdout, NULL);
+
+  timed_mutex_lock (&thread1_tid_mutex);
+  timed_mutex_lock (&thread2_tid_mutex);
+
+  timed_mutex_lock (&terminate_mutex);
+
+  i = pthread_create (&thread1, NULL, thread_func, (void *) (intptr_t) 1);
+  assert (i == 0);
+
+  i = pthread_create (&thread2, NULL, thread_func, (void *) (intptr_t) 2);
+  assert (i == 0);
+
+  if (!standalone)
+    {
+      tracer = proc_ulong ("/proc/self/status", "TracerPid:\t");
+      if (tracer == 0)
+	{
+	  fprintf (stderr, "The testcase must be run by GDB!\n");
+	  exit (EXIT_FAILURE);
+	}
+      if (tracer != getppid ())
+	{
+	  fprintf (stderr, "The testcase parent must be our GDB tracer!\n");
+	  exit (EXIT_FAILURE);
+	}
+    }
+
+  /* SIGCONT our debugger in the case of our crash as we would deadlock
+     otherwise.  */
+
+  atexit (cleanup);
+
+  printf ("Stopping GDB PID %lu.\n", (unsigned long) tracer);
+
+  if (tracer)
+    {
+      i = kill (tracer, SIGSTOP);
+      assert (i == 0);
+      state_wait (tracer, "T (stopped)");
+    }
+
+  /* Threads are now waiting at timed_mutex_lock (thread1_tid_mutex) and so
+     they could not trigger the breakpoint before GDB gets unstopped later.
+     Threads get resumed at pthread_cond_wait below.  Use `while' loops for
+     protection against spurious pthread_cond_wait wakeups.  */
+
+  printf ("Waiting till the threads initialize their TIDs.\n");
+
+  while (thread1_tid == 0)
+    {
+      i = pthread_cond_wait (&thread1_tid_cond, &thread1_tid_mutex);
+      assert (i == 0);
+    }
+
+  while (thread2_tid == 0)
+    {
+      i = pthread_cond_wait (&thread2_tid_cond, &thread2_tid_mutex);
+      assert (i == 0);
+    }
+
+  printf ("Thread 1 TID = %lu, thread 2 TID = %lu, PID = %lu.\n",
+	  (unsigned long) thread1_tid, (unsigned long) thread2_tid,
+	  (unsigned long) getpid ());
+
+  printf ("Waiting till the threads get trapped by the breakpoint.\n");
+
+  if (tracer)
+    {
+      /* s390x-unknown-linux-gnu will fail with "R (running)".  */
+
+      state_wait (thread1_tid, "t (tracing stop)");
+
+      state_wait (thread2_tid, "t (tracing stop)");
+    }
+
+  cleanup ();
+
+  printf ("Joining the threads.\n");
+
+  i = pthread_mutex_unlock (&terminate_mutex);
+  assert (i == 0);
+
+  i = pthread_join (thread1, NULL);
+  assert (i == 0);
+
+  i = pthread_join (thread2, NULL);
+  assert (i == 0);
+
+  printf ("Exiting.\n");	/* break-at-exit */
+
+  return EXIT_SUCCESS;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/ia64-sigill.exp
@@ -0,0 +1,76 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2010 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 SIGILL generated by some special cases of breakpoints on ia64.  Problem
+# was SIGILL being stored in non-current thread for later retrieval when its
+# breakpoint has been already deleted.  moribund locations are not active in
+# the default all-stop mode.
+
+set testfile "ia64-sigill"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" ${binfile} executable [list debug additional_flags=-lrt]] != "" } {
+    return -1
+}
+
+clean_restart $testfile
+
+if ![runto_main] {
+    return -1
+}
+
+set test "info addr label"
+gdb_test_multiple $test $test {
+    -re "Symbol \"label\" is at 0x\[0-9a-f\]+0 in .*\r\n$gdb_prompt $" {
+	# Verify the label really starts at the start of ia64 bundle.
+	pass $test
+
+	# ia64 generates SIGILL for breakpoint at B slot of an MIB bundle.
+	gdb_test "break *label+2" {Breakpoint [0-9]+ at 0x[0-9a-f]+2:.*}
+    }
+    -re "No symbol \"label\" in current context\\.\r\n$gdb_prompt $" {
+	pass $test
+
+	# Either this target never generates non-SIGTRAP signals or they do
+	# not depend on the breakpoint address.  Try any address.
+	gdb_breakpoint [gdb_get_line_number "break-here"]
+    }
+}
+
+gdb_test_no_output {set $sigill_bpnum=$bpnum}
+
+gdb_breakpoint [gdb_get_line_number "break-at-exit"]
+
+gdb_test_no_output "set debug infrun 1"
+
+# The ia64 SIGILL signal is visible only in the lin-lwp debug.
+gdb_test_no_output "set debug lin-lwp 1"
+
+gdb_test "continue" "Breakpoint \[0-9\]+,( .* in)? thread_func .*"
+
+gdb_test_no_output {delete $sigill_bpnum}
+
+set test "continue for the pending signal"
+gdb_test_multiple "continue" $test {
+    -re "Breakpoint \[0-9\]+, .*break-at-exit.*\r\n$gdb_prompt $" {
+	# Breakpoint has been skipped in the other thread.
+	pass $test
+    }
+    -re "Program received signal .*\r\n$gdb_prompt $" {
+	fail $test
+    }
+}



More information about the Gdb-patches mailing list