From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 60678 invoked by alias); 14 Jun 2016 13:14:58 -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 60657 invoked by uid 89); 14 Jun 2016 13:14:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:2608 X-HELO: mail-pa0-f65.google.com Received: from mail-pa0-f65.google.com (HELO mail-pa0-f65.google.com) (209.85.220.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 14 Jun 2016 13:14:55 +0000 Received: by mail-pa0-f65.google.com with SMTP id us13so1460486pab.1 for ; Tue, 14 Jun 2016 06:14:55 -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:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=3b1C1zz3lMUd1nLVAIlj+6iX097qHUVqQCy/8q7XADY=; b=WLQrw9Tj5aX1wN0EtWvRkkv3jmlkPEuI4rS82Wx20nLXrTgnX7m/vfhaadgC8KnDeN rjGf3RBEn1kR+F6+SzZKDAybq20saMQj1YoRNqOwJl95sABxaRmUa7ViYYAaBZUFzOOG r9hKr90S0COeQFW3JQ5cCqpVTwgYCy+t6lmwH4cL7NCJg5ROra5hZgvMAF/7CqbRpcc2 BPMX4UAPVcyQ+iBFhOutu65N+l6c6zYB0zygWSS9gpyy+74ma8es7MdAw1xfTMKe/8TV 1SHqToqh236USSB3ftvvDbEVhp1VF16b3bk34mze4mm5CnuBze5Z+T/d1NHdLU3XIHBz +TnA== X-Gm-Message-State: ALyK8tIdHlwm/fnnczXvuDRcmhh+L5+brcZUvTBOnK2K43uOAdDo2o1l1fIEwWgg1HMRGg== X-Received: by 10.66.151.71 with SMTP id uo7mr18636707pab.134.1465910093363; Tue, 14 Jun 2016 06:14:53 -0700 (PDT) Received: from E107787-LIN (gcc113.osuosl.org. [140.211.9.71]) by smtp.gmail.com with ESMTPSA id bt5sm13211591pac.47.2016.06.14.06.14.51 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Tue, 14 Jun 2016 06:14:52 -0700 (PDT) From: Yao Qi To: Pedro Alves Cc: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH 11/12] Use reinsert_breakpoint for vCont;s References: <1464859846-15619-1-git-send-email-yao.qi@linaro.org> <1464859846-15619-12-git-send-email-yao.qi@linaro.org> <61835b69-a4bf-a912-4eb3-b223c2a16614@redhat.com> Date: Tue, 14 Jun 2016 13:14:00 -0000 In-Reply-To: <61835b69-a4bf-a912-4eb3-b223c2a16614@redhat.com> (Pedro Alves's message of "Mon, 13 Jun 2016 16:55:29 +0100") Message-ID: <86h9cvud2z.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-06/txt/msg00253.txt.bz2 Pedro Alves writes: >> @@ -4293,7 +4313,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp, >>=20=20 >> step =3D maybe_hw_step (thread); >> } >> - else >> + else if (lwp->resume !=3D NULL && lwp->resume->kind !=3D resume_step) >> { >> /* If the thread isn't doing step-over, there shouldn't be any >> reinsert breakpoints. */ > > Consider (non-stop RSP): > > -> vCont;s:1 > <- OK > -> vCont;s:2 > <- OK > > The handling of the second vCont sets thread 1's lwp->resume to NULL. If so, the assert won't be called for thread 1. > The lwp->resume pointer is only meaningful within linux_resume > and its callees. (But this function is called in other contexts.) > When I wrote the patch, it took me a while to think about this condition check. I wanted to remove this condition and assert, but finally decided to leave it there, as it is not harmful. If lwp->resume is only meaningful within linux_resume and its callees, how about remove the condition check and assert here? >> @@ -5009,12 +5033,52 @@ linux_resume (struct thread_resume *resume_info,= size_t n) >> debug_printf ("Resuming, no pending status or step over needed\n"); >> } >>=20=20 >> + /* Before we resume the threads, if resume_step is requested by GDB, >> + stop all threads and install reinsert breakpoints. */ > > Looking again, I think the rationale for stopping threads should > be mentioned here, as it's not obvious. > How about this, /* Before we resume the threads, if resume_step is requested by GDB, we need to access the inferior memory to install reinsert breakpoints, so stop all threads. */ >> @@ -5110,7 +5174,8 @@ proceed_one_lwp (struct inferior_list_entry *entry= , void *except) >> if (debug_threads) >> debug_printf (" stepping LWP %ld, client wants it stepping\n", >> lwpid_of (thread)); >> - step =3D 1; >> + >> + step =3D maybe_hw_step (thread); >> } >> else if (lwp->bp_reinsert !=3D 0) >> { >> @@ -5176,6 +5241,30 @@ proceed_all_lwps (void) >> if (debug_threads) >> debug_printf ("Proceeding, no step-over needed\n"); >>=20=20 >> + /* Re-install the reinsert breakpoints on software single step target >> + if the client wants it step. */ >> + if (can_software_single_step ()) > > Not immediately obvious to why is this necessary. Where were they > removed in the first place? I'm it must be necessary, but maybe > extending the comment helps. How about this /* On software single step target, we removed reinsert breakpoints after we get any events from the inferior. If the client wants thread step, re-install these reinsert breakpoints. */ --=20 Yao (=E9=BD=90=E5=B0=A7)