From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18814 invoked by alias); 21 Apr 2015 02:36:04 -0000 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 Received: (qmail 18802 invoked by uid 89); 21 Apr 2015 02:36:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail-yh0-f74.google.com Received: from mail-yh0-f74.google.com (HELO mail-yh0-f74.google.com) (209.85.213.74) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 21 Apr 2015 02:36:00 +0000 Received: by yhzz6 with SMTP id z6so3835712yhz.0 for ; Mon, 20 Apr 2015 19:35:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:in-reply-to:references:cc:date :message-id:mime-version:content-type; bh=Xu8Q6VyDpAbugwyv/2BX8RprrnzzB6R6uXZ8s3QWyt8=; b=BHhVimFKQc6DadThlq/jSCYFdTAQTj0XaCU/4+5XCxhObYKeKkaUnUXtbA47B2W82F TVXWdklk4ASUqZE7ZS2Gw4Lbn8ztRYUZj1ARWCNNVsNfZJCwJYb5rwlDz8PtzELMj+IB WqSI/rnJlrZMess0ztomWyon4PmxzShthQwiZkWqW7YRNer0NoXr2YSyFb61DDQd6A// UkBIMd9tI+a0V84GVvbRMoys47sPTZwBhEGi/EWji5KsUeMXptPwyRy8n1BPhLwFn555 WSCxTDbo10go5ThcivaJQKPkPCpJXNB4/xaTwCtX5I3qc/d+4flfTR5A6hur1b+LeScE NG+A== X-Gm-Message-State: ALoCoQlgipwHHDdux6jE0FI5vJarFKJ6ePm5FgrzmxctyB4+U8tnB3uQz1Z7/heDAfy8RgaVrQsd7WaFchFt5fn0QITGaGyHrRHJKhbTNstNIf7lWDgtgEeNwcK4tmZgzKKYnU4qrq4uvxt4FCmnVt9vVXbAWw/UFmZk7wjHrGrZ/Lev4bRaFRY= X-Received: by 10.236.110.4 with SMTP id t4mr26952005yhg.50.1429583758458; Mon, 20 Apr 2015 19:35:58 -0700 (PDT) Received: from corpmail-nozzle1-2.hot.corp.google.com ([100.108.1.103]) by gmr-mx.google.com with ESMTPS id f100si89746yhp.7.2015.04.20.19.35.58 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 20 Apr 2015 19:35:58 -0700 (PDT) Received: from ruffy.mtv.corp.google.com ([172.17.128.44]) by corpmail-nozzle1-2.hot.corp.google.com with ESMTPS id CdsZ8DED.1; Mon, 20 Apr 2015 19:35:58 -0700 From: Doug Evans To: Pedro Alves Subject: Re: [PATCH v3 01/17] Fix and test "checkpoint" in non-stop mode In-Reply-To: <1429267521-21047-2-git-send-email-palves@redhat.com> References: <1429267521-21047-1-git-send-email-palves@redhat.com> <1429267521-21047-2-git-send-email-palves@redhat.com> cc: gdb-patches@sourceware.org Date: Tue, 21 Apr 2015 02:36:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg00750.txt.bz2 Pedro Alves writes: > Letting a "checkpoint" run to exit with "set non-stop on" behaves > differently compared to the default all-stop mode ("set non-stop > off"). > > Currently, in non-stop mode: > > (gdb) start > Temporary breakpoint 1 at 0x40086b: file src/gdb/testsuite/gdb.base/checkpoint.c, line 28. > Starting program: build/gdb/testsuite/gdb.base/checkpoint > > Temporary breakpoint 1, main () at src/gdb/testsuite/gdb.base/checkpoint.c:28 > 28 char *tmp = &linebuf[0]; > (gdb) checkpoint > checkpoint 1: fork returned pid 24948. > (gdb) c > Continuing. > Copy complete. > Deleting copy. > [Inferior 1 (process 24944) exited normally] > [Switching to process 24948] > (gdb) info threads > Id Target Id Frame > 1 process 24948 "checkpoint" (running) > > No selected thread. See `help thread'. > (gdb) c > The program is not being run. > (gdb) > > Two issues above: > > 1. Thread 1 got stuck in "(running)" state (it isn't really running) > > 2. While checkpoints try to preserve the illusion that the thread is > still the same when the process exits, GDB switched to "No thread > selected." instead of staying with thread 1 selected. > > Problem #1 is caused by handle_inferior_event and normal_stop not > considering that when a > TARGET_WAITKIND_SIGNALLED/TARGET_WAITKIND_EXITED event is reported, > and the inferior is mourned, the target may still have execution. Hi. What does "target may still have execution" mean here? Is it that there can be another inferior running? > Problem #2 is caused by the make_cleanup_restore_current_thread > cleanup installed by fetch_inferior_event not being able to find the > original thread 1's ptid in the thread list, thus not being able to > restore thread 1 as selected thread. The fix is to make the cleanup > installed by make_cleanup_restore_current_thread aware of thread ptid > changes, by installing a thread_ptid_changed observer that adjusts the > cleanup's data. I'm guessing this is less hacky than it sounds :-), but it does make me want to understand why this only comes up now since threads/inferiors have always been able to "go away" while gdb is doing something. > After the patch, we get the same in all-stop and non-stop modes: > > (gdb) c > Continuing. > Copy complete. > Deleting copy. > [Inferior 1 (process 25109) exited normally] > [Switching to process 25113] > (gdb) info threads > Id Target Id Frame > * 1 process 25113 "checkpoint" main () at src/gdb/testsuite/gdb.base/checkpoint.c:28 > (gdb) > > Turns out the whole checkpoints.exp file can run in non-stop mode > unmodified. I thought of moving most of the test file's contents to a > procedure that can be called twice, once in non-stop mode and another > in all-stop mode. But then, the test already takes over 30 seconds to > run on my machine, so I thought it'd be nicer to run all-stop and > non-stop mode in parallel. Thus I added a new checkpoint-ns.exp file > that just sources checkpoint.exp, and sets a knob that checkpoint.exp > reads to know it should test non-stop mode. No other test in the tree > currently uses this mechanism, but I can't see a reason we shouldn't > do this. Re: checkpoint-ns.exp: If we're going to have one "make check" involve both all-stop and non-stop testing (as opposed to doing one "make check" with all-stop and another one with non-stop), I'd rather see a more table-driven approach (the details don't matter to me much other than I'd rather have one file than a proliferation of foo-ns.exp files). Parallelizing that may involve an extra step, but that's ok. > > gdb/ChangeLog: > 2015-04-17 Pedro Alves > > * infrun.c (handle_inferior_event): If we get > TARGET_WAITKIND_SIGNALLED or TARGET_WAITKIND_EXITED in non-stop > mode, mark all threads of the exiting process as not-executing. > (normal_stop): If we get TARGET_WAITKIND_SIGNALLED or > TARGET_WAITKIND_EXITED in non-stop mode, finish all threads of the > exiting process, if inferior_ptid still points at a process. > * thread.c (struct current_thread_cleanup) : New field. > (current_thread_cleanup_chain): New global. > (restore_current_thread_ptid_changed): New function. > (restore_current_thread_cleanup_dtor): Remove the cleanup from the > current_thread_cleanup_chain list. > (make_cleanup_restore_current_thread): Add the cleanup data to the > current_thread_cleanup_chain list. > (_initialize_thread): Install restore_current_thread_ptid_changed > as thread_ptid_changed observer. > > gdb/testsuite/ChangeLog: > 2015-04-17 Pedro Alves > > * gdb.base/checkpoint-ns.exp: New file. > * gdb.base/checkpoint.exp (checkpoint_non_stop): New input > variable. If set, test in non-stop mode. Pass explicit > "checkpoint.c" to standard_testfile. > > v3: > No changes. > --- > gdb/infrun.c | 31 +++++++++++++++++++++----- > gdb/testsuite/gdb.base/checkpoint-ns.exp | 27 +++++++++++++++++++++++ > gdb/testsuite/gdb.base/checkpoint.exp | 31 +++++++++++++++++++++----- > gdb/thread.c | 38 ++++++++++++++++++++++++++++++++ > 4 files changed, 117 insertions(+), 10 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/checkpoint-ns.exp > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 7870f70..9c1fdc5 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -3804,8 +3804,18 @@ handle_inferior_event (struct execution_control_state *ecs) > any other process were left running. */ > if (!non_stop) > set_executing (minus_one_ptid, 0); > - else if (ecs->ws.kind != TARGET_WAITKIND_SIGNALLED > - && ecs->ws.kind != TARGET_WAITKIND_EXITED) > + else if (ecs->ws.kind == TARGET_WAITKIND_SIGNALLED > + && ecs->ws.kind == TARGET_WAITKIND_EXITED) pasto: s/&&/||/ > + { > + ptid_t pid_ptid; > + > + /* Some targets still have execution when a process exits. > + E.g., for "checkpoint", when when a fork exits and is s/when when/when/ > + mourned, linux-fork.c switches to another fork. */ > + pid_ptid = pid_to_ptid (ptid_get_pid (ecs->ptid)); > + set_executing (pid_ptid, 0); This is a bit confusing. I'm guessing ecs->ptid is for the inferior that just exited/signalled, but we're not changing pids here. I'm guessing we'll mourn the inferior later, but IWBN to elaborate on the comment a little, e.g., to say we always need to call set_executing here, even if the inferior exited/signalled (which the code didn't previously do). But it's not clear why "switches to another fork" comes into play here. > + } > + else > set_executing (ecs->ptid, 0); > > switch (ecs->ws.kind) > @@ -6550,6 +6560,7 @@ normal_stop (void) > struct target_waitstatus last; > ptid_t last_ptid; > struct cleanup *old_chain = make_cleanup (null_cleanup, NULL); > + ptid_t pid_ptid; > > get_last_target_status (&last_ptid, &last); > > @@ -6559,9 +6570,19 @@ normal_stop (void) > here, so do this before any filtered output. */ > if (!non_stop) > make_cleanup (finish_thread_state_cleanup, &minus_one_ptid); > - else if (last.kind != TARGET_WAITKIND_SIGNALLED > - && last.kind != TARGET_WAITKIND_EXITED > - && last.kind != TARGET_WAITKIND_NO_RESUMED) > + else if (last.kind == TARGET_WAITKIND_SIGNALLED > + || last.kind == TARGET_WAITKIND_EXITED) > + { > + /* Some targets still have execution when a process exits. > + E.g., for "checkpoint", when when a fork exits and is s/when when/when/ > + mourned, linux-fork.c switches to another fork. */ > + if (!ptid_equal (inferior_ptid, null_ptid)) > + { > + pid_ptid = pid_to_ptid (ptid_get_pid (inferior_ptid)); > + make_cleanup (finish_thread_state_cleanup, &pid_ptid); > + } > + } > + else if (last.kind != TARGET_WAITKIND_NO_RESUMED) > make_cleanup (finish_thread_state_cleanup, &inferior_ptid); > > /* As we're presenting a stop, and potentially removing breakpoints, > diff --git a/gdb/testsuite/gdb.base/checkpoint-ns.exp b/gdb/testsuite/gdb.base/checkpoint-ns.exp > new file mode 100644 > index 0000000..03a1747 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/checkpoint-ns.exp > @@ -0,0 +1,27 @@ > +# Copyright 2015 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 gdb checkpoint and restart in non-stop mode. > + > +# We drive non-stop mode from a separate file because the whole test > +# takes a while to run. This way, we can test both modes in parallel. > + > +# checkpoint.exp reads this variable. > +set checkpoint_non_stop 1 > + > +source $srcdir/$subdir/checkpoint.exp > + > +# Unset to avoid problems with non-parallel mode. > +unset checkpoint_non_stop > diff --git a/gdb/testsuite/gdb.base/checkpoint.exp b/gdb/testsuite/gdb.base/checkpoint.exp > index 6d94ab6..c297b50 100644 > --- a/gdb/testsuite/gdb.base/checkpoint.exp > +++ b/gdb/testsuite/gdb.base/checkpoint.exp > @@ -13,6 +13,20 @@ > # You should have received a copy of the GNU General Public License > # along with this program. If not, see . */ > > +# > +# This tests gdb checkpoint and restart. > +# > + > +# Note this file is also sourced by checkpoint-ns.exp to run all the > +# tests below in non-stop mode. That's driven by a separate file so > +# testing both all-stop and non-stop can be done in parallel. > + > +# This is the variable checkpoint-ns.exp sets to request non-stop > +# mode. When not set, default to all-stop mode. > +if {![info exists checkpoint_non_stop]} { > + set checkpoint_non_stop 0 > +} > + > if { [is_remote target] || ![isnative] } then { > continue > } > @@ -24,8 +38,10 @@ if {![istarget "*-*-linux*"]} then { > continue > } > > - > -standard_testfile .c > +# Must name the source file explicitly, otherwise when driven by > +# checkpoints-ns.exp, we'd try compiling checkpoints-ns.c, which > +# doesn't exist. > +standard_testfile checkpoint.c > > set pi_txt [gdb_remote_download host ${srcdir}/${subdir}/pi.txt] > if {[is_remote host]} { > @@ -42,9 +58,9 @@ if {[prepare_for_testing ${testfile}.exp $testfile $srcfile \ > > global gdb_prompt > > -# > -# This tests gdb checkpoint and restart. > -# > +if {$checkpoint_non_stop} { > + gdb_test_no_output "set non-stop on" > +} > > runto_main > set break1_loc [gdb_get_line_number "breakpoint 1"] > @@ -327,6 +343,11 @@ gdb_start > gdb_reinitialize_dir $srcdir/$subdir > gdb_load ${binfile} > > +if {$checkpoint_non_stop} { > + gdb_test_no_output "set non-stop on" \ > + "set non-stop on, for many checkpoints" > +} > + > runto_main > gdb_breakpoint $break1_loc > > diff --git a/gdb/thread.c b/gdb/thread.c > index 23dfcc9..46b5947 100644 > --- a/gdb/thread.c > +++ b/gdb/thread.c > @@ -1279,8 +1279,16 @@ restore_selected_frame (struct frame_id a_frame_id, int frame_level) > } > } > > +/* Data used by the cleanup installed by > + 'make_cleanup_restore_current_thread'. */ > + > struct current_thread_cleanup > { > + /* Next in list of currently installed 'struct > + current_thread_cleanup' cleanups. See > + 'current_thread_cleanup_chain' below. */ > + struct current_thread_cleanup *next; > + > ptid_t inferior_ptid; > struct frame_id selected_frame_id; > int selected_frame_level; > @@ -1289,6 +1297,29 @@ struct current_thread_cleanup > int was_removable; > }; > > +/* A chain of currently installed 'struct current_thread_cleanup' > + cleanups. Restoring the previously selected thread looks up the > + old thread in the thread list by ptid. If the thread changes ptid, > + we need to update the cleanup's thread structure so the look up > + succeeds. */ > +static struct current_thread_cleanup *current_thread_cleanup_chain; > + > +/* A thread_ptid_changed observer. Update all currently installed > + current_thread_cleanup cleanups that want to switch back to > + OLD_PTID to switch back to NEW_PTID instead. */ > + > +static void > +restore_current_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid) > +{ > + struct current_thread_cleanup *it; > + > + for (it = current_thread_cleanup_chain; it != NULL; it = it->next) > + { > + if (ptid_equal (it->inferior_ptid, old_ptid)) > + it->inferior_ptid = new_ptid; > + } > +} > + > static void > do_restore_current_thread_cleanup (void *arg) > { > @@ -1329,6 +1360,8 @@ restore_current_thread_cleanup_dtor (void *arg) > struct thread_info *tp; > struct inferior *inf; > > + current_thread_cleanup_chain = current_thread_cleanup_chain->next; > + > tp = find_thread_ptid (old->inferior_ptid); > if (tp) > tp->refcount--; > @@ -1362,6 +1395,9 @@ make_cleanup_restore_current_thread (void) > old->inf_id = current_inferior ()->num; > old->was_removable = current_inferior ()->removable; > > + old->next = current_thread_cleanup_chain; > + current_thread_cleanup_chain = old; > + > if (!ptid_equal (inferior_ptid, null_ptid)) > { > old->was_stopped = is_stopped (inferior_ptid); > @@ -1815,4 +1851,6 @@ Show printing of thread events (such as thread start and exit)."), NULL, > &setprintlist, &showprintlist); > > create_internalvar_type_lazy ("_thread", &thread_funcs, NULL); > + > + observer_attach_thread_ptid_changed (restore_current_thread_ptid_changed); > } > -- > 1.9.3 > -- /dje