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<scoped_restore_current_thread> 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 <pedro@palves.net>
---
gdb/infrun.c | 2 +-
gdb/regcache.c | 89 +++++++++++++++++++++++++++++++-------------------
gdb/regcache.h | 17 +++++++---
3 files changed, 70 insertions(+), 38 deletions(-)
@@ -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);
@@ -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<scoped_restore_current_thread> 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<scoped_restore_current_thread> 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_target_ops> test_ctx_1;
+ scoped_mock_context<test_target_ops> test_ctx_2;
};
using regcache_test_data_up = std::unique_ptr<regcache_test_data>;
@@ -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<gdb_byte> 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<target_ops_no_register> 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);
@@ -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);
};