From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14919 invoked by alias); 17 Oct 2010 19:04:32 -0000 Received: (qmail 14909 invoked by uid 22791); 17 Oct 2010 19:04:30 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,TW_TG,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 17 Oct 2010 19:04:25 +0000 Received: (qmail 6015 invoked from network); 17 Oct 2010 19:04:23 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 17 Oct 2010 19:04:23 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [rfc] linux-nat: Never PTRACE_CONT a stepping thread Date: Sun, 17 Oct 2010 19:04:00 -0000 User-Agent: KMail/1.13.2 (Linux/2.6.33-29-realtime; KDE/4.4.2; x86_64; ; ) Cc: Jan Kratochvil References: <20100921234325.GA31267@host1.dyn.jankratochvil.net> <201010161809.53308.pedro@codesourcery.com> <20101017182745.GA9936@host1.dyn.jankratochvil.net> In-Reply-To: <20101017182745.GA9936@host1.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201010172004.18986.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: 2010-10/txt/msg00278.txt.bz2 On Sunday 17 October 2010 19:27:45, Jan Kratochvil wrote: > On Sat, 16 Oct 2010 19:09:52 +0200, Pedro Alves wrote: > > This patch alone on top of current mainline fixes the sigstep-threads.exp > > test from . > > Maybe check that test in along with this patch? > > OK, yes, I will recheck that patch on top of this one. > > > > The corresponding remote.c patch (pasted below) that translates this > > into a vCont with three actions (e.g., "vCont;C1e:795a;s:7959;c") also > > fixes that test for gdbserver linux. > > Included it. > > > > It'd be much cleaner to make the target_resume interface similar > > to gdbserver's target_ops->resume interface (pass in a list of > > resume actions, similar to vCont), but that shouldn't be a > > prerequisite. Maybe someday... > > I agree that interface is better. Not a scope of this patch, though. Thanks. I just belatedly realized that this probably breaks software single-step archs though. :-/ > > > Checked-in. > > Thanks for the review. > > > Jan > > > http://sourceware.org/ml/gdb-cvs/2010-10/msg00102.html > > --- src/gdb/ChangeLog 2010/10/17 17:45:16 1.12266 > +++ src/gdb/ChangeLog 2010/10/17 18:24:46 1.12267 > @@ -1,4 +1,19 @@ > 2010-10-17 Jan Kratochvil > + Pedro Alves > + > + * gdbthread.h (currently_stepping): New declaration. > + * infrun.c (currently_stepping): Remove the forward declaration. > + (currently_stepping): Make it global. > + * linux-nat.c (resume_callback) stopped && lp->status == 0>: New > + variables tp and step, initialized them. Pass STEP to to_resume. > + Print also possibly "PTRACE_SINGLESTEP" if STEP. Initialize LP->STEP. > + * remote.c (currently_stepping_callback): New. > + (remote_vcont_resume) > + : > + New variable tp. Call currently_stepping_callback and step such > + thread. > + > +2010-10-17 Jan Kratochvil > > * infrun.c (follow_exec): Replace symbol_file_add_main by > symbol_file_add with SYMFILE_DEFER_BP_RESET, set_initial_language and > --- src/gdb/gdbthread.h 2010/01/12 21:40:24 1.54 > +++ src/gdb/gdbthread.h 2010/10/17 18:24:47 1.55 > @@ -352,4 +352,6 @@ > > extern void update_thread_list (void); > > +extern int currently_stepping (struct thread_info *tp); > + > #endif /* GDBTHREAD_H */ > --- src/gdb/infrun.c 2010/10/17 17:45:16 1.452 > +++ src/gdb/infrun.c 2010/10/17 18:24:47 1.453 > @@ -74,8 +74,6 @@ > static void set_schedlock_func (char *args, int from_tty, > struct cmd_list_element *c); > > -static int currently_stepping (struct thread_info *tp); > - > static int currently_stepping_or_nexting_callback (struct thread_info *tp, > void *data); > > @@ -4851,7 +4849,7 @@ > > /* Is thread TP in the middle of single-stepping? */ > > -static int > +int > currently_stepping (struct thread_info *tp) > { > return ((tp->step_range_end && tp->step_resume_breakpoint == NULL) > --- src/gdb/linux-nat.c 2010/09/06 13:59:02 1.184 > +++ src/gdb/linux-nat.c 2010/10/17 18:24:47 1.185 > @@ -1820,20 +1820,26 @@ > } > else if (lp->stopped && lp->status == 0) > { > + struct thread_info *tp = find_thread_ptid (lp->ptid); > + /* lp->step may already contain a stale value. */ > + int step = tp ? currently_stepping (tp) : 0; > + > if (debug_linux_nat) > fprintf_unfiltered (gdb_stdlog, > - "RC: PTRACE_CONT %s, 0, 0 (resuming sibling)\n", > + "RC: %s %s, 0, 0 (resuming sibling)\n", > + step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT", > target_pid_to_str (lp->ptid)); > > linux_ops->to_resume (linux_ops, > pid_to_ptid (GET_LWP (lp->ptid)), > - 0, TARGET_SIGNAL_0); > + step, TARGET_SIGNAL_0); > if (debug_linux_nat) > fprintf_unfiltered (gdb_stdlog, > - "RC: PTRACE_CONT %s, 0, 0 (resume sibling)\n", > + "RC: %s %s, 0, 0 (resume sibling)\n", > + step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT", > target_pid_to_str (lp->ptid)); > lp->stopped = 0; > - lp->step = 0; > + lp->step = step; > memset (&lp->siginfo, 0, sizeof (lp->siginfo)); > lp->stopped_by_watchpoint = 0; > } > --- src/gdb/remote.c 2010/07/28 20:20:26 1.421 > +++ src/gdb/remote.c 2010/10/17 18:24:47 1.422 > @@ -4416,6 +4416,12 @@ > return p; > } > > +static int > +currently_stepping_callback (struct thread_info *tp, void *data) > +{ > + return currently_stepping (tp); > +} > + > /* Resume the remote inferior by using a "vCont" packet. The thread > to be resumed is PTID; STEP and SIGGNAL indicate whether the > resumed thread should be single-stepped and/or signalled. If PTID > @@ -4458,6 +4464,8 @@ > } > else if (ptid_equal (ptid, minus_one_ptid) || ptid_is_pid (ptid)) > { > + struct thread_info *tp; > + > /* Resume all threads (of all processes, or of a single > process), with preference for INFERIOR_PTID. This assumes > inferior_ptid belongs to the set of all threads we are about > @@ -4468,6 +4476,12 @@ > p = append_resumption (p, endp, inferior_ptid, step, siggnal); > } > > + tp = iterate_over_threads (currently_stepping_callback, NULL); > + if (tp && !ptid_equal (tp->ptid, inferior_ptid)) > + { > + p = append_resumption (p, endp, tp->ptid, 1, TARGET_SIGNAL_0); > + } > + > /* And continue others without a signal. */ > p = append_resumption (p, endp, ptid, /*step=*/ 0, TARGET_SIGNAL_0); > } > --- src/gdb/testsuite/ChangeLog 2010/10/17 17:45:17 1.2482 > +++ src/gdb/testsuite/ChangeLog 2010/10/17 18:24:47 1.2483 > @@ -1,5 +1,10 @@ > 2010-10-17 Jan Kratochvil > > + * gdb.threads/sigstep-threads.exp: New file. > + * gdb.threads/sigstep-threads.c: New file. > + > +2010-10-17 Jan Kratochvil > + > * gdb.base/pie-execl.exp: New file. > * gdb.base/pie-execl.c: New file. > > --- src/gdb/testsuite/gdb.threads/sigstep-threads.c > +++ src/gdb/testsuite/gdb.threads/sigstep-threads.c 2010-10-17 18:24:59.596239000 +0000 > @@ -0,0 +1,54 @@ > +/* 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 . */ > + > +#include > +#include > +#include > + > +#include > +#include > +#define tgkill(tgid, tid, sig) syscall (__NR_tgkill, (tgid), (tid), (sig)) > +#define gettid() syscall (__NR_gettid) > + > +static volatile int var; > + > +static void > +handler (int signo) /* step-0 */ > +{ /* step-0 */ > + var++; /* step-1 */ > + tgkill (getpid (), gettid (), SIGUSR1); /* step-2 */ > +} > + > +static void * > +start (void *arg) > +{ > + signal (SIGUSR1, handler); > + tgkill (getpid (), gettid (), SIGUSR1); > + assert (0); > + > + return NULL; > +} > + > +int > +main (void) > +{ > + pthread_t thread; > + > + pthread_create (&thread, NULL, start, NULL); > + start (NULL); /* main-start */ > + return 0; > +} > --- src/gdb/testsuite/gdb.threads/sigstep-threads.exp > +++ src/gdb/testsuite/gdb.threads/sigstep-threads.exp 2010-10-17 18:24:59.896865000 +0000 > @@ -0,0 +1,74 @@ > +# 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 . > + > +set testfile sigstep-threads > +set srcfile ${testfile}.c > +set executable ${testfile} > +set binfile ${objdir}/${subdir}/${executable} > + > +if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { > + untested ${testfile}.exp > + return -1 > +} > + > +clean_restart $executable > + > +if ![runto_main] { > + return -1; > +} > + > +# `noprint' would not test the full logic of GDB. > +gdb_test "handle SIGUSR1 nostop print pass" "\r\nSIGUSR1\[ \t\]+No\[ \t\]+Yes\[ \t\]+Yes\[ \t\].*" > + > +gdb_test_no_output "set scheduler-locking off" > + > +gdb_breakpoint [gdb_get_line_number "step-1"] > +gdb_test_no_output {set $step1=$bpnum} > +gdb_continue_to_breakpoint "step-1" ".* step-1 .*" > +gdb_test_no_output {disable $step1} > + > +# 1 as we are now stopped at the `step-1' label. > +set step_at 1 > +for {set i 0} {$i < 100} {incr i} { > + set test "step $i" > + # Presume this step failed - as in the case of a timeout. > + set failed 1 > + gdb_test_multiple "step" $test { > + -re "\r\nProgram received signal SIGUSR1, User defined signal 1.\r\n" { > + exp_continue -continue_timer > + } > + -re "step-(\[012\]).*\r\n$gdb_prompt $" { > + set now $expect_out(1,string) > + if {$step_at == 2 && $now == 1} { > + set failed 0 > + } elseif {$step_at == 1 && $now == 2} { > + set failed 0 > + # Continue over the re-signalling back to the handle entry. > + gdb_test_no_output {enable $step1} "" > + gdb_test "continue" " step-1 .*" "" > + set now 1 > + gdb_test_no_output {disable $step1} "" > + } else { > + fail $test > + } > + set step_at $now > + } > + } > + if $failed { > + return > + } > +} > +# We can never reliably say the racy problematic case has been tested. > +pass "step" > -- Pedro Alves