From patchwork Wed Jun 15 16:41:16 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yao Qi X-Patchwork-Id: 13114 Received: (qmail 55377 invoked by alias); 15 Jun 2016 16:41:45 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 55334 invoked by uid 89); 15 Jun 2016 16:41:41 -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=lp, arrives X-HELO: mail-pf0-f194.google.com Received: from mail-pf0-f194.google.com (HELO mail-pf0-f194.google.com) (209.85.192.194) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 15 Jun 2016 16:41:30 +0000 Received: by mail-pf0-f194.google.com with SMTP id t190so2103555pfb.2 for ; Wed, 15 Jun 2016 09:41:30 -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=c6qbKJLucTnUavp7ISUzZ0Y/KhqyI/0tXg4VUnf4DKI=; b=QCCOuvIEo52+l/siiaRO2Ga+75ku/4AQZ4H5Xra9fhuPAyg2wNMARnWRsAdOizfCy5 JWkdcr7Jk8Rb1HQWKVAeQE3MUToNadVPsubJPwSOuqf+OxxKinuKAgvwjERf+Lnyznxl Ob2KiZHstTUcSMjnEQ9bYTlvMndzzGlEkSgpLE5RD94giQxd3cSYNYjxIh9hk75ltp4m JjWLd9DHLissms1ifKYuqozf9CSrGb5MSjTv1kwDda3xojueuL8Gp5+XGaeUNO7pTwNt hTxN6b5Jo7Hm5RsXmNIEOsxxUGlm0+HkEjRojny4lAQmK2HAKmpYsiYQNSpo8d+VoQfW 76Xw== X-Gm-Message-State: ALyK8tKzdUxR8oFKEdE0hyVtu2PI0IX9+V2icWGhN8Qi/PoTM0zOzaygrChIjWgcCaeoGg== X-Received: by 10.98.4.195 with SMTP id 186mr4737990pfe.98.1466008888596; Wed, 15 Jun 2016 09:41:28 -0700 (PDT) Received: from E107787-LIN (gcc113.osuosl.org. [140.211.9.71]) by smtp.gmail.com with ESMTPSA id o84sm49816548pfj.95.2016.06.15.09.41.24 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 15 Jun 2016 09:41:27 -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> <86h9cvud2z.fsf@gmail.com> <1cec772e-a659-3f2f-1eae-67d27fdbd9e0@redhat.com> Date: Wed, 15 Jun 2016 17:41:16 +0100 In-Reply-To: <1cec772e-a659-3f2f-1eae-67d27fdbd9e0@redhat.com> (Pedro Alves's message of "Tue, 14 Jun 2016 16:48:47 +0100") Message-ID: <86a8imtnf7.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Pedro Alves writes: > That doesn't tell the reader why we need to stop _all_ threads. The > threads that are about to be resumed are obviously stopped, and > thus we could already _access_ inferior memory through them. GDB may only resume some threads, and leave other threads running. In order to access inferior memory safely, we must stop all threads. > > I guess this is about flushing instruction caches? > No, it is not about flushing instruction caches. >>>> @@ -5176,6 +5241,30 @@ proceed_all_lwps (void) >>>> if (debug_threads) >>>> debug_printf ("Proceeding, no step-over needed\n"); >>>> >>>> + /* 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. > > Is that all events, even internal events? From the patch, it seemed > like it was only before reporting an event to gdb. > You are right, I though too much about supporting range-stepping. I rewrite the comments in the patch below, >> If the client wants >> thread step, re-install these reinsert breakpoints. */ >> > > If we only remove before reporting an event to gdb, then I don't > understand this. We already insert single-step breakpoints when > we process the resume request from gdb, no? We insert single-step breakpoints when we process the resume requests and threads are about to be resumed. If threads still have pending status, single-step breakpoints are not installed, so we need to install them in proceed_all_lwps. diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 5d0eafc..dc28c0d 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -2567,7 +2567,10 @@ resume_stopped_resumed_lwps (struct inferior_list_entry *entry) && !lp->status_pending_p && thread->last_status.kind == TARGET_WAITKIND_IGNORE) { - int step = thread->last_resume_kind == resume_step; + int step = 0; + + if (thread->last_resume_kind == resume_step) + step = maybe_hw_step (thread); if (debug_threads) debug_printf ("RSRL: resuming stopped-resumed LWP %s at %s: step=%d\n", @@ -3518,6 +3521,23 @@ linux_wait_1 (ptid_t ptid, return ignore_event (ourstatus); } + /* Remove reinsert breakpoints ... */ + if (can_software_single_step () + && has_reinsert_breakpoints (current_thread) + /*... if GDB requests this thread doing resume_step or ...*/ + && (current_thread->last_resume_kind == resume_step + /* GDBserver has already started the step-over for vCont;s, + but it gets some other signal, like SIGSTOP sent by + GDBserver for vCont;t or other signal program received. */ + || !maybe_internal_trap)) + { + stop_all_lwps (1, event_child); + + delete_reinsert_breakpoints (current_thread); + + unstop_all_lwps (1, event_child); + } + /* Note that all addresses are always "out of the step range" when there's no range to begin with. */ in_step_range = lwp_in_step_range (event_child); @@ -4281,12 +4301,6 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp, step = maybe_hw_step (thread); } - else - { - /* If the thread isn't doing step-over, there shouldn't be any - reinsert breakpoints. */ - gdb_assert (!has_reinsert_breakpoints (thread)); - } if (fast_tp_collecting == 1) { @@ -4841,7 +4855,6 @@ linux_resume_one_thread (struct inferior_list_entry *entry, void *arg) { struct thread_info *thread = (struct thread_info *) entry; struct lwp_info *lwp = get_thread_lwp (thread); - int step; int leave_all_stopped = * (int *) arg; int leave_pending; @@ -4910,10 +4923,14 @@ linux_resume_one_thread (struct inferior_list_entry *entry, void *arg) if (!leave_pending) { + int step = 0; + if (debug_threads) debug_printf ("resuming LWP %ld\n", lwpid_of (thread)); - step = (lwp->resume->kind == resume_step); + if (lwp->resume->kind == resume_step) + step = maybe_hw_step (thread); + linux_resume_one_lwp (lwp, step, lwp->resume->sig, NULL); } else @@ -4954,6 +4971,7 @@ linux_resume (struct thread_resume *resume_info, size_t n) struct thread_info *need_step_over = NULL; int any_pending; int leave_all_stopped; + int resume_step_is_handled = 0; if (debug_threads) { @@ -4997,12 +5015,55 @@ linux_resume (struct thread_resume *resume_info, size_t n) debug_printf ("Resuming, no pending status or step over needed\n"); } + /* If resume_step is requested by GDB, install reinsert breakpoints + when the thread is about to be actually resumed. IOW, we don't + insert reinsert breakpoints if any thread has pending status. */ + if (!leave_all_stopped && can_software_single_step ()) + { + struct inferior_list_entry *inf, *tmp; + + if (debug_threads) + debug_printf ("Handle resume_step.\n"); + + ALL_INFERIORS (&all_threads, inf, tmp) + { + struct thread_info *thread = (struct thread_info *) inf; + struct lwp_info *lwp = get_thread_lwp (thread); + + if (lwp->resume != NULL && lwp->resume->kind == resume_step) + { + if (!resume_step_is_handled) + { + /* We need to access the inferior memory to install + reinsert breakpoints, so stop all threads. */ + stop_all_lwps (0, NULL); + + if (debug_threads) + debug_printf ("Done stopping all threads.\n"); + + resume_step_is_handled = 1; + } + + install_software_single_step_breakpoints (lwp); + + if (debug_threads) + debug_printf ("Insert breakpoint for resume_step LWP %ld\n", + lwpid_of (thread)); + } + } + + if (debug_threads) + debug_printf ("Handle resume_step. Done\n"); + } + /* Even if we're leaving threads stopped, queue all signals we'd otherwise deliver. */ find_inferior (&all_threads, linux_resume_one_thread, &leave_all_stopped); if (need_step_over) start_step_over (get_thread_lwp (need_step_over)); + else if (resume_step_is_handled) + unstop_all_lwps (0, NULL); if (debug_threads) { @@ -5098,7 +5159,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 = 1; + + step = maybe_hw_step (thread); } else if (lwp->bp_reinsert != 0) { @@ -5164,6 +5226,33 @@ proceed_all_lwps (void) if (debug_threads) debug_printf ("Proceeding, no step-over needed\n"); + if (can_software_single_step ()) + { + struct inferior_list_entry *inf, *tmp; + + ALL_INFERIORS (&all_threads, inf, tmp) + { + struct thread_info *thread = (struct thread_info *) inf; + + /* On software single step target, we insert reinsert + breakpoints when the threads are about to be actually + resumed. IOW, we don't insert them if any thread has + pending status. Before we proceed threads, insert + reinsert breakpoints if the client wants it step. */ + if (thread->last_resume_kind == resume_step) + { + struct lwp_info *lwp = get_thread_lwp (thread); + + if (!has_reinsert_breakpoints (thread)) + install_software_single_step_breakpoints (lwp); + + if (debug_threads) + debug_printf ("Insert breakpoint for resume_step LWP %ld\n", + lwpid_of (thread)); + } + } + } + find_inferior (&all_threads, proceed_one_lwp, NULL); }