From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x342.google.com (mail-wm1-x342.google.com [IPv6:2a00:1450:4864:20::342]) by sourceware.org (Postfix) with ESMTPS id E01873858D37 for ; Wed, 11 Nov 2020 23:36:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E01873858D37 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x342.google.com with SMTP id s13so3844452wmh.4 for ; Wed, 11 Nov 2020 15:36:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=7GTqkqIiMr2b09k20JzQu6OqAc9DNP1r8ejaX+N1qMo=; b=AImeQ20W8xbdX73HUKEyrUpvDYhNNCNCxeuMk0R0siTclpqLeXNlXXq/DyZumqBZmV +Fkp6lzs3Sw3Sg9ysnLL9SPPdJt8tUHcswIvY8hgXH/104kJxMir6ycvyxZbHzfrc7E5 gn3OKHXh5Nsh8eVu3PHt6iR+YENP4oz+ebmdykFwKGfdobPfgfzhS6MGrxs9O6EshDhM h2cVl3WFgfvQP/uI43Hxytm0TvKt2QQXeUsRyNZIleFOR54ZTmFOLZMZDAkUM2yhovEG zRU7Bjnu+8yuFvQ/9zYdrjUqaoMez2PClSBE38rt9erRQAmdBACV+FemBE6tYC3oOXdy BNJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=7GTqkqIiMr2b09k20JzQu6OqAc9DNP1r8ejaX+N1qMo=; b=fbLa/3qDvW5/XYQKcXEp5LoNrN9hsfkKztU4VLQmig8szvYaO/UKVOe2yTyUSQmTh4 QQ+qNlOQ+ebBXMcKpIHdYTQfmXVmwNhCg44r6FDVD+C+AFe9S9/2S0CcWWgLyhoubXHP gBGFU4alGxQEYylKlG4hqT185kkUBxX8CCAmOYb2sJZG/7PNC+Lut32y5H2MOi/MIXma MXlpUz8eB8fUW3BGZaSC2LQcCLL1jRL0y4Mn4ZcreDHnVIN5CWMTyxyrJCGW7bf+oIHb OiUtZivjuY7jcJaqLGnbgxtTXzWMQAhSCKziIodQfB9IcUZYIOF07wyqyv5R3uaQKeh8 tRdQ== X-Gm-Message-State: AOAM533PEKV7Xa7szhzRhyKedK0yLuqM3unHLbq8bWHtxONhkJyh7Iat 6dWdnbMUwSUfDi+oBtrZByg74jRJUZXLlw== X-Google-Smtp-Source: ABdhPJz2sHqNPNFZ/Shxosbq4I/1thyKOoaYiCgqTmzrutbaAopEA8LjoyjGJ0uHmEH/a+5p4aZ9PA== X-Received: by 2002:a7b:c309:: with SMTP id k9mr6440166wmj.14.1605137787764; Wed, 11 Nov 2020 15:36:27 -0800 (PST) Received: from localhost (host212-140-123-128.range212-140.btcentralplus.com. [212.140.123.128]) by smtp.gmail.com with ESMTPSA id x63sm4534164wmb.48.2020.11.11.15.36.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Nov 2020 15:36:26 -0800 (PST) Date: Wed, 11 Nov 2020 23:36:25 +0000 From: Andrew Burgess To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 06/12] gdb: introduce status enum for displaced step prepare/finish Message-ID: <20201111233625.GP2729@embecosm.com> References: <20201110214614.2842615-1-simon.marchi@efficios.com> <20201110214614.2842615-7-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201110214614.2842615-7-simon.marchi@efficios.com> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 23:33:55 up 17 days, 14:37, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, 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, 11 Nov 2020 23:36:31 -0000 I looked though all the patches up to and including this one. I don't have too much knowledge of the displaced stepping mechanism, but so far everything seemed good. Thanks, Andrew * Simon Marchi via Gdb-patches [2020-11-10 16:46:08 -0500]: > This is a preparatory patch to reduce the size of the diff of the > upcoming main patch. It introduces enum types for the return values of > displaced step "prepare" and "finish" operations. I find that this > expresses better the intention of the code, rather than returning > arbitrary integer values (-1, 0 and 1) which are difficult to remember. > That makes the code easier to read. > > I put the new enum types in a new displaced-stepping.h file, because I > introduce that file in a later patch anyway. Putting it there avoids > having to move it later. > > There is one change in behavior for displaced_step_finish: it currently > returns 0 if the thread wasn't doing a displaced step and 1 if the > thread was doing a displaced step which was executed successfully. It > turns out that this distinction is not needed by any caller, so I've > merged these two cases into "_OK", rather than adding an extra > enumerator. > > gdb/ChangeLog: > > * infrun.c (displaced_step_prepare_throw): Change return type to > displaced_step_prepare_status. > (displaced_step_prepare): Likewise. > (displaced_step_finish): Change return type to > displaced_step_finish_status. > (resume_1): Adjust. > (stop_all_threads): Adjust. > * displaced-stepping.h: New file. > > Change-Id: I5c8fe07212cd398d5b486b5936d9d0807acd3788 > --- > gdb/displaced-stepping.h | 46 +++++++++++++++++++++++++ > gdb/infrun.c | 72 +++++++++++++++++++++++----------------- > 2 files changed, 88 insertions(+), 30 deletions(-) > create mode 100644 gdb/displaced-stepping.h > > diff --git a/gdb/displaced-stepping.h b/gdb/displaced-stepping.h > new file mode 100644 > index 00000000000..9c7e85c3769 > --- /dev/null > +++ b/gdb/displaced-stepping.h > @@ -0,0 +1,46 @@ > +/* Displaced stepping related things. > + > + Copyright (C) 2020 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#ifndef DISPLACED_STEPPING_H > +#define DISPLACED_STEPPING_H > + > +enum displaced_step_prepare_status > +{ > + /* A displaced stepping buffer was successfully allocated and prepared. */ > + DISPLACED_STEP_PREPARE_STATUS_OK, > + > + /* Something bad happened. */ > + DISPLACED_STEP_PREPARE_STATUS_ERROR, > + > + /* Not enough resources are available at this time, try again later. */ > + DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE, > +}; > + > +enum displaced_step_finish_status > +{ > + /* Either the instruction was stepped and fixed up, or the specified thread > + wasn't executing a displaced step (in which case there's nothing to > + finish). */ > + DISPLACED_STEP_FINISH_STATUS_OK, > + > + /* The thread was executing a displaced step, but didn't complete it. */ > + DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED, > +}; > + > +#endif /* DISPLACED_STEPPING_H */ > diff --git a/gdb/infrun.c b/gdb/infrun.c > index cdf198bb307..ed54c4331d7 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -19,6 +19,7 @@ > along with this program. If not, see . */ > > #include "defs.h" > +#include "displaced-stepping.h" > #include "infrun.h" > #include > #include "symtab.h" > @@ -1652,11 +1653,13 @@ displaced_step_dump_bytes (const gdb_byte *buf, size_t len) > Comments in the code for 'random signals' in handle_inferior_event > explain how we handle this case instead. > > - Returns 1 if preparing was successful -- this thread is going to be > - stepped now; 0 if displaced stepping this thread got queued; or -1 > - if this instruction can't be displaced stepped. */ > + Returns DISPLACED_STEP_PREPARE_STATUS_OK if preparing was successful -- this > + thread is going to be stepped now; DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE > + if displaced stepping this thread got queued; or > + DISPLACED_STEP_PREPARE_STATUS_ERROR if this instruction can't be displaced > + stepped. */ > > -static int > +static displaced_step_prepare_status > displaced_step_prepare_throw (thread_info *tp) > { > regcache *regcache = get_thread_regcache (tp); > @@ -1694,7 +1697,7 @@ displaced_step_prepare_throw (thread_info *tp) > target_pid_to_str (tp->ptid).c_str ()); > > global_thread_step_over_chain_enqueue (tp); > - return 0; > + return DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE; > } > else > displaced_debug_printf ("stepping %s now", > @@ -1725,7 +1728,7 @@ displaced_step_prepare_throw (thread_info *tp) > displaced_debug_printf ("breakpoint set in scratch pad. " > "Stepping over breakpoint in-line instead."); > > - return -1; > + return DISPLACED_STEP_PREPARE_STATUS_ERROR; > } > > /* Save the original contents of the copy area. */ > @@ -1749,7 +1752,7 @@ displaced_step_prepare_throw (thread_info *tp) > /* 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 -1; > + return DISPLACED_STEP_PREPARE_STATUS_ERROR; > } > > /* Save the information we need to fix things up if the step > @@ -1770,20 +1773,21 @@ displaced_step_prepare_throw (thread_info *tp) > > displaced_debug_printf ("displaced pc to %s", paddress (gdbarch, copy)); > > - return 1; > + return DISPLACED_STEP_PREPARE_STATUS_OK; > } > > /* Wrapper for displaced_step_prepare_throw that disabled further > attempts at displaced stepping if we get a memory error. */ > > -static int > +static displaced_step_prepare_status > displaced_step_prepare (thread_info *thread) > { > - int prepared = -1; > + displaced_step_prepare_status status > + = DISPLACED_STEP_PREPARE_STATUS_ERROR; > > try > { > - prepared = displaced_step_prepare_throw (thread); > + status = displaced_step_prepare_throw (thread); > } > catch (const gdb_exception_error &ex) > { > @@ -1810,7 +1814,7 @@ displaced_step_prepare (thread_info *thread) > displaced_state->failed_before = 1; > } > > - return prepared; > + return status; > } > > static void > @@ -1840,22 +1844,24 @@ displaced_step_restore (struct displaced_step_inferior_state *displaced, > displaced->step_copy)); > } > > -/* If we displaced stepped an instruction successfully, adjust > - registers and memory to yield the same effect the instruction would > - have had if we had executed it at its original address, and return > - 1. If the instruction didn't complete, relocate the PC and return > - -1. If the thread wasn't displaced stepping, return 0. */ > +/* If we displaced stepped an instruction successfully, adjust registers and > + memory to yield the same effect the instruction would have had if we had > + executed it at its original address, and return > + DISPLACED_STEP_PREPARE_STATUS_OK. If the instruction didn't complete, > + relocate the PC and return DISPLACED_STEP_PREPARE_STATUS_NOT_EXECUTED. > > -static int > + If the thread wasn't displaced stepping, return > + DISPLACED_STEP_PREPARE_STATUS_OK as well.. */ > + > +static displaced_step_finish_status > displaced_step_finish (thread_info *event_thread, enum gdb_signal signal) > { > struct displaced_step_inferior_state *displaced > = get_displaced_stepping_state (event_thread->inf); > - int ret; > > /* Was this event for the thread we displaced? */ > if (displaced->step_thread != event_thread) > - return 0; > + return DISPLACED_STEP_FINISH_STATUS_OK; > > /* Fixup may need to read memory/registers. Switch to the thread > that we're fixing up. Also, target_stopped_by_watchpoint checks > @@ -1879,7 +1885,8 @@ displaced_step_finish (thread_info *event_thread, enum gdb_signal signal) > displaced->step_original, > displaced->step_copy, > get_thread_regcache (displaced->step_thread)); > - ret = 1; > + > + return DISPLACED_STEP_FINISH_STATUS_OK; > } > else > { > @@ -1890,10 +1897,9 @@ displaced_step_finish (thread_info *event_thread, enum gdb_signal signal) > > pc = displaced->step_original + (pc - displaced->step_copy); > regcache_write_pc (regcache, pc); > - ret = -1; > - } > > - return ret; > + return DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED; > + } > } > > /* Data to be passed around while handling an event. This data is > @@ -2417,16 +2423,17 @@ resume_1 (enum gdb_signal sig) > && sig == GDB_SIGNAL_0 > && !current_inferior ()->waiting_for_vfork_done) > { > - int prepared = displaced_step_prepare (tp); > + displaced_step_prepare_status prepare_status > + = displaced_step_prepare (tp); > > - if (prepared == 0) > + if (prepare_status == DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE) > { > infrun_debug_printf ("Got placed in step-over queue"); > > tp->control.trap_expected = 0; > return; > } > - else if (prepared < 0) > + else if (prepare_status == DISPLACED_STEP_PREPARE_STATUS_ERROR) > { > /* Fallback to stepping over the breakpoint in-line. */ > > @@ -2440,7 +2447,7 @@ resume_1 (enum gdb_signal sig) > > insert_breakpoints (); > } > - else if (prepared > 0) > + else if (prepare_status == DISPLACED_STEP_PREPARE_STATUS_OK) > { > /* Update pc to reflect the new address from which we will > execute instructions due to displaced stepping. */ > @@ -2448,6 +2455,9 @@ resume_1 (enum gdb_signal sig) > > step = gdbarch_displaced_step_hw_singlestep (gdbarch); > } > + else > + gdb_assert_not_reached (_("Invalid displaced_step_prepare_status " > + "value.")); > } > > /* Do we need to do it the hard way, w/temp breakpoints? */ > @@ -4822,7 +4832,8 @@ stop_all_threads (void) > t->suspend.waitstatus.kind = TARGET_WAITKIND_IGNORE; > t->suspend.waitstatus_pending_p = 0; > > - if (displaced_step_finish (t, GDB_SIGNAL_0) < 0) > + if (displaced_step_finish (t, GDB_SIGNAL_0) > + == DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED) > { > /* Add it back to the step-over queue. */ > infrun_debug_printf > @@ -4850,7 +4861,8 @@ stop_all_threads (void) > sig = (event.ws.kind == TARGET_WAITKIND_STOPPED > ? event.ws.value.sig : GDB_SIGNAL_0); > > - if (displaced_step_finish (t, sig) < 0) > + if (displaced_step_finish (t, sig) > + == DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED) > { > /* Add it back to the step-over queue. */ > t->control.trap_expected = 0; > -- > 2.28.0 >