From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 282D13858D39 for ; Mon, 27 Mar 2023 12:40:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 282D13858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1679920815; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=tVniGvUzOVqgAy3lUr6scXrIgttI47W8qfkFqjqmQg8=; b=IvYq2V8T583MkmQwfB08Fs2qQNtBNgaGZlhg8lm9cVZmX0tl8q1nRGWFxD1ubvTkMQVMei NLxHSj2mCPh7GsF0l/3Bc5i1Muv5wMqNHgKD5osmV9UWcNVXnXcL+RzSjh8olq0QauxxAF dhQtYmcSJzwPy21w6PkHcZxYt4XBVnU= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-572-6qqZIm7NOoCmGuBTXF7pOw-1; Mon, 27 Mar 2023 08:40:14 -0400 X-MC-Unique: 6qqZIm7NOoCmGuBTXF7pOw-1 Received: by mail-wm1-f69.google.com with SMTP id bi7-20020a05600c3d8700b003edecc610abso5664542wmb.7 for ; Mon, 27 Mar 2023 05:40:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679920813; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=tVniGvUzOVqgAy3lUr6scXrIgttI47W8qfkFqjqmQg8=; b=cDRSWRv3c1AoLVHhsew3sCzqSImtuvP9DFfQBO3hsTs8uW+TO4Sy8zn87nB3l0A+Cl pwSgXGlOCdvx90UY83OEvPrQ9kWkqEaxZbEjNc235f/9FLJsFGcRCz367aRde5tqQAis 1Zrvu8f5qaA8xbsBd67JiY8jCajBGDl+gYMpV2YsboKTwxWnZGMwuIUdkaGji0ibF/xf KEJXq5zsRhv9cMGhOPvXrEdE/7VE4e8bGCMyV7+0UwTOdTh34ZVAnL7nEX6EStYZpQVU a0qSDOEktzok4eH/m4bWW4U8+7jQnlNkn8SFDJ8fSt1okwkONnlS1OF1olYtWN2yysjt KGHg== X-Gm-Message-State: AO0yUKVuqHMHuAksxUikJG545BfKh0nyPzXIwkc5DnGf0ik27iBSrZUQ 7gkUeTbmb7rGAVkVXu5Fblgz6dgKP+hFWsGS4lzsjXbDb8oGYb/qWeGrcwjnZy+gtgZSLy6sc9z V9ELc+APAQf9GH3KQBg8Jl3xFUgBirQ== X-Received: by 2002:a1c:ed05:0:b0:3eb:395b:8b62 with SMTP id l5-20020a1ced05000000b003eb395b8b62mr8867867wmh.39.1679920813331; Mon, 27 Mar 2023 05:40:13 -0700 (PDT) X-Google-Smtp-Source: AK7set8HDoAs64EHQX2ebT8QOEnfoycStFqJdWLuEorcEVZ38NAteF3//+0QhtqtB5v2lDv83Zgf0g== X-Received: by 2002:a1c:ed05:0:b0:3eb:395b:8b62 with SMTP id l5-20020a1ced05000000b003eb395b8b62mr8867854wmh.39.1679920812963; Mon, 27 Mar 2023 05:40:12 -0700 (PDT) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id t20-20020a05600c451400b003ee443bf0c7sm8854496wmo.16.2023.03.27.05.40.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Mar 2023 05:40:12 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH 01/31] displaced step: pass down target_waitstatus instead of gdb_signal In-Reply-To: <59e7c829-251d-0318-1101-2f45fb073586@palves.net> References: <20221212203101.1034916-1-pedro@palves.net> <20221212203101.1034916-2-pedro@palves.net> <87fsbnrpss.fsf@redhat.com> <87zg8cel2d.fsf@redhat.com> <877cv8cw5q.fsf@redhat.com> <59e7c829-251d-0318-1101-2f45fb073586@palves.net> Date: Mon, 27 Mar 2023 13:40:11 +0100 Message-ID: <87v8imbc5w.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Pedro Alves writes: > Hi! > > On 2023-03-22 9:29 p.m., Andrew Burgess wrote: >> Andrew Burgess writes: >> > ... >>> I took a look through and I'm happy with it. But I would like you to >>> consider holding off until my displaced step patch has some feedback. >>> >>> I'm currently rebasing the patch. My patch deletes >>> displaced_step_instruction_executed_successfully, which I realised makes >>> passing the signal (or now target_waitstatus) redundant. As such I'm >>> just testing an additional patch in the series which touches every place >>> you're touching - but removes the signal instead of changing it. >>> >>> I hope to post my updated series later today (once testing completes). >> >> Pedro, >> >> Thanks for your feedback on my displaced stepping series. You're right >> that just checking the $pc isn't going to be enough. So I'm now >> thinking that I should be passing the gdb_signal through to the >> gdbarch_displaced_step_fixup function. > > I guess passing down a boolean (or enum) indicating "success/failure" would > also be an alternative. That may avoid every arch having to do the > same check. That's what I did in the end. I posted a V3 for my displaced step series. > >> >> Rather than change things to pass through the gdb_signal though, and >> then have this patch come along and s/gdb_signal/target_waitstatus/, I >> wonder how you'd feel about merging this patch sooner rather than later? >> >> I'm planning to rebase my displaced stepping series off this patch -- I >> just want to check you'd be OK with this patch possibly landing before >> the rest of this series? > > I'm absolutely fine with putting this in before the rest of this series. > > How about I just merge it immediately, so none of us have to carry it > around? That would be great, as our patches will have a minor merge conflict in displaced-stepping.c. > > I've updated the commit log a bit so it doesn't assume it is being merged along > the rest of the series. > > From 7abd5c8ee74055621dc3a73fbcf2e8940712bb01 Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Tue, 22 Jun 2021 15:42:51 +0100 > Subject: [PATCH] displaced step: pass down target_waitstatus instead of > gdb_signal > > This commit tweaks displaced_step_finish & friends to pass down a > target_waitstatus instead of a gdb_signal. This is needed because a > patch later in the step-over-{thread-exit,clone] series will want to > make displaced_step_buffers::finish handle > TARGET_WAITKIND_THREAD_EXITED. It also helps with the > TARGET_WAITKIND_THREAD_CLONED patch later in that same series. > > It's also a bit more logical this way, as we don't have to pass down > signals when the thread didn't actually stop for a signal. So we can > also think of it as a clean up. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338 > Change-Id: I4c5d338647b028071bc498c4e47063795a2db4c0 > Approved-By: Andrew Burgess LGTM. Thanks, Andrew > --- > gdb/displaced-stepping.c | 16 +++++++++++----- > gdb/displaced-stepping.h | 2 +- > gdb/gdbarch-gen.h | 4 ++-- > gdb/gdbarch.c | 4 ++-- > gdb/gdbarch_components.py | 2 +- > gdb/infrun.c | 17 +++++++---------- > gdb/linux-tdep.c | 5 +++-- > gdb/linux-tdep.h | 2 +- > gdb/rs6000-tdep.c | 4 ++-- > 9 files changed, 30 insertions(+), 26 deletions(-) > > diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c > index 9f98ea8c35b..3fefdf322d8 100644 > --- a/gdb/displaced-stepping.c > +++ b/gdb/displaced-stepping.c > @@ -192,12 +192,18 @@ write_memory_ptid (ptid_t ptid, CORE_ADDR memaddr, > } > > static bool > -displaced_step_instruction_executed_successfully (gdbarch *arch, > - gdb_signal signal) > +displaced_step_instruction_executed_successfully > + (gdbarch *arch, const target_waitstatus &status) > { > - if (signal != GDB_SIGNAL_TRAP) > + if (status.kind () == TARGET_WAITKIND_STOPPED > + && status.sig () != GDB_SIGNAL_TRAP) > return false; > > + /* All other (thread event) waitkinds can only happen if the > + instruction fully executed. For example, a fork, or a syscall > + entry can only happen if the syscall instruction actually > + executed. */ > + > if (target_stopped_by_watchpoint ()) > { > if (gdbarch_have_nonsteppable_watchpoint (arch) > @@ -210,7 +216,7 @@ displaced_step_instruction_executed_successfully (gdbarch *arch, > > displaced_step_finish_status > displaced_step_buffers::finish (gdbarch *arch, thread_info *thread, > - gdb_signal sig) > + const target_waitstatus &status) > { > gdb_assert (thread->displaced_step_state.in_progress ()); > > @@ -256,7 +262,7 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread, > regcache *rc = get_thread_regcache (thread); > > bool instruction_executed_successfully > - = displaced_step_instruction_executed_successfully (arch, sig); > + = displaced_step_instruction_executed_successfully (arch, status); > > if (instruction_executed_successfully) > { > diff --git a/gdb/displaced-stepping.h b/gdb/displaced-stepping.h > index e154927ad92..d2c3fc1b2b4 100644 > --- a/gdb/displaced-stepping.h > +++ b/gdb/displaced-stepping.h > @@ -168,7 +168,7 @@ struct displaced_step_buffers > CORE_ADDR &displaced_pc); > > displaced_step_finish_status finish (gdbarch *arch, thread_info *thread, > - gdb_signal sig); > + const target_waitstatus &status); > > const displaced_step_copy_insn_closure * > copy_insn_closure_by_addr (CORE_ADDR addr); > diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h > index a3fc0b9272b..bbf2376fc4b 100644 > --- a/gdb/gdbarch-gen.h > +++ b/gdb/gdbarch-gen.h > @@ -1101,8 +1101,8 @@ extern void set_gdbarch_displaced_step_prepare (struct gdbarch *gdbarch, gdbarch > > /* Clean up after a displaced step of THREAD. */ > > -typedef displaced_step_finish_status (gdbarch_displaced_step_finish_ftype) (struct gdbarch *gdbarch, thread_info *thread, gdb_signal sig); > -extern displaced_step_finish_status gdbarch_displaced_step_finish (struct gdbarch *gdbarch, thread_info *thread, gdb_signal sig); > +typedef displaced_step_finish_status (gdbarch_displaced_step_finish_ftype) (struct gdbarch *gdbarch, thread_info *thread, const target_waitstatus &ws); > +extern displaced_step_finish_status gdbarch_displaced_step_finish (struct gdbarch *gdbarch, thread_info *thread, const target_waitstatus &ws); > extern void set_gdbarch_displaced_step_finish (struct gdbarch *gdbarch, gdbarch_displaced_step_finish_ftype *displaced_step_finish); > > /* Return the closure associated to the displaced step buffer that is at ADDR. */ > diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c > index b676e346fd0..84beb087336 100644 > --- a/gdb/gdbarch.c > +++ b/gdb/gdbarch.c > @@ -4098,13 +4098,13 @@ set_gdbarch_displaced_step_prepare (struct gdbarch *gdbarch, > } > > displaced_step_finish_status > -gdbarch_displaced_step_finish (struct gdbarch *gdbarch, thread_info *thread, gdb_signal sig) > +gdbarch_displaced_step_finish (struct gdbarch *gdbarch, thread_info *thread, const target_waitstatus &ws) > { > gdb_assert (gdbarch != NULL); > gdb_assert (gdbarch->displaced_step_finish != NULL); > if (gdbarch_debug >= 2) > gdb_printf (gdb_stdlog, "gdbarch_displaced_step_finish called\n"); > - return gdbarch->displaced_step_finish (gdbarch, thread, sig); > + return gdbarch->displaced_step_finish (gdbarch, thread, ws); > } > > void > diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py > index 2b1a2b4f602..52beaeaa245 100644 > --- a/gdb/gdbarch_components.py > +++ b/gdb/gdbarch_components.py > @@ -1819,7 +1819,7 @@ Clean up after a displaced step of THREAD. > """, > type="displaced_step_finish_status", > name="displaced_step_finish", > - params=[("thread_info *", "thread"), ("gdb_signal", "sig")], > + params=[("thread_info *", "thread"), ("const target_waitstatus &", "ws")], > predefault="NULL", > invalid="(! gdbarch->displaced_step_finish) != (! gdbarch->displaced_step_prepare)", > ) > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 5c9babb9104..ee812baf8da 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -1895,7 +1895,8 @@ displaced_step_prepare (thread_info *thread) > DISPLACED_STEP_FINISH_STATUS_OK as well. */ > > static displaced_step_finish_status > -displaced_step_finish (thread_info *event_thread, enum gdb_signal signal) > +displaced_step_finish (thread_info *event_thread, > + const target_waitstatus &event_status) > { > displaced_step_thread_state *displaced = &event_thread->displaced_step_state; > > @@ -1917,7 +1918,7 @@ displaced_step_finish (thread_info *event_thread, enum gdb_signal signal) > /* Do the fixup, and release the resources acquired to do the displaced > step. */ > return gdbarch_displaced_step_finish (displaced->get_original_gdbarch (), > - event_thread, signal); > + event_thread, event_status); > } > > /* Data to be passed around while handling an event. This data is > @@ -5128,7 +5129,7 @@ handle_one (const wait_one_event &event) > /* We caught the event that we intended to catch, so > there's no event to save as pending. */ > > - if (displaced_step_finish (t, GDB_SIGNAL_0) > + if (displaced_step_finish (t, event.ws) > == DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED) > { > /* Add it back to the step-over queue. */ > @@ -5143,7 +5144,6 @@ handle_one (const wait_one_event &event) > } > else > { > - enum gdb_signal sig; > struct regcache *regcache; > > infrun_debug_printf > @@ -5154,10 +5154,7 @@ handle_one (const wait_one_event &event) > /* Record for later. */ > save_waitstatus (t, event.ws); > > - sig = (event.ws.kind () == TARGET_WAITKIND_STOPPED > - ? event.ws.sig () : GDB_SIGNAL_0); > - > - if (displaced_step_finish (t, sig) > + if (displaced_step_finish (t, event.ws) > == DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED) > { > /* Add it back to the step-over queue. */ > @@ -5759,7 +5756,7 @@ handle_inferior_event (struct execution_control_state *ecs) > has been done. Perform cleanup for parent process here. Note > that this operation also cleans up the child process for vfork, > because their pages are shared. */ > - displaced_step_finish (ecs->event_thread, GDB_SIGNAL_TRAP); > + displaced_step_finish (ecs->event_thread, ecs->ws); > /* Start a new step-over in another thread if there's one > that needs it. */ > start_step_over (); > @@ -6124,7 +6121,7 @@ resumed_thread_with_pending_status (struct thread_info *tp, > static int > finish_step_over (struct execution_control_state *ecs) > { > - displaced_step_finish (ecs->event_thread, ecs->event_thread->stop_signal ()); > + displaced_step_finish (ecs->event_thread, ecs->ws); > > bool had_step_over_info = step_over_info_valid_p (); > > diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c > index d0bda5ad9c4..1fc9cb6faee 100644 > --- a/gdb/linux-tdep.c > +++ b/gdb/linux-tdep.c > @@ -2626,13 +2626,14 @@ linux_displaced_step_prepare (gdbarch *arch, thread_info *thread, > /* See linux-tdep.h. */ > > displaced_step_finish_status > -linux_displaced_step_finish (gdbarch *arch, thread_info *thread, gdb_signal sig) > +linux_displaced_step_finish (gdbarch *arch, thread_info *thread, > + const target_waitstatus &status) > { > linux_info *per_inferior = get_linux_inferior_data (thread->inf); > > gdb_assert (per_inferior->disp_step_bufs.has_value ()); > > - return per_inferior->disp_step_bufs->finish (arch, thread, sig); > + return per_inferior->disp_step_bufs->finish (arch, thread, status); > } > > /* See linux-tdep.h. */ > diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h > index 16e1b806b26..e09a6ef32b1 100644 > --- a/gdb/linux-tdep.h > +++ b/gdb/linux-tdep.h > @@ -72,7 +72,7 @@ extern displaced_step_prepare_status linux_displaced_step_prepare > /* Implementation of gdbarch_displaced_step_finish. */ > > extern displaced_step_finish_status linux_displaced_step_finish > - (gdbarch *arch, thread_info *thread, gdb_signal sig); > + (gdbarch *arch, thread_info *thread, const target_waitstatus &status); > > /* Implementation of gdbarch_displaced_step_copy_insn_closure_by_addr. */ > > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 52dcc89b2df..8b400047cfb 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -1089,13 +1089,13 @@ ppc_displaced_step_prepare (gdbarch *arch, thread_info *thread, > > static displaced_step_finish_status > ppc_displaced_step_finish (gdbarch *arch, thread_info *thread, > - gdb_signal sig) > + const target_waitstatus &status) > { > ppc_inferior_data *per_inferior = get_ppc_per_inferior (thread->inf); > > gdb_assert (per_inferior->disp_step_buf.has_value ()); > > - return per_inferior->disp_step_buf->finish (arch, thread, sig); > + return per_inferior->disp_step_buf->finish (arch, thread, status); > } > > /* Implementation of gdbarch_displaced_step_restore_all_in_ptid. */ > > base-commit: 91ffa03af1cc32515190c3b52d7b964f5abead5f > -- > 2.36.0