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.129.124]) by sourceware.org (Postfix) with ESMTPS id A0F073858D20 for ; Fri, 3 Feb 2023 10:45:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A0F073858D20 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=1675421101; 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=2TqdAbi/2ONScanv96VQRpTJu/SJw6DT5smdFx/qbp0=; b=U43ghkhvfBSuzbCjSJGu7RKd2VZyJQiNIGYOQ7Ryct9EHY7AG1kunSvxLbaEo+wxg90l6B NneAofTnYqnH5Jq+Ro/6yiOgdh9AqEn4INBZftBV5X7/dtmDzyGOKIfqW0Y5papnqu6A3d S61w2ICcBqS0oS9kazHSGfr5RpYTxWs= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-569--ZDkZBfJOVyH9_EBStYtEw-1; Fri, 03 Feb 2023 05:44:55 -0500 X-MC-Unique: -ZDkZBfJOVyH9_EBStYtEw-1 Received: by mail-qv1-f69.google.com with SMTP id ob12-20020a0562142f8c00b004c6c72bf1d0so2490394qvb.9 for ; Fri, 03 Feb 2023 02:44:54 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=2TqdAbi/2ONScanv96VQRpTJu/SJw6DT5smdFx/qbp0=; b=lo41ztGVVs9xVVT/dMTDgzJvFTpLdIPCenR48oPI/gI+V6IyR2P/Shk3z2cmU6EskW yVwSCfsemTNYPpYzcfKHFBbcoC4lrKmADaf0Q6gC487QV2YI7Cnk+Pd8MIMnD1TUxF8R cAH/i1CFHvQaxK+otPuSDAuvUeev4acaK54JBb0XPI29Jl2hoYKXnmsh9KMxE/iTBnUa T0nbXeiwUfNIhnhQUwawSSvg3XyAWcy+/aTeq4It6OAfaO9m0ZRQelkQxZbG03rBALnp I0qcCWr1Yn3XgGrwLRWPe1XujAvLfoCj2z+BVlgCeSEV432OtHsYJ25uqlzIlH0NxvfK jfBg== X-Gm-Message-State: AO0yUKX1kWjfen9Tnv1WENw5EnxsD0l5/AyMVTprNPiLm8SgaSYz/eBi lJMN+RGvazvYQOhSSyNuJkmYyNkh1qeNd3NaLrXTR2Niti4Eu/h5xMPxa2b+FF1jk1y7q8tupae vIEyeC60P7BpcIUp2kchpGQ== X-Received: by 2002:a05:622a:18a1:b0:3b4:d5be:a2e0 with SMTP id v33-20020a05622a18a100b003b4d5bea2e0mr15023225qtc.20.1675421094167; Fri, 03 Feb 2023 02:44:54 -0800 (PST) X-Google-Smtp-Source: AK7set9m2qhCTzPJSryoYVrdTRi4At3Uhad1ncre1VUhkuisJzGQB83+VUTkijkfWCPD/XXCU+KTCw== X-Received: by 2002:a05:622a:18a1:b0:3b4:d5be:a2e0 with SMTP id v33-20020a05622a18a100b003b4d5bea2e0mr15023205qtc.20.1675421093846; Fri, 03 Feb 2023 02:44:53 -0800 (PST) Received: from localhost (95.72.115.87.dyn.plus.net. [87.115.72.95]) by smtp.gmail.com with ESMTPSA id u31-20020a05622a199f00b003b0b903720esm1385782qtc.13.2023.02.03.02.44.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 Feb 2023 02:44:53 -0800 (PST) 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: <20221212203101.1034916-2-pedro@palves.net> References: <20221212203101.1034916-1-pedro@palves.net> <20221212203101.1034916-2-pedro@palves.net> Date: Fri, 03 Feb 2023 10:44:51 +0000 Message-ID: <87fsbnrpss.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.8 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: > 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". > 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. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338 > Change-Id: I4c5d338647b028071bc498c4e47063795a2db4c0 > --- > gdb/displaced-stepping.c | 11 ++++++----- > gdb/displaced-stepping.h | 2 +- > gdb/gdbarch-components.py | 2 +- > gdb/gdbarch-gen.h | 4 ++-- > gdb/gdbarch.c | 4 ++-- > gdb/infrun.c | 17 +++++++---------- > gdb/linux-tdep.c | 5 +++-- > gdb/linux-tdep.h | 2 +- > gdb/rs6000-tdep.c | 4 ++-- > 9 files changed, 25 insertions(+), 26 deletions(-) > > diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c > index 7dfd63d8716..7b5d327008d 100644 > --- a/gdb/displaced-stepping.c > +++ b/gdb/displaced-stepping.c > @@ -192,10 +192,11 @@ 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; > > if (target_stopped_by_watchpoint ()) > @@ -210,7 +211,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 +257,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 de40ae2f3d8..e23a8d6736b 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-components.py b/gdb/gdbarch-components.py > index e7230949aad..5d60f7677f0 100644 > --- a/gdb/gdbarch-components.py > +++ b/gdb/gdbarch-components.py > @@ -1829,7 +1829,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/gdbarch-gen.h b/gdb/gdbarch-gen.h > index a663316df16..5c9390ea6b3 100644 > --- a/gdb/gdbarch-gen.h > +++ b/gdb/gdbarch-gen.h > @@ -1080,8 +1080,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 ddb8dec1c72..559b1dea0d9 100644 > --- a/gdb/gdbarch.c > +++ b/gdb/gdbarch.c > @@ -4097,13 +4097,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/infrun.c b/gdb/infrun.c > index b24cc6d932d..0590310ffac 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -1894,7 +1894,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; > > @@ -1916,7 +1917,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 > @@ -5068,7 +5069,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. */ > @@ -5083,7 +5084,6 @@ handle_one (const wait_one_event &event) > } > else > { > - enum gdb_signal sig; > struct regcache *regcache; > > infrun_debug_printf > @@ -5094,10 +5094,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. */ > @@ -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. 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? Thanks, Andrew > /* Start a new step-over in another thread if there's one > that needs it. */ > start_step_over (); > @@ -6064,7 +6061,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 c30d9fb13f8..8a1d701d7c9 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 95cc29c828c..cba67351574 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 cbd84514795..dc0f78ed9ab 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. */ > -- > 2.36.0