[4/4] Introduce class target_stack
Commit Message
Currently, the target stack is represented by a singly linked list,
with target_ops having a pointer to the target beneath. This poses a
problem for multi-process / multi-target debugging. In that case, we
will naturally want multiple instances of target stacks. E.g., one
stack for inferior 1 which is debugging a core file, and another
target stack for inferior 2 which is debugging a remote process. The
problem then is in finding a target's "beneath" target, if we consider
that for some target_ops types, we'll be sharing a single target_ops
instance between several inferiors. For example, so far, I found no
need to have multiple instances of the spu_multiarch_target /
exec_target / dummy_target targets.
Thus this patch, which changes the target stack representation to an
array of pointers. For now, there's still a single global instance of
this new target_stack class, though further down in the multi-target
work, each inferior will have its own instance.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* target.h (target_ops) <beneath>: Now a method. All references
updated.
(class target_stack): New.
* target.c (g_target_stack): New.
(g_current_top_target): Delete.
(current_top_target): Get the top target out of g_target_stack.
(target_stack::push, target_stack::unpush): New.
(push_target, unpush_target): Reimplement.
(target_is_pushed): Reimplement in terms of g_target_stack.
(target_ops::beneath, target_stack::find_beneath): New.
---
gdb/target.c | 116 +++++++++++++++++++++++++++++------------------------------
gdb/target.h | 47 ++++++++++++++++++++++--
2 files changed, 103 insertions(+), 60 deletions(-)
Comments
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> The problem then is in finding a target's "beneath" target, if we
Pedro> consider that for some target_ops types, we'll be sharing a
Pedro> single target_ops instance between several inferiors. For
Pedro> example, so far, I found no need to have multiple instances of
Pedro> the spu_multiarch_target / exec_target / dummy_target targets.
Is it the case that sometimes a particular target_ops instance will have
to be shared among multiple target stacks?
If so, then this all makes sense to me.
If not, then it seems maybe slightly off, because then if you need to
add state to some existing target_ops, you will have to check whether it
is shareable and then ensure it is unshared -- does this make sense?
But on the other hand, an instance of something like dummy_target should
be cheap (sizeof == 24 here). So maybe a simpler rule is available.
On the third hand, it's only slightly off, and I don't want to get in
the way of this important project; but I would still like to understand.
Pedro> +/* The target stack. */
Pedro> +static target_stack g_target_stack;
As an aside, I often wonder about blank lines between comments and
definitions.
I prefer not to have them, though not for any actual reason. But some
spots have them and some do not, so in practice what I do is either
follow the local style; or leave the blank line out if I think I can; or
else feel guilty since I vaguely recall there was a rule to have one and
so put it in even though I'd rather not.
Pedro> + Note that rather than allow an empty stack, we always have the
Pedro> + dummy target at the bottom stratum, so we can call the function
Pedro> + vectors without checking them. */
Probably just "methods" rather than "function vectors".
Tom
On 05/29/2018 04:13 AM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> The problem then is in finding a target's "beneath" target, if we
> Pedro> consider that for some target_ops types, we'll be sharing a
> Pedro> single target_ops instance between several inferiors. For
> Pedro> example, so far, I found no need to have multiple instances of
> Pedro> the spu_multiarch_target / exec_target / dummy_target targets.
>
> Is it the case that sometimes a particular target_ops instance will have
> to be shared among multiple target stacks?
>
> If so, then this all makes sense to me.
Right, it is the case. That is true for targets that support
multi-process. E.g., native targets and remote targets.
The current model in the branch is:
- Each inferior gets its own target stack.
- There's only ever one instance of the native target_ops.
All native processes/inferiors use the same target_ops, because
it's the same ptrace API "instance" that talks to all of them.
For example, if you do "info os processes" to list all processes,
it's the single native target_ops instance that implements that
call.
- There's one remote_target (target_ops) instance for each
remote connection. All processes/inferiors debugged via that
connection share the same remote_target instance.
You can have multiple remote connections.
- Each process_stratum target_ops instance has its own PID number
space. IOW, a process is uniquely identified by the
(process_stratum target_ops *, int PID) tuple.
Since each inferior has its own target stack, we should be able to
do things like activate "target record full" for inferior 1, and
"target record btrace" for inferior 2, though I haven't done that.
>
> Pedro> +/* The target stack. */
> Pedro> +static target_stack g_target_stack;
>
> As an aside, I often wonder about blank lines between comments and
> definitions.
>
> I prefer not to have them, though not for any actual reason. But some
> spots have them and some do not, so in practice what I do is either
> follow the local style; or leave the blank line out if I think I can; or
> else feel guilty since I vaguely recall there was a rule to have one and
> so put it in even though I'd rather not.
I think the main rule is: blank line in definition, no blank line
in declarations:
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Documenting_Your_Code
That's what I think most newer code follows, but it's like you say,
in reality I guess most folks follow the local style.
I've fixed that case you pointed out.
The case in our codebase where I most tend to find blank lines odd,
is in the documentation of structure fields:
struct S
{
/* This is foo. */
int foo;
/* This is bar. */
int bar;
};
I think I dislike it because that way you lose the visual grouping
between comment and field that you otherwise have with:
struct S
{
/* This is foo. */
int foo;
/* This is bar. */
int bar;
};
Curiously, this case that is not explicitly specified in the wiki,
though the struct example there does not include spaces.
>
> Pedro> + Note that rather than allow an empty stack, we always have the
> Pedro> + dummy target at the bottom stratum, so we can call the function
> Pedro> + vectors without checking them. */
>
> Probably just "methods" rather than "function vectors".
Indeed, probably less confusing to newcomers. That was just
copied over from elsewhere, but I'll adjust it.
Thanks,
Pedro Alves
@@ -119,16 +119,17 @@ static std::unordered_map<const target_info *, target_open_ftype *>
static struct target_ops *the_dummy_target;
static struct target_ops *the_debug_target;
+/* The target stack. */
+static target_stack g_target_stack;
+
/* Top of target stack. */
/* The target structure we are currently using to talk to a process
or file or whatever "inferior" we have. */
-static target_ops *g_current_top_target;
-
target_ops *
current_top_target ()
{
- return g_current_top_target;
+ return g_target_stack.top ();
}
/* Command list for target. */
@@ -631,49 +632,46 @@ default_execution_direction (struct target_ops *self)
to_execution_direction must be implemented for reverse async");
}
-/* Push a new target type into the stack of the existing target accessors,
- possibly superseding some of the existing accessors.
-
- Rather than allow an empty stack, we always have the dummy target at
- the bottom stratum, so we can call the function vectors without
- checking them. */
+/* See target.h. */
void
-push_target (struct target_ops *t)
+target_stack::push (target_ops *t)
{
- struct target_ops **cur;
-
- /* Find the proper stratum to install this target in. */
- for (cur = &g_current_top_target; (*cur) != NULL; cur = &(*cur)->m_beneath)
+ /* If there's already a target at this stratum, remove it. */
+ if (m_stack[t->to_stratum] != NULL)
{
- if ((int) (t->to_stratum) >= (int) (*cur)->to_stratum)
- break;
+ target_ops *prev = m_stack[t->to_stratum];
+ m_stack[t->to_stratum] = NULL;
+ target_close (prev);
}
- /* If there's already targets at this stratum, remove them. */
- /* FIXME: cagney/2003-10-15: I think this should be popping all
- targets to CUR, and not just those at this stratum level. */
- while ((*cur) != NULL && t->to_stratum == (*cur)->to_stratum)
- {
- /* There's already something at this stratum level. Close it,
- and un-hook it from the stack. */
- struct target_ops *tmp = (*cur);
+ /* Now add the new one. */
+ m_stack[t->to_stratum] = t;
- (*cur) = (*cur)->m_beneath;
- tmp->m_beneath = NULL;
- target_close (tmp);
- }
+ if (m_top < t->to_stratum)
+ m_top = t->to_stratum;
+}
- /* We have removed all targets in our stratum, now add the new one. */
- t->m_beneath = (*cur);
- (*cur) = t;
+/* See target.h. */
+
+void
+push_target (struct target_ops *t)
+{
+ g_target_stack.push (t);
}
-/* Remove a target_ops vector from the stack, wherever it may be.
- Return how many times it was removed (0 or 1). */
+/* See target.h. */
int
unpush_target (struct target_ops *t)
+{
+ return g_target_stack.unpush (t);
+}
+
+/* See target.h. */
+
+bool
+target_stack::unpush (target_ops *t)
{
struct target_ops **cur;
struct target_ops *tmp;
@@ -682,31 +680,30 @@ unpush_target (struct target_ops *t)
internal_error (__FILE__, __LINE__,
_("Attempt to unpush the dummy target"));
+ gdb_assert (t != NULL);
+
/* Look for the specified target. Note that we assume that a target
can only occur once in the target stack. */
- for (cur = &g_current_top_target; (*cur) != NULL; cur = &(*cur)->m_beneath)
+ if (m_stack[t->to_stratum] != t)
{
- if ((*cur) == t)
- break;
+ /* If we don't find target_ops, quit. Only open targets should be
+ closed. */
+ return false;
}
- /* If we don't find target_ops, quit. Only open targets should be
- closed. */
- if ((*cur) == NULL)
- return 0;
-
/* Unchain the target. */
- tmp = (*cur);
- (*cur) = (*cur)->m_beneath;
- tmp->m_beneath = NULL;
+ m_stack[t->to_stratum] = NULL;
+
+ if (m_top == t->to_stratum)
+ m_top = t->beneath ()->to_stratum;
/* Finally close the target. Note we do this after unchaining, so
any target method calls from within the target_close
implementation don't end up in T anymore. */
target_close (t);
- return 1;
+ return true;
}
/* Unpush TARGET and assert that it worked. */
@@ -751,13 +748,7 @@ pop_all_targets (void)
int
target_is_pushed (struct target_ops *t)
{
- for (target_ops *cur = current_top_target ();
- cur != NULL;
- cur = cur->beneath ())
- if (cur == t)
- return 1;
-
- return 0;
+ return g_target_stack.is_pushed (t);
}
/* Default implementation of to_get_thread_local_address. */
@@ -2625,7 +2616,7 @@ target_thread_address_space (ptid_t ptid)
target_ops *
target_ops::beneath () const
{
- return m_beneath;
+ return g_target_stack.find_beneath (this);
}
void
@@ -3213,16 +3204,25 @@ default_thread_architecture (struct target_ops *ops, ptid_t ptid)
/* See target.h. */
-struct target_ops *
-find_target_at (enum strata stratum)
+target_ops *
+target_stack::find_beneath (const target_ops *t) const
{
- for (target_ops *t = current_top_target (); t != NULL; t = t->beneath ())
- if (t->to_stratum == stratum)
- return t;
+ /* Look for a non-empty slot at stratum levels beneath T's. */
+ for (int stratum = t->to_stratum - 1; stratum >= 0; --stratum)
+ if (m_stack[stratum] != NULL)
+ return m_stack[stratum];
return NULL;
}
+/* See target.h. */
+
+struct target_ops *
+find_target_at (enum strata stratum)
+{
+ return g_target_stack.at (stratum);
+}
+
/* See target.h */
@@ -61,7 +61,11 @@ struct inferior;
of variables any more (the file target is handling them and they
never get to the process target). So when you push a file target,
it goes into the file stratum, which is always below the process
- stratum. */
+ stratum.
+
+ Note that rather than allow an empty stack, we always have the
+ dummy target at the bottom stratum, so we can call the function
+ vectors without checking them. */
#include "target/target.h"
#include "target/resume.h"
@@ -425,7 +429,6 @@ struct target_info
struct target_ops
{
/* To the target under this one. */
- target_ops *m_beneath;
target_ops *beneath () const;
/* Free resources associated with the target. Note that singleton
@@ -1268,6 +1271,46 @@ extern void set_native_target (target_ops *target);
NULL. */
extern target_ops *get_native_target ();
+/* Type that manages a target stack. See description of target stacks
+ and strata at the top of the file. */
+
+class target_stack
+{
+public:
+ target_stack () = default;
+ DISABLE_COPY_AND_ASSIGN (target_stack);
+
+ /* Push a new target into the stack of the existing target
+ accessors, possibly superseding some existing accessor. */
+ void push (target_ops *t);
+
+ /* Remove a target from the stack, wherever it may be. Return true
+ if it was removed, false otherwise. */
+ bool unpush (target_ops *t);
+
+ /* Returns true if T is pushed on the target stack. */
+ bool is_pushed (target_ops *t) const
+ { return at (t->to_stratum) == t; }
+
+ /* Return the target at STRATUM. */
+ target_ops *at (strata stratum) const { return m_stack[stratum]; }
+
+ /* Return the target at the top of the stack. */
+ target_ops *top () const { return at (m_top); }
+
+ /* Find the next target down the stack from the specified target. */
+ target_ops *find_beneath (const target_ops *t) const;
+
+private:
+ /* The stratum of the top target target. */
+ enum strata m_top {};
+
+ /* The stack, represented as an array, with one slot per stratum.
+ If no target is pushed at some stratum, the corresponding slot is
+ null. */
+ target_ops *m_stack[(int) debug_stratum + 1] {};
+};
+
/* The ops structure for our "current" target process. This should
never be NULL. If there is no target, it points to the dummy_target. */