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 E97A23858D35 for ; Thu, 16 Mar 2023 16:07:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E97A23858D35 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=1678982878; 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=pujygBlzbFnrJF/hzLoz1NxRqwDRyVR3GxNQ+GRIpoQ=; b=QPYO+Xg5l7/hpk0x15jnH7iQOt6x5FYRJquhlL1B7LLjGKlOCc8vJ/pFx+dDa656ICNHYs isscHo0m/SBf3zbpwPGc/B6752M1XrDRuxBYDbBG7Mp/4jhHsta7vhY7EHcynG0ZMIG1+F i6ESYf3P2cPHzrkzD2+CZhJDtzReC2k= Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-422-Ga3Ir-pfMOO2CMBesQ6IAw-1; Thu, 16 Mar 2023 12:07:57 -0400 X-MC-Unique: Ga3Ir-pfMOO2CMBesQ6IAw-1 Received: by mail-ed1-f70.google.com with SMTP id k12-20020a50c8cc000000b004accf30f6d3so3742273edh.14 for ; Thu, 16 Mar 2023 09:07:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678982876; 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=pujygBlzbFnrJF/hzLoz1NxRqwDRyVR3GxNQ+GRIpoQ=; b=WjcEgvoWnKEoDHnaYd2vjgEEUyn3aVf5RDZhMRdB9oHBmbucKsjAx3ketoLe35f7tm CnFEGU+K0GTUdEkDCt4nwevuNCDT1VbFk9FoniMAQt27SmZKoLORVpQSgtuU71eOq+qP Do0O6os5iZ41jhA0hSlSPZNP1QyAmrVQMLn8MChLDncv494o43qUWHx4lvyGpA69sin1 c6ThY3Xb8gV/MHZw+PjrNoAN6hMO2cErRtwOjIG9j/GyAbWCP+U/cwEqDyHUTMhAI7VJ uOWeoK2zMZ7E1Jg96qjsfU8Nx/Lw3KJpzphabEY8885cNP5+wHT1ST+KOnBqYssrSVke cgVg== X-Gm-Message-State: AO0yUKWfWnYJDvB3MB1pPqrJuDD4H+d7Kpai1nRTxKMy3M5ZnQ3XHCMg 8Xd6NAgC9+1iXDkb+82nm5LfeiphaGzR2Qf33DY+gaSTZCY88e440WPXjwmXaKsHx5eFd1nW4UT y3yLCpVwUuXxS6eRGuCMn08QRyr5ERw== X-Received: by 2002:a17:907:9862:b0:92f:a61a:f755 with SMTP id ko2-20020a170907986200b0092fa61af755mr5292812ejc.73.1678982875938; Thu, 16 Mar 2023 09:07:55 -0700 (PDT) X-Google-Smtp-Source: AK7set+N91vTzfz/GabsoTp/P1veRpHEQuD4jIhAzCJASGb8Ov+BX9UG/fUydi6QUuCGBHZlps5v7w== X-Received: by 2002:a17:907:9862:b0:92f:a61a:f755 with SMTP id ko2-20020a170907986200b0092fa61af755mr5292789ejc.73.1678982875650; Thu, 16 Mar 2023 09:07:55 -0700 (PDT) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id hb12-20020a170907160c00b008d9ddd2da88sm4004300ejc.6.2023.03.16.09.07.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Mar 2023 09:07:55 -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: References: <20221212203101.1034916-1-pedro@palves.net> <20221212203101.1034916-2-pedro@palves.net> <87fsbnrpss.fsf@redhat.com> Date: Thu, 16 Mar 2023 16:07:54 +0000 Message-ID: <87zg8cel2d.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: > On 2023-02-03 10:44 a.m., Andrew Burgess wrote: >> Pedro Alves writes: >> >>> This commit tweaks displaced_step_finish & friends to pass down a >>> target_waitstatus instead of a gdb_signal. This needed because a >> >> missing word: "This IS needed". > > Fixed. > >>> @@ -5699,7 +5696,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); >> >> This change is interesting. >> >> If I understand the code correctly, this call will eventually end up in >> displaced_step_buffers::finish (displaced-stepping.c), which in turn >> calls displaced_step_instruction_executed_successfully. >> >> Previously, we always passed GDB_SIGNAL_TRAP here, which (if we ignore >> the watchpoint check in >> displaced_step_instruction_executed_successfully) means that >> displaced_step_instruction_executed_successfully would always return >> true, and then displaced_step_buffers::finish would call >> gdbarch_displaced_step_fixup. >> >> After this change, we know that esc->ws.kind is either >> TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED, so we know that >> displaced_step_instruction_executed_successfully will always return >> false, and displaced_step_buffers::finish will no longer call >> gdbarch_displaced_step_fixup. >> > > Good catch! > > I was tweaking the change to address your comment, and was coming to the > conclusion that what I really wanted was this: > > static bool > displaced_step_instruction_executed_successfully > (gdbarch *arch, const target_waitstatus &status) > { > if (status.kind () == TARGET_WAITKIND_STOPPED > && status.sig () != GDB_SIGNAL_TRAP) > return false; > > /* All other 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. */ > > (the comment is new) > > And then, I remembered, I actually wrote that early if like that originally, > and I changed it in response to this review: > > https://inbox.sourceware.org/gdb-patches/44f74af8-248b-1af8-3612-980c08607bf4@simark.ca/ > > The review was totally right, it was my response that was misguided. > > But I'm confused because I am pretty sure that I wrote a reply to that > message, saying that I did not intend to change the behavior, so I'd "fix" it. > I can't find it in my outbox either, I guess I erroneously canceled my > email window instead of sending the message... > > Anyhow, looks like I made it worse while trying to address Simon's comment. :-P > > So I think I should go back to what I had, like before, and my response > to Simon should have been instead: > > - yes, the change is intended. If we stopped for an event other than > TARGET_WAITKIND_STOPPED, like for instance, TARGET_WAITKIND_FORKED, > TARGET_WAITKIND_SYSCALL_ENTRY, then it must be that the instruction > executed successfully, otherwise the syscall wouldn't have triggered. > >> What I don't understand well enough is what this actually means for a >> running inferior. >> >> It's odd because the comment in infrun.c (just above your change) >> indicates that to get to this point the displaced step must have >> completed successfully, while after this change, the new code path in >> displaced_step_buffers::finish indicates we believe the displaced step >> didn't complete successfully: >> >> /* Since the instruction didn't complete, all we can do is relocate the >> PC. */ >> >> Do you know if any of our test cases hit this path? >> > > I know that you posted a series for this, which I plan to take a good look > at (I actually planned on doing that earlier this week, but I had a couple > major distractions, sorry). > > WDYT of the version below? 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). Thanks, Andrew > > From 0ccfd6822ef83eddb48c6f7bf21533858f177bc9 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 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 > the series. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338 > Change-Id: I4c5d338647b028071bc498c4e47063795a2db4c0 > --- > 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 06b32a80f6a..08ae1ce4397 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 ddb97f60315..5805dcb75ef 100644 > --- a/gdb/gdbarch-gen.h > +++ b/gdb/gdbarch-gen.h > @@ -1103,8 +1103,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 7e4e34d5aca..0b122a86055 100644 > --- a/gdb/gdbarch.c > +++ b/gdb/gdbarch.c > @@ -4096,13 +4096,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 caa65c334ec..8d72bff347e 100644 > --- a/gdb/gdbarch_components.py > +++ b/gdb/gdbarch_components.py > @@ -1878,7 +1878,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 ab77300f1ff..99f2a8e039d 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 > @@ -5122,7 +5123,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. */ > @@ -5137,7 +5138,6 @@ handle_one (const wait_one_event &event) > } > else > { > - enum gdb_signal sig; > struct regcache *regcache; > > infrun_debug_printf > @@ -5148,10 +5148,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. */ > @@ -5753,7 +5750,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 (); > @@ -6118,7 +6115,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 e6ce13a1c67..036ea9f931d 100644 > --- a/gdb/linux-tdep.c > +++ b/gdb/linux-tdep.c > @@ -2621,13 +2621,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 104515de030..f9bbc435184 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -1088,13 +1088,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: 2562954ede66f32bff7d985e752b8052c2ae5775 > -- > 2.36.0