From patchwork Tue Jan 5 13:04:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 10224 Received: (qmail 22919 invoked by alias); 5 Jan 2016 13:04:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 22909 invoked by uid 89); 5 Jan 2016 13:04:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=no version=3.3.2 spammy=20152016, substitute, merges, versa X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 05 Jan 2016 13:04:52 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 4EC0FA3D2D for ; Tue, 5 Jan 2016 13:04:51 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u05D4nPO001378 for ; Tue, 5 Jan 2016 08:04:50 -0500 Message-ID: <568BBF71.5030806@redhat.com> Date: Tue, 05 Jan 2016 13:04:49 +0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: gdb-patches@sourceware.org Subject: Re: [PATCH] Fix PR19388: Can't access $_siginfo in breakpoint (catch signal) condition References: <1450717854-28543-1-git-send-email-palves@redhat.com> In-Reply-To: <1450717854-28543-1-git-send-email-palves@redhat.com> > gdb/testsuite/ChangeLog: > 2015-12-21 Pedro Alves > > PR breakpoints/19388 > * gdb.base/catch-signal-siginfo-cond.c: New file. I just now noticed that I had forgotten to git add this file... Here's the same patch now with that file included (and copyright year updated). ------------- From: Pedro Alves Subject: [PATCH] Fix PR19388: Can't access $_siginfo in breakpoint (catch signal) condition Accessing $_siginfo from a "catch signal" breakpoint condition doesn't work. The condition always fails with "Selected thread is running": (gdb) catch signal Catchpoint 3 (standard signals) (gdb) condition $bpnum $_siginfo.si_signo == 5 (gdb) continue Continuing. Error in testing breakpoint condition: Selected thread is running. Catchpoint 3 (signal SIGUSR1), 0x0000003615e35877 in __GI_raise (sig=10) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 56 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig); (gdb) When accessing the $_siginfo object, we check whether the thread is marked running (external/public) state and refuse the access if so. This is so "print $_siginfo" at the prompt fails nicelly when the current thread is running. While evaluating breakpoint conditionals, we haven't decided yet whether the thread is going to stop, so is_running still returns true, and we thus always error out. Evaluating an expression that requires registers access is really conceptually the same -- we could think of $_siginfo as a pseudo register. However, in that case we check whether the thread is marked executing (internal/private state), not running (external/public state). Changing the $_siginfo validation to check is_executing as well fixes the bug in question. So this commit merges both the registers and $_siginfo checks into a single function. Note that checking is_executing is not fully correct, not even for registers. See PR 19389. However, I think this is the lesser of two evils and ends up as an improvement. We at least now have a single place to fix. Tested on x86_64 GNU/Linux. gdb/ChangeLog: 2016-01-05 Pedro Alves PR breakpoints/19388 * frame.c (get_current_frame): Use validate_registers_access. * gdbthread.h (validate_registers_access): Declare. * infrun.c (validate_siginfo_access): Delete. (siginfo_value_read, siginfo_value_write): Use validate_registers_access. * thread.c (validate_registers_access): New function. gdb/testsuite/ChangeLog: 2016-01-05 Pedro Alves PR breakpoints/19388 * gdb.base/catch-signal-siginfo-cond.c: New file. * gdb.base/catch-signal-siginfo-cond.exp: New file. --- gdb/frame.c | 9 +--- gdb/gdbthread.h | 5 ++ gdb/infrun.c | 27 +++------- gdb/testsuite/gdb.base/catch-signal-siginfo-cond.c | 33 ++++++++++++ .../gdb.base/catch-signal-siginfo-cond.exp | 61 ++++++++++++++++++++++ gdb/thread.c | 22 ++++++++ 6 files changed, 128 insertions(+), 29 deletions(-) create mode 100644 gdb/testsuite/gdb.base/catch-signal-siginfo-cond.c create mode 100644 gdb/testsuite/gdb.base/catch-signal-siginfo-cond.exp diff --git a/gdb/frame.c b/gdb/frame.c index 00a5ebc..48c9b33 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1473,14 +1473,7 @@ get_current_frame (void) error (_("No memory.")); /* Traceframes are effectively a substitute for the live inferior. */ if (get_traceframe_number () < 0) - { - if (ptid_equal (inferior_ptid, null_ptid)) - error (_("No selected thread.")); - if (is_exited (inferior_ptid)) - error (_("Invalid selected thread.")); - if (is_executing (inferior_ptid)) - error (_("Target is executing.")); - } + validate_registers_access (); if (current_frame == NULL) { diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index b75184b..718817b 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -555,6 +555,11 @@ extern int thread_is_in_step_over_chain (struct thread_info *tp); extern void thread_cancel_execution_command (struct thread_info *thr); +/* Check if it makes sense to access a register of current thread at + this point. If not, throw an error (e.g., the thread is + executing). */ +extern void validate_registers_access (void); + extern struct thread_info *thread_list; #endif /* GDBTHREAD_H */ diff --git a/gdb/infrun.c b/gdb/infrun.c index 0c61d26..f92643f 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -8684,25 +8684,6 @@ signals_info (char *signum_exp, int from_tty) "to change these tables.\n")); } -/* Check if it makes sense to read $_siginfo from the current thread - at this point. If not, throw an error. */ - -static void -validate_siginfo_access (void) -{ - /* No current inferior, no siginfo. */ - if (ptid_equal (inferior_ptid, null_ptid)) - error (_("No thread selected.")); - - /* Don't try to read from a dead thread. */ - if (is_exited (inferior_ptid)) - error (_("The current thread has terminated")); - - /* ... or from a spinning thread. */ - if (is_running (inferior_ptid)) - error (_("Selected thread is running.")); -} - /* The $_siginfo convenience variable is a bit special. We don't know for sure the type of the value until we actually have a chance to fetch the data. The type can change depending on gdbarch, so it is @@ -8721,7 +8702,9 @@ siginfo_value_read (struct value *v) { LONGEST transferred; - validate_siginfo_access (); + /* If we can access registers, so can we access $_siginfo. Likewise + vice versa. */ + validate_registers_access (); transferred = target_read (¤t_target, TARGET_OBJECT_SIGNAL_INFO, @@ -8742,7 +8725,9 @@ siginfo_value_write (struct value *v, struct value *fromval) { LONGEST transferred; - validate_siginfo_access (); + /* If we can access registers, so can we access $_siginfo. Likewise + vice versa. */ + validate_registers_access (); transferred = target_write (¤t_target, TARGET_OBJECT_SIGNAL_INFO, diff --git a/gdb/testsuite/gdb.base/catch-signal-siginfo-cond.c b/gdb/testsuite/gdb.base/catch-signal-siginfo-cond.c new file mode 100644 index 0000000..3a9ee3d --- /dev/null +++ b/gdb/testsuite/gdb.base/catch-signal-siginfo-cond.c @@ -0,0 +1,33 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2015-2016 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 . */ + +#include +#include + +void +handle (int sig) +{ +} + +int +main (void) +{ + signal (SIGUSR1, handle); + raise (SIGUSR1); + raise (SIGUSR1); /* set breakpoint here */ + return 0; +} diff --git a/gdb/testsuite/gdb.base/catch-signal-siginfo-cond.exp b/gdb/testsuite/gdb.base/catch-signal-siginfo-cond.exp new file mode 100644 index 0000000..7fabfd6 --- /dev/null +++ b/gdb/testsuite/gdb.base/catch-signal-siginfo-cond.exp @@ -0,0 +1,61 @@ +# Copyright 2015-2016 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 . + +# Regression test for PR19388. Make sure we can access $_siginfo in a +# "catch signal" condition. A buggy GDB would fail with: +# +# (gdb) continue +# Continuing. +# Error in testing breakpoint condition: +# Selected thread is running. +# +# Catchpoint 3 (signal SIGUSR1), 0x0000003615e35877 in __GI_raise (sig=10) at raise.c:56 +# 56 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig); +# (gdb) FAIL: gdb.base/catch-signal-siginfo-cond.exp: continue + +if [target_info exists gdb,nosignals] { + verbose "Skipping catch-signal-siginfo-cond.exp because of nosignals." + return -1 +} + +if { ![supports_get_siginfo_type] } { + verbose "Skipping catch-signal-siginfo-cond.exp because of lack of support." + return -1 +} + +standard_testfile + +if { [prepare_for_testing "failed to prepare" "${testfile}" $srcfile {debug}] } { + return -1 +} + +if ![runto_main] then { + fail "Can't run to main" + return -1 +} + +gdb_breakpoint [gdb_get_line_number "set breakpoint here"] + +gdb_test "catch signal" + +# It's OK to refer to standard Unix signal numbers 1-15 by number. + +# SIGUSR1 == 10. The catchpoint should not cause a stop. +gdb_test "condition \$bpnum \$_siginfo.si_signo == 11" +gdb_test "continue" "set breakpoint here.*" "continue: catchpoint does not trigger" + +# This time the catchpoint should cause a stop. +gdb_test "condition \$bpnum \$_siginfo.si_signo == 10" +gdb_test "continue" "Catchpoint .*signal SIGUSR1.*" "continue: catchpoint triggers" diff --git a/gdb/thread.c b/gdb/thread.c index 4c2259f..56526e4 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1098,6 +1098,28 @@ finish_thread_state_cleanup (void *arg) finish_thread_state (*ptid_p); } +/* See gdbthread.h. */ + +void +validate_registers_access (void) +{ + /* No selected thread, no registers. */ + if (ptid_equal (inferior_ptid, null_ptid)) + error (_("No thread selected.")); + + /* Don't try to read from a dead thread. */ + if (is_exited (inferior_ptid)) + error (_("The current thread has terminated")); + + /* ... or from a spinning thread. FIXME: This isn't actually fully + correct. It'll allow an user-requested access (e.g., "print $pc" + at the prompt) when a thread is not executing for some internal + reason, but is marked running from the user's perspective. E.g., + the thread is waiting for its turn in the step-over queue. */ + if (is_executing (inferior_ptid)) + error (_("Selected thread is running.")); +} + int pc_in_thread_step_range (CORE_ADDR pc, struct thread_info *thread) {