From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 77466 invoked by alias); 22 Apr 2015 20:16:00 -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 77457 invoked by uid 89); 22 Apr 2015 20:16:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 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; Wed, 22 Apr 2015 20:15:58 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 9FF98B59BC; Wed, 22 Apr 2015 20:15:57 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3MKFuTo016139; Wed, 22 Apr 2015 16:15:56 -0400 Message-ID: <5538017B.9040907@redhat.com> Date: Wed, 22 Apr 2015 20:16:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH v3 10/17] Implement all-stop on top of a target running non-stop mode References: <1429267521-21047-1-git-send-email-palves@redhat.com> <1429267521-21047-11-git-send-email-palves@redhat.com> <86a8y1zqkd.fsf@gmail.com> In-Reply-To: <86a8y1zqkd.fsf@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-04/txt/msg00846.txt.bz2 On 04/21/2015 12:08 PM, Yao Qi wrote: > Pedro Alves writes: > >> gdb/ChangeLog: >> 2015-04-17 Pedro Alves >> >> * NEWS: Mention "maint set/show target-non-stop". >> * breakpoint.c (update_global_location_list): Check >> target_is_non_stop_p instead of non_stop. >> * infcmd.c (attach_command_post_wait, attach_command): Likewise. >> * infrun.c (show_can_use_displaced_stepping) >> (can_use_displaced_stepping_p, start_step_over_inferior): >> Likewise. >> (resume): Always resume a single thread if the target is in >> non-stop mode. >> (proceed): Check target_is_non_stop_p instead of non_stop. If in >> all-mode but the target is always in non-stop mode, start all the > ^^^^^^^^ > > all-stop mode. Thanks, fixed. > >> (handle_signal_stop) : If we get a signal while >> stepping over a breakpoint, and the target is always in non-stop >> mode, restart all threads. > >> @@ -5614,6 +5666,17 @@ handle_signal_stop (struct execution_control_state *ecs) >> /* Reset trap_expected to ensure breakpoints are re-inserted. */ >> ecs->event_thread->control.trap_expected = 0; >> >> + if (!non_stop && target_is_non_stop_p ()) >> + { > > This path is about the case that a signal is got while in in-line > stepping, isn't? If so, non_stop should be an invariant false. We > don't need to check it. Hmm, not sure what you mean: - We need to do this with displaced stepping too, because we can't deliver signals while doing a displaced step. See comments at the top of displaced_step_prepare and in do_target_resume. - We can certainly get a signal while doing an in-line step-over. The simplest would be, trying to step-over a breakpoint here: *(volatile int *)0 = 1; which usually results SIGSEGV. - We can step past breakpoints in-line in non-stop mode too. However, I agree there's something wrong here. If we get here with "set non-stop on", and we were doing an in-line step-over, then we also need to restart threads, not just in all-stop + "target always-non-stop". I've applied this change on top now, and it tested OK on x86-64, native with both displaced on/off, and gdbserver. diff --git a/gdb/infrun.c b/gdb/infrun.c index e236510..3e60313 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -5732,6 +5732,8 @@ handle_signal_stop (struct execution_control_state *ecs) && ecs->event_thread->control.trap_expected && ecs->event_thread->control.step_resume_breakpoint == NULL) { + int was_in_line; + /* We were just starting a new sequence, attempting to single-step off of a breakpoint and expecting a SIGTRAP. Instead this signal arrives. This signal will take us out @@ -5747,20 +5749,29 @@ handle_signal_stop (struct execution_control_state *ecs) "infrun: signal arrived while stepping over " "breakpoint\n"); + was_in_line = step_over_info_valid_p (); clear_step_over_info (); insert_hp_step_resume_breakpoint_at_frame (frame); ecs->event_thread->step_after_step_resume_breakpoint = 1; /* Reset trap_expected to ensure breakpoints are re-inserted. */ ecs->event_thread->control.trap_expected = 0; - if (!non_stop && target_is_non_stop_p ()) + if (target_is_non_stop_p ()) { keep_going (ecs); - /* We've canceled the step-over temporarily while the - signal handler executes. Let other threads run, - according to schedlock. */ - restart_threads (ecs->event_thread); + /* The step-over has been canceled temporarily while the + signal handler executes. */ + if (was_in_line) + { + /* We had paused all threads, restart them. */ + restart_threads (ecs->event_thread); + } + else if (debug_infrun) + { + fprintf_unfiltered (gdb_stdlog, + "infrun: no need to restart threads\n"); + } return; } -- 1.9.3 I've now folded that into the patch that teaches non-stop about in-line step overs. >> + keep_going (ecs); >> + >> + /* We've canceled the step-over temporarily while the >> + signal handler executes. Let other threads run, >> + according to schedlock. */ > > IMO, we don't need to run other threads according to schedlock. GDB > resumes some threads here and operations should be invisible to user, > schedlock, as a user visible option, shouldn't affect what threads > should be resumed. On the other hand, restart_threads is to undo the > changes done by stop_all_threads, so no user preference (schedlock) > should be considered here. Yes, you're right, thanks for noticing this. The code in restart_threads does not in fact look at schedlock (through e.g., user_visible_resume_ptid) at all, it's the comment that is stale from a previous attempt, from before https://sourceware.org/ml/gdb-patches/2015-03/msg00293.html. It was issues like that led to that schedlock patch, even.