From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23997 invoked by alias); 27 May 2009 22:00:19 -0000 Received: (qmail 23962 invoked by uid 22791); 27 May 2009 22:00:15 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,SPF_PASS,ZMIde_GENERICSPAM1 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 27 May 2009 22:00:02 +0000 Received: (qmail 20473 invoked from network); 27 May 2009 22:00:00 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 27 May 2009 22:00:00 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Fixes for a couple of infrun bugs (thread hop, revert to step thread). Date: Wed, 27 May 2009 22:00:00 -0000 User-Agent: KMail/1.9.10 Cc: Ulrich Weigand MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200905272300.28249.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-05/txt/msg00594.txt.bz2 [ Hi Ulrich, I'm CCing you because this touches code you've touched recently. ] Given a testcase like this: void * thread_function (void *arg) { pthread_exit (NULL); } void hoop_me (void) {} int main (int argc, char **argv) { pthread_t thread; pthread_create (&thread, NULL, thread_function, NULL); pthread_join (thread, NULL); hop_me (); /* <<< set thread "thread_function" specific breakpoint here */ } Say you're stopped inside thread_function (in thread 2), and set a breakpoint at the "hop_me" call, and make it trigger only on thread 2 (like so: "break hoopme thread 2"). Then, step over thread 2's exit. GDB is expected to be able to make the main thread hop over the thread specific breakpoint, but, it gets in trouble. E.g., 25 pthread_exit (NULL); (gdb) b 37 thread 2 Breakpoint 2 at 0x400653: file ../../../src/gdb/testsuite/gdb.threads/thread-exits.c, line 37. (gdb) n [Thread 0x40800950 (LWP 27593) exited] warning: Error removing breakpoint 0 warning: Error removing breakpoint 0 warning: Error removing breakpoint 0 Cannot step over breakpoint hit in wrong thread (gdb) The problem here is that we tried to remove memory breakpoints with inferior_ptid still pointing at the thread that had just exited. We're already switching context to the event thread a bit after removing breakpoints, so the fix here is just to do that *before* removing breakpoints. This is this hunk in the patch at the bottom: @@ -2896,6 +2897,11 @@ targets should add new threads to the th if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: thread_hop_needed\n"); + /* Switch context before touching inferior memory, the + previous thread may have exited. */ + if (!ptid_equal (inferior_ptid, ecs->ptid)) + context_switch (ecs->ptid); + /* Saw a breakpoint, but it was hit by the wrong thread. Just continue. */ @@ -2922,9 +2928,6 @@ targets should add new threads to the th error (_("Cannot step over breakpoint hit in wrong thread")); else { /* Single step */ - if (!ptid_equal (inferior_ptid, ecs->ptid)) - context_switch (ecs->ptid); - if (!non_stop) { /* Only need to require the next event from this The new threxit-hop-specific.exp test exercises this. (( As I've been saying, we're incrementally reaching a state where we'll always be able to switch inferior_ptid early and be done with it... In this particular case, I *think* we could move it yet a bit higher up, to the beginning of the thread hop handling without much trouble, but, there's a bit of software-single-stepping related thread hoping code that makes me want to do that as a separate change. Note that if we switch context before thread hoping, in practice means that we move the thread hoping to after the all mighty line: /* See if something interesting happened to the non-current thread. If so, then switch to that thread. */ )) Ok, on to the next issue, and actually, the original issue that made be see the above. This triggers with stepping over an execl call, in a threaded app, but the thread exit example is simpler. The test for this at the end exercises the execl variant. Let's try the same pthread_exit example again, but now with the remove-breakpoints bug fixed. We'll add a little twist. The code that should be thread-hopped stays in a loop, something like so: void * thread_function (void *arg) { pthread_exit (NULL); } void hoop_me (void) {} int main (int argc, char **argv) { pthread_t thread; pthread_create (&thread, NULL, thread_function, NULL); pthread_join (thread, NULL); while (1) + hop_me (); /* <<< set thread "thread_function" specific breakpoint here */ } Here's what one sees (line number don't match, but should show the example OK): 0x0) at ../../../src/gdb/testsuite/gdb.threads/threxit-hop-specific.c:26 26 pthread_exit (NULL); (gdb) n During symbol reading, incomplete CFI data; unspecified registers (e.g., rax) at 0x4005cf. [Thread 0x40800950 (LWP 7559) exited] Couldn't get registers: No such process. (gdb) Again, but with "set debug infrun 1": infrun: switching back to stepped thread infrun: Switching context from Thread 0x7ffff7fd66e0 (LWP 7728) to Thread 0x40800950 (LWP 7729) Couldn't get registers: No such process. (gdb) LWP 7729 here was thread 2, which exited. The error happens because we're trying to resume it, and in the process tried to read registers from it. The code in question that tried to resume was this not-so-old bit of handle_inferior_event: /* In all-stop mode, if we're currently stepping but have stopped in some other thread, we need to switch back to the stepped thread. */ if (!non_stop) { struct thread_info *tp; tp = iterate_over_threads (currently_stepping_callback, ecs->event_thread); if (tp) { ... keep_going (ecs); <<< return; } currently_stepping_callback will not return true for threads that have a step-resume breakpoint (the thread was doing a next over a pthread_exit call, so there should be a step-resume), but, this was tested on native linux, where we have thread exit events, and, deleting a thread clears its step-resume breakpoint. But, if the thread was deleted, how can iterate_over_threads find it? Ah, at the time it exited, inferior_ptid pointed at it, and, we don't delete the thread in that case, but instead leave it listed with state set to "exited". Now, before suggesting that we should clear enough stepping state of exited threads so that currently_stepping_callback doesn't find those, keep in mind targets where we do *not* have thread exit events. In that case, we'll have to ask the target if the thread is still alive. But, wait, if there are no thread exit events, then the original thread should still have a step-resume breakpoint? Yes, but we could be single-stepping until the thread exit. Plus, I think we *do* want to switch back the focus here to the thread that was nexting. If we try that against gdbserver, where we don't have thread exit notifications (there's no such thing in the remote protocol), GDB doesn't throw any error, but that's just by luck (bug). Looking closer, we see this: infrun: stop_pc = 0x400658 infrun: switching back to stepped thread infrun: Switching context from Thread 32013 to Thread 32105 Sending packet: $Hg7d69#b9...Packet received: E01 Sending packet: $g#67...Packet received: 0000000000000000[... ... ...] So, the remote target refuses (E01) to select the thread (Hg) that is gone already, but GDB silently ignores it, and ends up reading registers from the wrong thread... We should take advantage of those errors to realize the thread is gone, but that's for another day. All this brings me to the patch below, and two new tests, that should cover these bugs. Does it look ok? Tested against native x86_64-linux and against local gdbserver. -- Pedro Alves gdb/ 2009-05-27 Pedro Alves * infrun.c (handle_inferior_event): When thread hoping, switch inferior_ptid to the event thread before removing breakpoints from the target. If not stopping, also try to revert back to a thread that was doing a "next". Check if that thread still exists before resuming. (currently_stepping_thread): Delete and merge with ... (currently_stepping): ... this. (currently_stepping_callback): Rename to ... (currently_stepping_or_nexting_callback): ... this, and also return true if the thread was stepping over a call (has a step-resume breakpoint). gdb/testsuite/ 2009-05-27 Pedro Alves * gdb.threads/threxit-hop-specific.c: New. * gdb.threads/threxit-hop-specific.exp: New. * gdb.threads/thread-execl.c: New. * gdb.threads/thread-execl.exp: New. --- gdb/infrun.c | 57 +++++++++++++------- gdb/testsuite/gdb.threads/thread-execl.c | 48 +++++++++++++++++ gdb/testsuite/gdb.threads/thread-execl.exp | 52 ++++++++++++++++++ gdb/testsuite/gdb.threads/threxit-hop-specific.c | 48 +++++++++++++++++ gdb/testsuite/gdb.threads/threxit-hop-specific.exp | 59 +++++++++++++++++++++ 5 files changed, 245 insertions(+), 19 deletions(-) Index: src/gdb/infrun.c =================================================================== --- src.orig/gdb/infrun.c 2009-05-27 22:00:21.000000000 +0100 +++ src/gdb/infrun.c 2009-05-27 22:44:18.000000000 +0100 @@ -75,7 +75,8 @@ static void set_schedlock_func (char *ar static int currently_stepping (struct thread_info *tp); -static int currently_stepping_callback (struct thread_info *tp, void *data); +static int currently_stepping_or_nexting_callback (struct thread_info *tp, + void *data); static void xdb_handle_command (char *args, int from_tty); @@ -2896,6 +2897,11 @@ targets should add new threads to the th if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: thread_hop_needed\n"); + /* Switch context before touching inferior memory, the + previous thread may have exited. */ + if (!ptid_equal (inferior_ptid, ecs->ptid)) + context_switch (ecs->ptid); + /* Saw a breakpoint, but it was hit by the wrong thread. Just continue. */ @@ -2922,9 +2928,6 @@ targets should add new threads to the th error (_("Cannot step over breakpoint hit in wrong thread")); else { /* Single step */ - if (!ptid_equal (inferior_ptid, ecs->ptid)) - context_switch (ecs->ptid); - if (!non_stop) { /* Only need to require the next event from this @@ -3483,7 +3486,7 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME ( if (!non_stop) { struct thread_info *tp; - tp = iterate_over_threads (currently_stepping_callback, + tp = iterate_over_threads (currently_stepping_or_nexting_callback, ecs->event_thread); if (tp) { @@ -3498,6 +3501,21 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME ( return; } + /* If the stepping thread exited, then don't try reverting + back to it, just keep going. We need to query the target + in case it doesn't support thread exit events. */ + if (is_exited (tp->ptid) + || !target_thread_alive (tp->ptid)) + { + if (debug_infrun) + fprintf_unfiltered (gdb_stdlog, "\ +infrun: not switching back to stepped thread, it has vanished\n"); + + delete_thread (tp->ptid); + keep_going (ecs); + return; + } + /* Otherwise, we no longer expect a trap in the current thread. Clear the trap_expected flag before switching back -- this is what keep_going would do as well, if we called it. */ @@ -3931,28 +3949,29 @@ infrun: BPSTAT_WHAT_SET_LONGJMP_RESUME ( keep_going (ecs); } -/* Are we in the middle of stepping? */ +/* Is thread TP in the middle of single-stepping? */ static int -currently_stepping_thread (struct thread_info *tp) +currently_stepping (struct thread_info *tp) { - return (tp->step_range_end && tp->step_resume_breakpoint == NULL) - || tp->trap_expected - || tp->stepping_through_solib_after_catch; + return ((tp->step_range_end && tp->step_resume_breakpoint == NULL) + || tp->trap_expected + || tp->stepping_through_solib_after_catch + || bpstat_should_step ()); } -static int -currently_stepping_callback (struct thread_info *tp, void *data) -{ - /* Return true if any thread *but* the one passed in "data" is - in the middle of stepping. */ - return tp != data && currently_stepping_thread (tp); -} +/* Returns true if any thread *but* the one passed in "data" is in the + middle of stepping or of handling a "next". */ static int -currently_stepping (struct thread_info *tp) +currently_stepping_or_nexting_callback (struct thread_info *tp, void *data) { - return currently_stepping_thread (tp) || bpstat_should_step (); + if (tp == data) + return 0; + + return (tp->step_range_end + || tp->trap_expected + || tp->stepping_through_solib_after_catch); } /* Inferior has stepped into a subroutine call with source code that Index: src/gdb/testsuite/gdb.threads/threxit-hop-specific.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ src/gdb/testsuite/gdb.threads/threxit-hop-specific.c 2009-05-27 22:40:47.000000000 +0100 @@ -0,0 +1,48 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2009 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 +#include + +void * +thread_function (void *arg) +{ + /* We'll next over this, with scheduler-locking off. */ + pthread_exit (NULL); +} + +void +hop_me (void) +{ +} + +int +main (int argc, char **argv) +{ + pthread_t thread; + + pthread_create (&thread, NULL, thread_function, NULL); + pthread_join (thread, NULL); /* wait for exit */ + + /* The main thread should be able to hoop over the breakpoint set + here.*/ + hop_me (); /* set thread specific breakpoint here */ + + /* ... and reach here. */ + exit (0); /* set exit breakpoint here */ +} Index: src/gdb/testsuite/gdb.threads/threxit-hop-specific.exp =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ src/gdb/testsuite/gdb.threads/threxit-hop-specific.exp 2009-05-27 22:00:34.000000000 +0100 @@ -0,0 +1,59 @@ +# Copyright (C) 2009 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 . + +# Test that GDB doesn't get stuck when thread hoping over a thread +# specific breakpoint when the selected thread has gone away. + +set testfile "threxit-hop-specific" +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} + +runto_main + +# Get ourselves to the thread that exits +gdb_breakpoint "thread_function" +gdb_test "continue" ".*thread_function.*" "continue to thread start" + +# Set a thread specific breakpoint somewhere the main thread will pass +# by, but make it specific to the thread that is going to exit. Step +# over the pthread_exit call. GDB should still be able to step over +# the thread specific breakpoint, and reach the other breakpoint, +# which is not thread specific. +set bpthrline [gdb_get_line_number "set thread specific breakpoint here"] +gdb_test "break $bpthrline thread 2" \ + "Breakpoint .*$srcfile.*$bpthrline.*" \ + "set thread specific breakpoint" + +set bpexitline [gdb_get_line_number "set exit breakpoint here"] +gdb_breakpoint "$bpexitline" + +gdb_test "next" \ + ".*set exit breakpoint here.*" \ + "get passed the thread specific breakpoint" + +return 0 Index: src/gdb/testsuite/gdb.threads/thread-execl.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ src/gdb/testsuite/gdb.threads/thread-execl.c 2009-05-27 22:00:34.000000000 +0100 @@ -0,0 +1,48 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2009 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 +#include + +static const char *image; + +void * +thread_execler (void *arg) +{ + /* Exec ourselves again. */ + if (execl (image, image, NULL) == -1) + { + perror ("execl"); + abort (); + } + + return NULL; +} + +int +main (int argc, char **argv) +{ + pthread_t thread; + + image = argv[0]; + + pthread_create (&thread, NULL, thread_execler, NULL); + pthread_join (thread, NULL); + + return 0; +} Index: src/gdb/testsuite/gdb.threads/thread-execl.exp =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ src/gdb/testsuite/gdb.threads/thread-execl.exp 2009-05-27 22:00:34.000000000 +0100 @@ -0,0 +1,52 @@ +# Copyright (C) 2009 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 . + +# Test that GDB doesn't get stuck when stepping over an exec call done +# by a thread other than the main thread. + +# There's no support for exec events in the remote protocol. +if { [is_remote target] } { + return 0 +} + +set testfile "thread-execl" +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} + +runto_main + +# Get ourselves to the thread that execs +gdb_breakpoint "thread_execler" +gdb_test "continue" ".*thread_execler.*" "continue to thread start" + +# Now set a breakpoint at `main', and step over the execl call. The +# breakpoint at main should be reached. GDB should not try to revert +# back to the old thread from the old image and resume stepping it +# (since it is gone). +gdb_breakpoint "main" +gdb_test "next" ".*main.*" "get to main in new image" + +return 0