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

Jan Kratochvil
2010-07-27T20:55:23Z

On Tue, 27 Jul 2010 13:43:16 +0200, Pedro Alves wrote:
> On Sunday 25 July 2010 19:52:17, Jan Kratochvil wrote:
> > On Sun, 25 Jul 2010 00:26:04 +0200, Pedro Alves wrote:

> If your check on s390 is good, than that might have broken (rare ...) corner
> cases on s390, like a SIGILL reported for an instruction set at the address
> consecutive to an inserted software breakpoint.

gdb/gdbserver/linux-low.c maybe_internal_trap may get set wrong on s390, OK.

> > 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).
>
> I don't have a list. Some archs may generate that only on some
> situations (or used to and no longer do on more up to date versions).
kernel

> > > (Alternatively, recode to avoid the assumption)
> >
> > Sorry I do not try to code anything without an iron to test it.
>
> What I meant was to have your new code not depend on decr_pc_after
> break being 0, you don't need any other system to try that on.
> It meant, to not have any code path in the SIGILL
> case that calls gdbarch_decr_pc_after_break at all. Conceptually,
> something like:
>
> if (sig == SIGTRAP && software_breakpoint_inserted_at (PC - decr_pc_after_breakpoint))
> {
> rewind pc, discard signal
> return 1
> }
> else if (sig == SIGILL && software_breakpoint_inserted_at (PC))
> {
> discard signal
> return 1
> }
> return 0

I believe we could need some ` - decr_pc_after_sigill' in the second
conditional as shown on the s390 case. But only if GDB would place any such
SIGILL instruction so this paragraph is just a hand waving.

Not implementing this alternative way, thanks for the analysis.

> Okay. Please fix the spelling of "alternative".

I am sorry. Checked-in.

Thanks,
Jan

http://sourceware.org/ml/gdb-cvs/2010-07/msg00170.html

--- src/gdb/ChangeLog 2010/07/27 20:44:30 1.12025
+++ src/gdb/ChangeLog 2010/07/27 20:51:37 1.12026
@@ -1,3 +1,14 @@
+2010-07-27 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.
+
2010-07-27 Tom Tromey <tromey@redhat.com>

* NEWS: Mention labels, .gdb_index.
--- src/gdb/ia64-linux-nat.c 2010/07/07 16:15:16 1.49
+++ src/gdb/ia64-linux-nat.c 2010/07/27 20:51:37 1.50
@@ -809,6 +809,18 @@
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 @@
/* 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);
}
--- src/gdb/linux-nat.c 2010/07/25 09:31:12 1.178
+++ src/gdb/linux-nat.c 2010/07/27 20:51:37 1.179
@@ -2605,6 +2605,29 @@
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 alternative 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 @@

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 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 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 @@

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 @@
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 @@
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 @@
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,
--- src/gdb/linux-nat.h 2010/06/11 12:10:12 1.35
+++ src/gdb/linux-nat.h 2010/07/27 20:51:37 1.36
@@ -172,3 +172,7 @@

/* Compute and return the processor core of a given thread. */
int linux_nat_core_of_thread_1 (ptid_t ptid);
+
+/* Set alternative SIGTRAP-like events recognizer. */
+void linux_nat_set_status_is_event (struct target_ops *t,
+ int (*status_is_event) (int status));
--- src/gdb/testsuite/ChangeLog 2010/07/27 18:08:47 1.2391
+++ src/gdb/testsuite/ChangeLog 2010/07/27 20:51:37 1.2392
@@ -1,3 +1,8 @@
+2010-07-27 Jan Kratochvil <jan.kratochvil@redhat.com>
+
+ * gdb.threads/ia64-sigill.exp: New file.
+ * gdb.threads/ia64-sigill.c: New file.
+
2010-07-27 Tom Tromey <tromey@redhat.com>

* gdb.opt/inline-cmds.c (ATTR): New define.
--- src/gdb/testsuite/gdb.threads/ia64-sigill.c
+++ src/gdb/testsuite/gdb.threads/ia64-sigill.c 2010-07-27 20:51:55.245809000 +0000
@@ -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;
+}
--- src/gdb/testsuite/gdb.threads/ia64-sigill.exp
+++ src/gdb/testsuite/gdb.threads/ia64-sigill.exp 2010-07-27 20:51:55.778901000 +0000
@@ -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
+ }
+}