From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by sourceware.org (Postfix) with ESMTPS id 97D3E386EC6E for ; Wed, 25 Nov 2020 01:41:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 97D3E386EC6E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wm1-f68.google.com with SMTP id p22so635153wmg.3 for ; Tue, 24 Nov 2020 17:41:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=gabNvm6IT7yrofbzihUG4StDWPTwvVzLX5GKI5ox+YY=; b=qk03YhTloF20irlWUcki4OVEaX2HKDT1SUDW9hEo6o41hvwjBdyt9J64LRj7mMjg7t lVXEatnH1M+qTIgy58apy+8SlnmZdq9S+SDKsxQh0MxHZikHWDf5PobojtWavDA5Oxj5 UDzPFMcpGacjLuFKd6jMlR1IX6VzdRyRnW0i2EtOzCob8vebgKZhuykzzcLoUr5LuXi4 ntvdWric7IKm0rflMbRBhTfBm8pezXK/ZV5syorVQfqj0cqJrj6pDJs1qY7UzXWa9cOx 1a0Jz+7aIRrCTT9tWyAQSK4DjAMV7UXuKFNeuGQ2MOpErqNeGOz7ZTtjRVTJhHqT7y57 jK9g== X-Gm-Message-State: AOAM533qspy28fCO+8/5j5JG1PbdQpSLzzmfwcYIsQUdudZPbfHsrQZi QVoxR3T01fEVPqD6NdWkK7QoaKfdUsqXGg== X-Google-Smtp-Source: ABdhPJzylAC+7t3O5CD/lyTuZJvc5A/oNDiVvA1biPaM3RCw9Ye8Tkxs2D9HHTpytbJIvnn5A13ylQ== X-Received: by 2002:a1c:2008:: with SMTP id g8mr1118004wmg.19.1606268459661; Tue, 24 Nov 2020 17:40:59 -0800 (PST) Received: from ?IPv6:2001:8a0:f91f:e900:1d90:d745:3c32:c159? ([2001:8a0:f91f:e900:1d90:d745:3c32:c159]) by smtp.gmail.com with ESMTPSA id a9sm1359626wrp.21.2020.11.24.17.40.58 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 Nov 2020 17:40:58 -0800 (PST) Subject: Re: [PATCH 09/12] gdb: move displaced stepping logic to gdbarch, allow starting concurrent displaced steps To: Simon Marchi , gdb-patches@sourceware.org References: <20201110214614.2842615-1-simon.marchi@efficios.com> <20201110214614.2842615-10-simon.marchi@efficios.com> From: Pedro Alves Message-ID: <72a75576-a54b-bcc8-e8d3-5a57571fe234@palves.net> Date: Wed, 25 Nov 2020 01:40:57 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20201110214614.2842615-10-simon.marchi@efficios.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.4 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Wed, 25 Nov 2020 01:41:04 -0000 Hi, So overall this is looking pretty good. A few comments below. Please also see the comments in response to patch #12. This patch causes a surprising slowdown. 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? > > - 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" > > - 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" > pc. Release any resources that was allocated for this displaced step. "that were allocated" > 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" > @@ -37,6 +43,188 @@ show_debug_displaced (struct ui_file *file, int from_tty, > fprintf_filtered (file, _("Displace stepping debugging is %s.\n"), value); > } > > +displaced_step_prepare_status > +displaced_step_buffer::prepare (thread_info *thread, CORE_ADDR &displaced_pc) > +{ ... > + > + 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? > + { > + /* Failed to write the PC. Release the architecture's displaced > + stepping resources and the thread's displaced stepping state. */ > + m_copy_insn_closure.reset (); > + > + return DISPLACED_STEP_PREPARE_STATUS_ERROR; > + } > + > + /* This marks the buffer as being in use. */ > + m_current_thread = thread; > + > + return DISPLACED_STEP_PREPARE_STATUS_OK; > +} > > @@ -94,37 +99,78 @@ struct displaced_step_inferior_state > /* Put this object back in its original state. */ > void reset () > { > - failed_before = 0; > - step_thread = nullptr; > - step_gdbarch = nullptr; > - step_closure.reset (); > - step_original = 0; > - step_copy = 0; > - step_saved_copy.clear (); > + failed_before = false; > } > > /* True if preparing a displaced step ever failed. If so, we won't > try displaced stepping for this inferior again. */ > - int failed_before; > + bool failed_before; > +}; > + > +/* Per-thread displaced stepping state. */ > + > +struct displaced_step_thread_state > +{ > + /* Return true if this thread is currently executing a displaced step. */ > + bool in_progress () const > + { > + return m_original_gdbarch != nullptr; > + } > + > + /* Return the gdbarch of the thread prior to the step. */ > + gdbarch *get_original_gdbarch () const > + { > + return m_original_gdbarch; > + } > + > + /* Mark this thread as currently executing a displaced step. > + > + ORIGINAL_GDBARCH is the current gdbarch of the thread (before the step > + is executed). */ > + void set (gdbarch *original_gdbarch) > + { > + m_original_gdbarch = original_gdbarch; > + } > + > + /* Mark this thread as no longer executing a displaced step. */ > + void reset () > + { > + m_original_gdbarch = nullptr; > + } > + > +private: > + gdbarch *m_original_gdbarch = nullptr; > +}; > + > +/* Manage access to a single displaced stepping buffer. */ > + > +struct displaced_step_buffer > +{ > + displaced_step_buffer (CORE_ADDR buffer_addr) explicit. > + : 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? > }; > > #endif /* DISPLACED_STEPPING_H */ > @@ -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`. */ > + 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. > + 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. > > - if (breakpoint_in_range_p (aspace, copy, len)) > + if (status == DISPLACED_STEP_PREPARE_STATUS_ERROR) > @@ -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. > + > + /* Steal the global thread step over chain. */ It would be good expand the command explaining _why_ steal. > + thread_info *threads_to_step = global_thread_step_over_chain_head; > + global_thread_step_over_chain_head = NULL; > > - for (tp = global_thread_step_over_chain_head; tp != NULL; tp = next) > + infrun_debug_printf ("stealing global queue of threads to step, length = %d", > + thread_step_over_chain_length (threads_to_step)); > + > + for (thread_info *tp = threads_to_step; tp != NULL; tp = next) > { > struct execution_control_state ecss; > struct execution_control_state *ecs = &ecss; > @@ -2005,13 +1902,27 @@ start_step_over (void) > if (!ecs->wait_some_more) > error (_("Command aborted.")); > > - gdb_assert (tp->resumed); > + /* If the thread's step over could not be initiated, it was re-added > + to the global step over chain. */ > + if (tp->resumed) > + { > + infrun_debug_printf ("[%s] was resumed.", > + target_pid_to_str (tp->ptid).c_str ()); > + gdb_assert (!thread_is_in_step_over_chain (tp)); > + } > + else > + { > + infrun_debug_printf ("[%s] was NOT resumed.", > + target_pid_to_str (tp->ptid).c_str ()); > + gdb_assert (thread_is_in_step_over_chain (tp)); > + } > > /* If we started a new in-line step-over, we're done. */ > if (step_over_info_valid_p ()) > { > gdb_assert (tp->control.trap_expected); > - return true; > + started = true; > + break; > } > > if (!target_is_non_stop_p ()) > @@ -2024,7 +1935,8 @@ start_step_over (void) > /* With remote targets (at least), in all-stop, we can't > issue any further remote commands until the program stops > again. */ > - return true; > + started = true; > + break; > } > > /* Either the thread no longer needed a step-over, or a new > @@ -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... > + 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? > + } > + } > + > + 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 checking displaced stepping is supported, and thread > ecs->ptid is displaced stepping. */ > if (displaced_step_in_progress_thread (ecs->event_thread)) > { > - struct inferior *parent_inf > - = find_inferior_ptid (ecs->target, ecs->ptid); > struct regcache *child_regcache; > CORE_ADDR parent_pc; > > - if (ecs->ws.kind == TARGET_WAITKIND_FORKED) > - { > - struct displaced_step_inferior_state *displaced > - = get_displaced_stepping_state (parent_inf); > - > - /* Restore scratch pad for child process. */ > - displaced_step_restore (displaced, ecs->ws.value.related_pid); > - } > - > /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED, > indicating that the displaced stepping of syscall instruction > has been done. Perform cleanup for parent process here. Note > diff --git a/gdb/infrun.h b/gdb/infrun.h > index c83cb333083..d5e6d279f1a 100644 > --- a/gdb/infrun.h > +++ b/gdb/infrun.h > @@ -226,9 +226,6 @@ extern void clear_exit_convenience_vars (void); > /* Dump LEN bytes at BUF in hex to a string and return it. */ > extern std::string displaced_step_dump_bytes (const gdb_byte *buf, size_t len); > > -extern struct displaced_step_copy_insn_closure * > - get_displaced_step_copy_insn_closure_by_addr (CORE_ADDR addr); > - > extern void update_observer_mode (void); > > extern void signal_catch_update (const unsigned int *); > diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c > index c57181765e0..f3d1ccb4d1c 100644 > --- a/gdb/linux-tdep.c > +++ b/gdb/linux-tdep.c > @@ -199,6 +199,9 @@ struct linux_info > yet. Positive if we tried looking it up, and found it. Negative > if we tried looking it up but failed. */ > int vsyscall_range_p = 0; > + > + /* Inferior's displaced step buffer. */ > + gdb::optional disp_step_buf; > }; > > /* Per-inferior data key. */ > @@ -2530,6 +2533,61 @@ linux_displaced_step_location (struct gdbarch *gdbarch) > return addr; > } > > +/* Implementation gdbarch_displaced_step_prepare. */ "Implementation of" ? > + > +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. > + > +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. :-) > +{ > + 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); > + ... > /* 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? > + 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++; > + > + return num; > +}