From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id AqT1Hpiwvl95FgAAWB0awg (envelope-from ) for ; Wed, 25 Nov 2020 14:29:28 -0500 Received: by simark.ca (Postfix, from userid 112) id 7173A1F0AB; Wed, 25 Nov 2020 14:29:28 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id AA4B81E552 for ; Wed, 25 Nov 2020 14:29:26 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0BA103896C02; Wed, 25 Nov 2020 19:29:26 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0BA103896C02 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1606332566; bh=ozhiMJcBYuhbfhv990mb7TX2cK+cPcUhyDqgkZDBzOw=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=Vh0+2g87WJyt9b1ZrX+Hn37KHnrS90nrlJ2WGMYyUPfB/jx9T8EjgPD+DYGebDviK gErfvrpZ825Wwa5IOMRNVz9CZ70n63hwVAABBhTdRJ/laJ+a6I4GYILWImFUpCiuLE r00HMbihxALBHiii0nHjLSwDYiOgPrZuJZkZwBjo= Received: from mail.efficios.com (mail.efficios.com [167.114.26.124]) by sourceware.org (Postfix) with ESMTPS id DAD763851C1A for ; Wed, 25 Nov 2020 19:29:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DAD763851C1A Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 959D92F0C99; Wed, 25 Nov 2020 14:29:22 -0500 (EST) Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id M3uGz2qMilJA; Wed, 25 Nov 2020 14:29:21 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id AB56F2F09D2; Wed, 25 Nov 2020 14:29:21 -0500 (EST) DKIM-Filter: OpenDKIM Filter v2.10.3 mail.efficios.com AB56F2F09D2 X-Virus-Scanned: amavisd-new at efficios.com Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id jRNWBb1YD41Y; Wed, 25 Nov 2020 14:29:21 -0500 (EST) Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) by mail.efficios.com (Postfix) with ESMTPSA id 849132F09D1; Wed, 25 Nov 2020 14:29:21 -0500 (EST) Subject: Re: [PATCH 09/12] gdb: move displaced stepping logic to gdbarch, allow starting concurrent displaced steps To: Pedro Alves , gdb-patches@sourceware.org References: <20201110214614.2842615-1-simon.marchi@efficios.com> <20201110214614.2842615-10-simon.marchi@efficios.com> <72a75576-a54b-bcc8-e8d3-5a57571fe234@palves.net> Message-ID: <1e9654db-de61-a5d4-48c3-a493b8598f0d@efficios.com> Date: Wed, 25 Nov 2020 14:29:21 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <72a75576-a54b-bcc8-e8d3-5a57571fe234@palves.net> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2020-11-24 8:40 p.m., Pedro Alves wrote: > On 11/10/20 9:46 PM, Simon Marchi via Gdb-patches wrote: > >> - Ask the gdbarch for the displaced step buffer location >> - Save the existing bytes in the displaced step buffer >> - Ask the gdbarch to copy the instruction into the displaced step buffer >> - Set the pc of the thread to the beginning of the displaced step buffer >> >> Similarly, the "fixup" phase, executed after the instruction was >> successfully single-stepped, is driven by the infrun code (function >> displaced_step_fixup). The steps are roughly: > > displaced_step_fixup -> displaced_step_finish. And maybe "finish" phase? Fixed. >> >> - Restore the original bytes in the displaced stepping buffer >> - Ask the gdbarch to fixup the instruction result (adjust the target's >> registers or memory to do as if the instruction had been executed in >> its original location) >> >> The displaced_step_inferior_state::step_thread field indicates which >> thread (if any) is currently using the displaced stepping buffer, so it >> is used by displaced_step_prepare_throw to check if the displaced >> stepping buffer is free to use or not. >> >> This patch defers the whole task of preparing and cleaning up after a >> displaced step to the gdbarch. Two new main gdbarch methods are added, >> with the following sematics: > > "sematics" -> "semantics" Fixed. >> >> - gdbarch_displaced_step_prepare: Prepare for the given thread to >> execute a displaced step of the instruction located at its current PC. >> Upon return, everything should be ready for GDB to resume the thread >> (with either a single step or continue, as indicated by >> gdbarch_displaced_step_hw_singlestep) to make it displaced step the >> instruction. >> >> - gdbarch_displaced_step_finish: Called when the thread stopped after >> having started a displaced step. Verify if the instruction was >> executed, if so apply any fixup required to compensate for the fact >> that the instruction was executed at different place than its original > > "at a different place" Fixed. >> pc. Release any resources that was allocated for this displaced step. > > "that were allocated" Fixed. > >> Upon return, everything should be ready for GDB to resume the >> thread in its "normal" code path. >> >> The displaced_step_prepare_throw function now pretty much just offloads >> to gdbarch_displaced_step_prepare and the displaced_step_finish function >> offloads to gdbarch_displaced_step_finish. >> >> The gdbarch_displaced_step_location method is now unnecessary, so is >> removed. Indeed, the core of GDB doesn't know how many displaced step >> buffers there are nor where they are. >> >> To keep the existing behavior for existing architectures, the logic that >> was previously implemented in infrun.c for preparing and finishing a >> displaced step is moved to displaced-stepping.c, to the >> displaced_step_buffer class. Architectures are modified to implement >> the new gdbarch methods using this class. The behavior is not expeicted >> to change. > > "expeicted" -> "expected" Fixed. >> + >> + m_original_pc = regcache_read_pc (regcache); >> + displaced_pc = m_addr; >> + >> + /* Save the original contents of the displaced stepping buffer. */ >> + m_saved_copy.resize (len); >> + >> + int status = target_read_memory (m_addr, m_saved_copy.data (), len); >> + if (status != 0) >> + throw_error (MEMORY_ERROR, >> + _("Error accessing memory address %s (%s) for " >> + "displaced-stepping scratch space."), >> + paddress (arch, m_addr), safe_strerror (status)); >> + >> + displaced_debug_printf ("saved %s: %s", >> + paddress (arch, m_addr), >> + displaced_step_dump_bytes >> + (m_saved_copy.data (), len).c_str ()); >> + >> + m_copy_insn_closure = gdbarch_displaced_step_copy_insn (arch, >> + m_original_pc, >> + m_addr, >> + regcache); >> + if (m_copy_insn_closure == nullptr) >> + { >> + /* The architecture doesn't know how or want to displaced step >> + this instruction or instruction sequence. Fallback to >> + stepping over the breakpoint in-line. */ >> + return DISPLACED_STEP_PREPARE_STATUS_ERROR; >> + } >> + >> + try >> + { >> + /* Resume execution at the copy. */ >> + regcache_write_pc (regcache, m_addr); >> + } >> + catch (...) > > I'd rather we don't use 'catch (...)' throughout gdb. It swallows > too much. We should catch gdb_exception_error in most cases or > gdb_exception when we also want to catch Ctrl-C/QUIT. If something > throws something else, I'd rather not swallow it, since it's most > probably a bug. > > I'm a bit confused on error handling here. Before the patch, we used > displaced_step_reset_cleanup, but didn't swallow the error. After the patch, > the exception is swallowed and DISPLACED_STEP_PREPARE_STATUS_ERROR is > returned. Was that on purpose? Hmm, I probably did this to ensure that any error would be reported by returning a status code, rather than some errors returning _ERROR and some errors throwing an exception. So just for consistency. Would it make sense to remove the _ERROR status code and report all errors using exceptions then? >> +/* Manage access to a single displaced stepping buffer. */ >> + >> +struct displaced_step_buffer >> +{ >> + displaced_step_buffer (CORE_ADDR buffer_addr) > > explicit. Fixed. >> + : m_addr (buffer_addr) >> + {} >> + >> + displaced_step_prepare_status prepare (thread_info *thread, >> + CORE_ADDR &displaced_pc); >> + >> + displaced_step_finish_status finish (gdbarch *arch, thread_info *thread, >> + gdb_signal sig); >> + >> + const displaced_step_copy_insn_closure * >> + copy_insn_closure_by_addr (CORE_ADDR addr); >> >> - /* If this is not nullptr, this is the thread carrying out a >> - displaced single-step in process PID. This thread's state will >> - require fixing up once it has completed its step. */ >> - thread_info *step_thread; >> + void restore_in_ptid (ptid_t ptid); >> >> - /* The architecture the thread had when we stepped it. */ >> - gdbarch *step_gdbarch; >> +private: >> >> - /* The closure provided gdbarch_displaced_step_copy_insn, to be used >> - for post-step cleanup. */ >> - displaced_step_copy_insn_closure_up step_closure; >> + CORE_ADDR m_original_pc = 0; >> + const CORE_ADDR m_addr; >> >> - /* The address of the original instruction, and the copy we >> - made. */ >> - CORE_ADDR step_original, step_copy; >> + /* If set, the thread currently using the buffer. */ >> + thread_info *m_current_thread = nullptr; >> >> - /* Saved contents of copy area. */ >> - gdb::byte_vector step_saved_copy; >> + gdb::byte_vector m_saved_copy; >> + displaced_step_copy_insn_closure_up m_copy_insn_closure; > > Missing comments? Fixed. >> @@ -1554,9 +1549,9 @@ show_can_use_displaced_stepping (struct ui_file *file, int from_tty, >> static bool >> gdbarch_supports_displaced_stepping (gdbarch *arch) >> { >> - /* Only check for the presence of step_copy_insn. Other required methods >> - are checked by the gdbarch validation. */ >> - return gdbarch_displaced_step_copy_insn_p (arch); >> + /* Only check for the presence of `prepare`. `finish` is required by the >> + gdbarch verification to be provided if `prepare` is provided. */ > > This reads a little funny to me. I'd suggest: > > /* Only check for the presence of `prepare`. The gdbarch verification ensures > that if `prepare` is provided, so is `finish`. */ Fixed to use that. >> + return gdbarch_displaced_step_prepare_p (arch); >> } >> >> /* Return non-zero if displaced stepping can/should be used to step > >> @@ -1669,96 +1662,52 @@ displaced_step_prepare_throw (thread_info *tp) >> jump/branch). */ >> tp->control.may_range_step = 0; >> >> - /* We have to displaced step one thread at a time, as we only have >> - access to a single scratch space per inferior. */ >> - >> - displaced_step_inferior_state *displaced >> - = get_displaced_stepping_state (tp->inf); >> - >> - if (displaced->step_thread != nullptr) >> - { >> - /* Already waiting for a displaced step to finish. Defer this >> - request and place in queue. */ >> - >> - displaced_debug_printf ("deferring step of %s", >> - target_pid_to_str (tp->ptid).c_str ()); >> - >> - global_thread_step_over_chain_enqueue (tp); >> - return DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE; >> - } >> - else >> - displaced_debug_printf ("stepping %s now", >> - target_pid_to_str (tp->ptid).c_str ()); >> - >> - displaced_step_reset (displaced); >> + /* We are about to start a displaced step for this thread. If one is already >> + in progress, something's wrong.. */ > > Double period. Fixed. >> + gdb_assert (!disp_step_thread_state->in_progress ()); >> >> scoped_restore_current_thread restore_thread; >> >> switch_to_thread (tp); >> >> - original = regcache_read_pc (regcache); >> + CORE_ADDR original_pc = regcache_read_pc (regcache); >> + CORE_ADDR displaced_pc; >> >> - copy = gdbarch_displaced_step_location (gdbarch); >> - len = gdbarch_max_insn_length (gdbarch); >> + displaced_step_prepare_status status = >> + gdbarch_displaced_step_prepare (gdbarch, tp, displaced_pc); > > " = " on the next line. Fixed. >> @@ -1934,14 +1831,22 @@ static step_over_what thread_still_needs_step_over (struct thread_info *tp); >> static bool >> start_step_over (void) >> { >> - struct thread_info *tp, *next; >> + thread_info *next; >> + bool started = false; >> >> /* Don't start a new step-over if we already have an in-line >> step-over operation ongoing. */ >> if (step_over_info_valid_p ()) >> - return false; >> + return started; > > I'd move the "bool started = false;" below this if line and continue > writing explicit "return false" here, as it's less indirection. Done. > >> + >> + /* Steal the global thread step over chain. */ > > It would be good expand the command explaining _why_ steal. How about: /* Steal the global thread step over chain. As we try to initiate displaced steps, threads will be enqueued in the global chain if no buffers are available. If we iterated on the global chain directly, we might iterate indefinitely. */ >> @@ -2033,7 +1945,30 @@ start_step_over (void) >> displaced step on a thread of other process. */ >> } >> >> - return false; >> + /* If there are threads left in the THREADS_TO_STEP list, but we have >> + detected that we can't start anything more, put back these threads >> + in the global list. */ > > Do we also need to do this if an exception happens to escape the function? > We might end up pretty bonkers anyhow if that happens, though... Yes, I realized that when playing with this code again yesterday. A scope_exit would help for this I think. But I'm also wondering if we should enqueue back the thread TP that was dequeued, that caused an error. This one has already been dequeued from THREADS_TO_STEP. For example, if thread_still_needs_step_over throws (it reads the PC, so that could happen), should we put back the thread in the global chain? If we do, and the same error happens over and over, the thread will never leave the step over chain and be the first in line, preventing any other displaced step to happen. If, on the other hand, the error was just an intermittent one and we don't put it back in the queue, the next time we'll resume the thread we'll realize it needs a step over and enqueue it again. So it sounds less risky to me to just not enqueue back the thread on error. I'm really not sure, I find it difficult to reason about all the possible cases. Also, re-reading that code, I noticed that we only return "true" when an inline step over is started, or if a displaced step on an all-stop target was started. That's the case even before the patch. I suppose that's on purpose, because the caller wants to know whether it can resume more stuff or not? If so, I think we should update start_step_over's documentation: /* Are there any pending step-over requests? If so, run all we can now and return true. Otherwise, return false. */ >> + if (threads_to_step == NULL) >> + infrun_debug_printf ("step-over queue now empty"); >> + else >> + { >> + infrun_debug_printf ("putting back %d threads to step in global queue", >> + thread_step_over_chain_length (threads_to_step)); >> + >> + while (threads_to_step != nullptr) >> + { >> + thread_info *thread = threads_to_step; >> + >> + /* Remove from that list. */ >> + thread_step_over_chain_remove (&threads_to_step, thread); >> + >> + /* Add to global list. */ >> + global_thread_step_over_chain_enqueue (thread); >> + > > Spurious empty line. But, did you look into splicing the whole threads_to_step > chain into the global chain in O(1), with just some prev/next pointer > adjustments? Fixed that in an experimental patch for now, but it would make sense to do so, so I'll probably end up folding it into this one. >> + } >> + } >> + >> + return started; >> } >> >> /* Update global variables holding ptids to hold NEW_PTID if they were >> @@ -3618,18 +3553,16 @@ prepare_for_detach (void) >> struct inferior *inf = current_inferior (); >> ptid_t pid_ptid = ptid_t (inf->pid); >> >> - displaced_step_inferior_state *displaced = get_displaced_stepping_state (inf); >> - >> /* Is any thread of this process displaced stepping? If not, >> there's nothing else to do. */ >> - if (displaced->step_thread == nullptr) >> + if (displaced_step_in_progress (inf)) >> return; >> >> infrun_debug_printf ("displaced-stepping in-process while detaching"); >> >> scoped_restore restore_detaching = make_scoped_restore (&inf->detaching, true); >> >> - while (displaced->step_thread != nullptr) >> + while (displaced_step_in_progress (inf)) >> { >> struct execution_control_state ecss; >> struct execution_control_state *ecs; >> @@ -5293,25 +5226,23 @@ handle_inferior_event (struct execution_control_state *ecs) >> { >> struct regcache *regcache = get_thread_regcache (ecs->event_thread); >> struct gdbarch *gdbarch = regcache->arch (); >> + inferior *parent_inf = find_inferior_ptid (ecs->target, ecs->ptid); >> + >> + if (ecs->ws.kind == TARGET_WAITKIND_FORKED) >> + { >> + /* Restore in the child process any displaced stepping buffers that >> + were in use at the time of the fork. */ >> + gdbarch_displaced_step_restore_all_in_ptid >> + (gdbarch, parent_inf, ecs->ws.value.related_pid); >> + } > > Why was this moved out of the displaced_step_in_progress_thread check > below? If: 1. thread 1 is doing a displaced step 2. thread 2 does a fork, not letting time for thread 1 to complete event_thread is thread 2, and it is not doing a displaced step, so we don't enter the if (displaced_step_in_progress_thread (ecs->event_thread)) But we still want to restore the bytes in the child used as the displaced stepping buffer for thread 1. Does that make sense? Is it possible that the current code is not correct in that regard? >> @@ -2530,6 +2533,61 @@ linux_displaced_step_location (struct gdbarch *gdbarch) >> return addr; >> } >> >> +/* Implementation gdbarch_displaced_step_prepare. */ > > "Implementation of" ? Changed for /* See linux-tdep.h. */ > >> + >> +displaced_step_prepare_status >> +linux_displaced_step_prepare (gdbarch *arch, thread_info *thread, >> + CORE_ADDR &displaced_pc) >> +{ >> + linux_info *per_inferior = get_linux_inferior_data (thread->inf); >> + >> + if (!per_inferior->disp_step_buf.has_value ()) >> + { >> + CORE_ADDR disp_step_buf_addr >> + = linux_displaced_step_location (thread->inf->gdbarch); >> + >> + per_inferior->disp_step_buf.emplace (disp_step_buf_addr); >> + } >> + >> + return per_inferior->disp_step_buf->prepare (thread, displaced_pc); >> +} >> + >> +/* Implementation gdbarch_displaced_step_finish. */ > > Ditto. Changed for "See...". > >> + >> +displaced_step_finish_status >> +linux_displaced_step_finish (gdbarch *arch, thread_info *thread, gdb_signal sig) >> +{ >> + linux_info *per_inferior = get_linux_inferior_data (thread->inf); >> + >> + gdb_assert (per_inferior->disp_step_buf.has_value ()); >> + >> + return per_inferior->disp_step_buf->finish (arch, thread, sig); >> +} >> + >> +const displaced_step_copy_insn_closure * >> +linux_displaced_step_copy_insn_closure_by_addr (inferior *inf, CORE_ADDR addr) > > Ditto on the immaginary comment. :-) Added "See...". > >> +{ >> + linux_info *per_inferior = linux_inferior_data.get (inf); >> + >> + if (per_inferior == nullptr >> + || !per_inferior->disp_step_buf.has_value ()) >> + return nullptr; >> + >> + return per_inferior->disp_step_buf->copy_insn_closure_by_addr (addr); >> +} >> + >> +void >> +linux_displaced_step_restore_all_in_ptid (inferior *parent_inf, ptid_t ptid) >> +{ >> + linux_info *per_inferior = linux_inferior_data.get (parent_inf); >> + Same here. >> /* See gdbthread.h. */ >> @@ -403,9 +411,32 @@ thread_is_in_step_over_chain (struct thread_info *tp) >> >> /* See gdbthread.h. */ >> >> +int >> +thread_step_over_chain_length (thread_info *tp) >> +{ >> + if (tp == nullptr) >> + return 0; >> + >> + int num = 1; > > Should we add: > > gdb_assert (thread_is_in_step_over_chain (tp)); > > ? > > But then again, it's a bit odd to allow tp == nullptr, > but not allow tp->step_over_next == nullptr? Well, a pointer to an empty thread step over chain is a nullptr: when the global chain is empty, global_thread_step_over_chain is nullptr. Same for threads_to_step that is local to start_step_over. So it makes sense that we can do thread_step_over_chain_length(nullptr). However, it would be a mistake to pass a non-nullptr pointer to a thread that is not in a step over chain, so I think it would be a good idea to add the assert you propose. I will clarify the documentation of the function: /* Return the length of the the step over chain TP is in. If TP is non-nullptr, the thread must be in a step over chain. TP may be nullptr, in which case it denotes an empty list, so a length of 0. */ >> + thread_info *iter = tp->step_over_next; >> + >> + while (iter != tp) >> + { >> + num++; >> + iter = iter->step_over_next; >> + } > > This seems to be begging for a "for": > > int num = 1; > for (thread_info *iter = tp->step_over_next; > iter != tp; > iter = iter->step_over_next) > num++; True, changed for: for (thread_info *iter = tp->step_over_next; iter != tp; iter->step_over_next) ++num; Simon