From patchwork Mon Apr 3 18:52:06 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 67230 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 20666384842D for ; Mon, 3 Apr 2023 18:53:31 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 20666384842D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1680548011; bh=AhQAG6CS/RaaQP8yAtUxuTo91ysH2fjuS5lrMtCutf0=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=v5hXiZSikXBY6AgAANkR8UW8xI+uRKVRVv/CorO/Im5MBCrqZ4UFz8NYbmnMmqRMo /1XWp6c+1DcwcnFFL/wMyHlqcIAmiLApNf6GilKlFdOqYermGxcrfV7fLCmRnxKfRu W8zBYeiH69U0SKo3gi6/sMSDDiVVARnYa2F2GGjU= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 821323858C00 for ; Mon, 3 Apr 2023 18:52:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 821323858C00 Received: from localhost.localdomain (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id D3E4D1E225; Mon, 3 Apr 2023 14:52:20 -0400 (EDT) To: gdb-patches@sourceware.org Cc: Simon Marchi , Pedro Alves Subject: [PATCH 5/7] gdb: make regcache::raw_update switch to right inferior Date: Mon, 3 Apr 2023 14:52:06 -0400 Message-Id: <20230403185208.197965-6-simon.marchi@efficios.com> X-Mailer: git-send-email 2.40.0 In-Reply-To: <20230403185208.197965-1-simon.marchi@efficios.com> References: <20230403185208.197965-1-simon.marchi@efficios.com> MIME-Version: 1.0 X-Spam-Status: No, score=-1173.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_SOFTFAIL, 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 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-Patchwork-Original-From: Simon Marchi via Gdb-patches From: Simon Marchi Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" With the following patch, which teaches the amd-dbgapi target to handle inferiors that fork, we end up with target stacks in the following state, when an inferior that does not use the GPU forks an inferior that eventually uses the GPU. inf 1 inf 2 ----- ----- amd-dbgapi linux-nat linux-nat exec exec When a GPU thread from inferior 2 hits a breakpoint, the following sequence of events would happen, if it was not for the current patch. - we start with inferior 1 as current - do_target_wait_1 makes inferior 2 current, does a target_wait, which returns a stop event for an amd-dbgapi wave (thread). - do_target_wait's scoped_restore_current_thread restores inferior 1 as current - fetch_inferior_event calls switch_to_target_no_thread with linux-nat as the process target, since linux-nat is officially the process target of inferior 2. This makes inferior 1 the current inferior, as it's the first inferior with that target. - In handle_signal_stop, we have: ecs->event_thread->suspend.stop_pc = regcache_read_pc (get_thread_regcache (ecs->event_thread)); context_switch (ecs); regcache_read_pc executes while inferior 1 is still the current one (because it's before the `context_switch`). This is a problem, because the regcache is for a ptid managed by the amd-dbgapi target (e.g. (12345, 1, 1)), a ptid that does not make sense for the linux-nat target. The fetch_registers target call goes directly to the linux-nat target, which gets confused. - We would then get an error like: Couldn't get extended state status: No such process. ... since linux-nat tries to do a ptrace call on tid 1. GDB should switch to the inferior the ptid belongs to before doing the target call to fetch registers, to make sure the call hits the right target stack (it should be handled by the amd-dbgapi target in this case). In fact the following patch does this change, and it would be enough to fix this specific problem. However, I propose to change regcache to make it switch to the right inferior, if needed, before doing target calls. That makes the interface as a whole more independent of the global context. My first attempt at doing this was to find an inferior using the process stratum target and the ptid that regcache already knows about: gdb::optional restore_thread; inferior *inf = find_inferior_ptid (this->target (), this->ptid ()); if (inf != current_inferior ()) { restore_thread.emplace (); switch_to_inferior_no_thread (inf); } However, this caused some failures in fork-related tests and gdbserver boards. When we detach a fork child, we may create a regcache for the child, but there is no corresponding inferior. For instance, to restore the PC after a displaced step over the fork syscall. So find_inferior_ptid would return nullptr, and switch_to_inferior_no_thread would hit a failed assertion. So, this patch adds to regcache the information "the inferior to switch to to makes target calls". In typical cases, it will be the inferior that matches the regcache's ptid. But in some cases, like the detached fork child one, it will be another inferior (in this example, it will be the fork parent inferior). The problem that we witnessed was in regcache::raw_update specifically, but I looked for other regcache methods doing target calls, and added the same inferior switching code to raw_write too. In the regcache constructor and in get_thread_arch_aspace_regcache, "inf_for_target_calls" replaces the process_stratum_target parameter. We suppose that the process stratum target that would be passed otherwise is the same that is in inf_for_target_calls's target stack, so we don't need to pass both in parallel. The process stratum target is still used as a key in the `target_pid_ptid_regcache_map` map, but that's it. There is one spot that needs to be updated outside of the regcache code, which is the path that handles the "restore PC after a displaced step in a fork child we're about to detach" case mentioned above. regcache_test_data needs to be changed to include full-fledged mock contexts (because there now needs to be inferiors, not just targets). Change-Id: Id088569ce106e1f194d9ae7240ff436f11c5e123 Reviewed-By: Pedro Alves --- gdb/infrun.c | 2 +- gdb/regcache.c | 89 +++++++++++++++++++++++++++++++------------------- gdb/regcache.h | 17 +++++++--- 3 files changed, 70 insertions(+), 38 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 11a788467a8a..f32e037f3649 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -5805,7 +5805,7 @@ handle_inferior_event (struct execution_control_state *ecs) list yet at this point. */ child_regcache - = get_thread_arch_aspace_regcache (parent_inf->process_target (), + = get_thread_arch_aspace_regcache (parent_inf, ecs->ws.child_ptid (), gdbarch, parent_inf->aspace); diff --git a/gdb/regcache.c b/gdb/regcache.c index cfa8a3d78335..56292fbd4bff 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -208,11 +208,12 @@ reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo) } } -regcache::regcache (process_stratum_target *target, gdbarch *gdbarch, +regcache::regcache (inferior *inf_for_target_calls, gdbarch *gdbarch, const address_space *aspace_) /* The register buffers. A read/write register cache can only hold [0 .. gdbarch_num_regs). */ - : detached_regcache (gdbarch, false), m_aspace (aspace_), m_target (target) + : detached_regcache (gdbarch, false), m_aspace (aspace_), + m_inf_for_target_calls (inf_for_target_calls) { m_ptid = minus_one_ptid; } @@ -348,14 +349,17 @@ using target_pid_ptid_regcache_map static target_pid_ptid_regcache_map regcaches; struct regcache * -get_thread_arch_aspace_regcache (process_stratum_target *target, +get_thread_arch_aspace_regcache (inferior *inf_for_target_calls, ptid_t ptid, gdbarch *arch, struct address_space *aspace) { - gdb_assert (target != nullptr); + gdb_assert (inf_for_target_calls != nullptr); + + process_stratum_target *proc_target = inf_for_target_calls->process_target (); + gdb_assert (proc_target != nullptr); /* Find the map for this target. */ - pid_ptid_regcache_map &pid_ptid_regc_map = regcaches[target]; + pid_ptid_regcache_map &pid_ptid_regc_map = regcaches[proc_target]; /* Find the map for this pid. */ ptid_regcache_map &ptid_regc_map = pid_ptid_regc_map[ptid.pid ()]; @@ -369,7 +373,7 @@ get_thread_arch_aspace_regcache (process_stratum_target *target, } /* It does not exist, create it. */ - regcache *new_regcache = new regcache (target, arch, aspace); + regcache *new_regcache = new regcache (inf_for_target_calls, arch, aspace); new_regcache->set_ptid (ptid); /* Work around a problem with g++ 4.8 (PR96537): Call the regcache_up constructor explictly instead of implicitly. */ @@ -383,10 +387,11 @@ get_thread_arch_regcache (process_stratum_target *target, ptid_t ptid, struct gdbarch *gdbarch) { scoped_restore_current_inferior restore_current_inferior; - set_current_inferior (find_inferior_ptid (target, ptid)); + inferior *inf = find_inferior_ptid (target, ptid); + set_current_inferior (inf); address_space *aspace = target_thread_address_space (ptid); - return get_thread_arch_aspace_regcache (target, ptid, gdbarch, aspace); + return get_thread_arch_aspace_regcache (inf, ptid, gdbarch, aspace); } static process_stratum_target *current_thread_target; @@ -591,6 +596,9 @@ regcache::raw_update (int regnum) if (get_register_status (regnum) == REG_UNKNOWN) { + gdb::optional maybe_restore_thread + = maybe_switch_inferior (m_inf_for_target_calls); + target_fetch_registers (this, regnum); /* A number of targets can't access the whole set of raw @@ -842,6 +850,9 @@ regcache::raw_write (int regnum, const gdb_byte *buf) m_descr->sizeof_register[regnum]) == 0)) return; + gdb::optional maybe_restore_thread + = maybe_switch_inferior (m_inf_for_target_calls); + target_prepare_to_store (this); raw_supply (regnum, buf); @@ -1610,16 +1621,16 @@ regcache_count (process_stratum_target *target, ptid_t ptid) /* Wrapper around get_thread_arch_aspace_regcache that does some self checks. */ static void -get_thread_arch_aspace_regcache_and_check (process_stratum_target *target, +get_thread_arch_aspace_regcache_and_check (inferior *inf_for_target_calls, ptid_t ptid) { /* We currently only test with a single gdbarch. Any gdbarch will do, so use the current inferior's gdbarch. Also use the current inferior's address space. */ - gdbarch *arch = current_inferior ()->gdbarch; - address_space *aspace = current_inferior ()->aspace; - regcache *regcache - = get_thread_arch_aspace_regcache (target, ptid, arch, aspace); + gdbarch *arch = inf_for_target_calls->gdbarch; + address_space *aspace = inf_for_target_calls->aspace; + regcache *regcache = get_thread_arch_aspace_regcache (inf_for_target_calls, + ptid, arch, aspace); SELF_CHECK (regcache != NULL); SELF_CHECK (regcache->ptid () == ptid); @@ -1633,6 +1644,9 @@ get_thread_arch_aspace_regcache_and_check (process_stratum_target *target, struct regcache_test_data { regcache_test_data () + /* The specific arch doesn't matter. */ + : test_ctx_1 (current_inferior ()->gdbarch), + test_ctx_2 (current_inferior ()->gdbarch) { /* Ensure the regcaches container is empty at the start. */ registers_changed (); @@ -1644,8 +1658,8 @@ struct regcache_test_data registers_changed (); } - test_target_ops test_target1; - test_target_ops test_target2; + scoped_mock_context test_ctx_1; + scoped_mock_context test_ctx_2; }; using regcache_test_data_up = std::unique_ptr; @@ -1670,12 +1684,12 @@ populate_regcaches_for_test () for (long lwp : { 1, 2, 3 }) { get_thread_arch_aspace_regcache_and_check - (&data->test_target1, ptid_t (pid, lwp)); + (&data->test_ctx_1.mock_inferior, ptid_t (pid, lwp)); expected_regcache_size++; SELF_CHECK (regcaches_size () == expected_regcache_size); get_thread_arch_aspace_regcache_and_check - (&data->test_target2, ptid_t (pid, lwp)); + (&data->test_ctx_2.mock_inferior, ptid_t (pid, lwp)); expected_regcache_size++; SELF_CHECK (regcaches_size () == expected_regcache_size); } @@ -1693,7 +1707,8 @@ get_thread_arch_aspace_regcache_test () size_t regcaches_size_before = regcaches_size (); /* Test that getting an existing regcache doesn't create a new one. */ - get_thread_arch_aspace_regcache_and_check (&data->test_target1, ptid_t (2, 2)); + get_thread_arch_aspace_regcache_and_check (&data->test_ctx_1.mock_inferior, + ptid_t (2, 2)); SELF_CHECK (regcaches_size () == regcaches_size_before); } @@ -1715,12 +1730,14 @@ registers_changed_ptid_target_test () { regcache_test_data_up data = populate_regcaches_for_test (); - registers_changed_ptid (&data->test_target1, minus_one_ptid); + registers_changed_ptid (&data->test_ctx_1.mock_target, minus_one_ptid); SELF_CHECK (regcaches_size () == 6); /* Check that we deleted the regcache for the right target. */ - SELF_CHECK (regcache_count (&data->test_target1, ptid_t (2, 2)) == 0); - SELF_CHECK (regcache_count (&data->test_target2, ptid_t (2, 2)) == 1); + SELF_CHECK (regcache_count (&data->test_ctx_1.mock_target, + ptid_t (2, 2)) == 0); + SELF_CHECK (regcache_count (&data->test_ctx_2.mock_target, + ptid_t (2, 2)) == 1); } /* Test marking regcaches of a specific (target, pid) as changed. */ @@ -1730,13 +1747,15 @@ registers_changed_ptid_target_pid_test () { regcache_test_data_up data = populate_regcaches_for_test (); - registers_changed_ptid (&data->test_target1, ptid_t (2)); + registers_changed_ptid (&data->test_ctx_1.mock_target, ptid_t (2)); SELF_CHECK (regcaches_size () == 9); /* Regcaches from target1 should not exist, while regcaches from target2 should exist. */ - SELF_CHECK (regcache_count (&data->test_target1, ptid_t (2, 2)) == 0); - SELF_CHECK (regcache_count (&data->test_target2, ptid_t (2, 2)) == 1); + SELF_CHECK (regcache_count (&data->test_ctx_1.mock_target, + ptid_t (2, 2)) == 0); + SELF_CHECK (regcache_count (&data->test_ctx_2.mock_target, + ptid_t (2, 2)) == 1); } /* Test marking regcaches of a specific (target, ptid) as changed. */ @@ -1746,12 +1765,14 @@ registers_changed_ptid_target_ptid_test () { regcache_test_data_up data = populate_regcaches_for_test (); - registers_changed_ptid (&data->test_target1, ptid_t (2, 2)); + registers_changed_ptid (&data->test_ctx_1.mock_target, ptid_t (2, 2)); SELF_CHECK (regcaches_size () == 11); /* Check that we deleted the regcache for the right target. */ - SELF_CHECK (regcache_count (&data->test_target1, ptid_t (2, 2)) == 0); - SELF_CHECK (regcache_count (&data->test_target2, ptid_t (2, 2)) == 1); + SELF_CHECK (regcache_count (&data->test_ctx_1.mock_target, + ptid_t (2, 2)) == 0); + SELF_CHECK (regcache_count (&data->test_ctx_2.mock_target, + ptid_t (2, 2)) == 1); } class target_ops_no_register : public test_target_ops @@ -1812,9 +1833,9 @@ target_ops_no_register::xfer_partial (enum target_object object, class readwrite_regcache : public regcache { public: - readwrite_regcache (process_stratum_target *target, + readwrite_regcache (inferior *inf_for_target_calls, struct gdbarch *gdbarch) - : regcache (target, gdbarch, nullptr) + : regcache (inf_for_target_calls, gdbarch, nullptr) {} }; @@ -1861,7 +1882,8 @@ cooked_read_test (struct gdbarch *gdbarch) break; } - readwrite_regcache readwrite (&mockctx.mock_target, gdbarch); + readwrite_regcache readwrite (&mockctx.mock_inferior, gdbarch); + readwrite.set_ptid (mockctx.mock_ptid); gdb::def_vector buf (register_size (gdbarch, nonzero_regnum)); readwrite.raw_read (nonzero_regnum, buf.data ()); @@ -1978,7 +2000,8 @@ cooked_write_test (struct gdbarch *gdbarch) /* Create a mock environment. A process_stratum target pushed. */ scoped_mock_context ctx (gdbarch); - readwrite_regcache readwrite (&ctx.mock_target, gdbarch); + readwrite_regcache readwrite (&ctx.mock_inferior, gdbarch); + readwrite.set_ptid (ctx.mock_ptid); const int num_regs = gdbarch_num_cooked_regs (gdbarch); for (auto regnum = 0; regnum < num_regs; regnum++) @@ -2093,9 +2116,9 @@ regcache_thread_ptid_changed () gdb_assert (regcaches.empty ()); /* Populate the regcaches container. */ - get_thread_arch_aspace_regcache (&target1.mock_target, old_ptid, arch, + get_thread_arch_aspace_regcache (&target1.mock_inferior, old_ptid, arch, nullptr); - get_thread_arch_aspace_regcache (&target2.mock_target, old_ptid, arch, + get_thread_arch_aspace_regcache (&target2.mock_inferior, old_ptid, arch, nullptr); gdb_assert (regcaches.size () == 2); diff --git a/gdb/regcache.h b/gdb/regcache.h index 2bd2f57b8332..57ddac465f09 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -29,6 +29,7 @@ struct gdbarch; struct address_space; class thread_info; struct process_stratum_target; +struct inferior; extern struct regcache *get_current_regcache (void); extern struct regcache *get_thread_regcache (process_stratum_target *target, @@ -40,7 +41,7 @@ extern struct regcache *get_thread_regcache (thread_info *thread); extern struct regcache *get_thread_arch_regcache (process_stratum_target *targ, ptid_t, struct gdbarch *); extern struct regcache *get_thread_arch_aspace_regcache - (process_stratum_target *target, ptid_t, + (inferior *inf_for_target_calls, ptid_t, struct gdbarch *, struct address_space *); extern enum register_status @@ -421,7 +422,7 @@ class regcache : public detached_regcache void debug_print_register (const char *func, int regno); protected: - regcache (process_stratum_target *target, gdbarch *gdbarch, + regcache (inferior *inf_for_target_calls, gdbarch *gdbarch, const address_space *aspace); private: @@ -448,13 +449,21 @@ class regcache : public detached_regcache makes sense, like PC or SP). */ const address_space * const m_aspace; + /* The inferior to switch to, to make target calls. + + This may not be the inferior of thread M_PTID. For instance, this + regcache might be for a fork child we are about to detach, so there will + never be an inferior for that thread / process. Nevertheless, we need to + be able to switch to the target stack that can handle register reads / + writes for this regcache, and that's what this inferior is for. */ + inferior *m_inf_for_target_calls; + /* If this is a read-write cache, which thread's registers is it connected to? */ - process_stratum_target *m_target; ptid_t m_ptid; friend struct regcache * - get_thread_arch_aspace_regcache (process_stratum_target *target, ptid_t ptid, + get_thread_arch_aspace_regcache (inferior *inf_for_target_calls, ptid_t ptid, struct gdbarch *gdbarch, struct address_space *aspace); };