[patch/rfc] Simplify target stack

[patch/rfc] Simplify target stack

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

Hello,

The attached patch simplifies the target-stack by folding the "struct 
target_stack_item" into the "struct target_ops".  The field "struct 
target_ops . beneath" being added.

This in turn greatly simplifies the logic needed to walk the target 
stack (target_beneath becomes a one-liner), and that in turn lets me 
correctly implement the new target read/write partial methods I just posted.

Note that this implementation is still limited to a single target stack 
(due to all the target_ops vectors being static).  Follow-on changes can 
eliminate that restriction.

Once I've finished testing, I'll look to commit it in a day or so,
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 7342 bytes --]

2003-10-15  Andrew Cagney  <cagney@redhat.com>

	* target.c (target_stack): Change to a static target_ops.
	(update_current_target): Walk the "struct target_ops" stack.
	(pop_target, do_xfer_memory, target_info): Ditto.
	(find_target_beneath): Ditto.
	(push_target): Rewrite to use the "struct target_ops" stack.
	(unpush_target): Ditto.
	* target.h (struct target_stack_item): Delete definition.
	(target_stack): Delete declaration.
	(struct target_ops): Add field "beneath".

Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.55
diff -u -r1.55 target.c
--- target.c	2 Oct 2003 20:28:30 -0000	1.55
+++ target.c	15 Oct 2003 22:17:46 -0000
@@ -180,7 +180,7 @@
 
 /* Top of target stack.  */
 
-struct target_stack_item *target_stack;
+static struct target_ops *target_stack;
 
 /* The target structure we are currently using to talk to a process
    or file or whatever "inferior" we have.  */
@@ -534,15 +534,13 @@
 static void
 update_current_target (void)
 {
-  struct target_stack_item *item;
   struct target_ops *t;
 
   /* First, reset current_target */
   memset (&current_target, 0, sizeof current_target);
 
-  for (item = target_stack; item; item = item->next)
+  for (t = target_stack; t; t = t->beneath)
     {
-      t = item->target_ops;
 
 #define INHERIT(FIELD, TARGET) \
       if (!current_target.FIELD) \
@@ -645,7 +643,7 @@
 int
 push_target (struct target_ops *t)
 {
-  struct target_stack_item *cur, *prev, *tmp;
+  struct target_ops **cur;
 
   /* Check magic number.  If wrong, it probably means someone changed
      the struct definition, but not all the places that initialize one.  */
@@ -657,42 +655,30 @@
       internal_error (__FILE__, __LINE__, "failed internal consistency check");
     }
 
-  /* Find the proper stratum to install this target in. */
-
-  for (prev = NULL, cur = target_stack; cur; prev = cur, cur = cur->next)
+  /* Find the proper stratum to install this target in.  */
+  for (cur = &target_stack; (*cur) != NULL; cur = &(*cur)->beneath)
     {
-      if ((int) (t->to_stratum) >= (int) (cur->target_ops->to_stratum))
+      if ((int) (t->to_stratum) >= (int) (*cur)->to_stratum)
 	break;
     }
 
-  /* If there's already targets at this stratum, remove them. */
-
-  if (cur)
-    while (t->to_stratum == cur->target_ops->to_stratum)
-      {
-	/* There's already something on this stratum.  Close it off.  */
-	if (cur->target_ops->to_close)
-	  (cur->target_ops->to_close) (0);
-	if (prev)
-	  prev->next = cur->next;	/* Unchain old target_ops */
-	else
-	  target_stack = cur->next;	/* Unchain first on list */
-	tmp = cur->next;
-	xfree (cur);
-	cur = tmp;
-      }
+  /* If there's already targets at this stratum, remove them.  */
+  /* FIXME: cagney/2003-10-15: I think this should be poping 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);
+      (*cur) = (*cur)->beneath;
+      tmp->beneath = NULL;
+      if (tmp->to_close)
+	(tmp->to_close) (0);
+    }
 
   /* We have removed all targets in our stratum, now add the new one.  */
-
-  tmp = (struct target_stack_item *)
-    xmalloc (sizeof (struct target_stack_item));
-  tmp->next = cur;
-  tmp->target_ops = t;
-
-  if (prev)
-    prev->next = tmp;
-  else
-    target_stack = tmp;
+  t->beneath = (*cur);
+  (*cur) = t;
 
   update_current_target ();
 
@@ -701,7 +687,8 @@
   if (targetdebug)
     setup_target_debug ();
 
-  return prev != 0;
+  /* On top?  */
+  return (t == target_stack);
 }
 
 /* Remove a target_ops vector from the stack, wherever it may be. 
@@ -710,7 +697,8 @@
 int
 unpush_target (struct target_ops *t)
 {
-  struct target_stack_item *cur, *prev;
+  struct target_ops **cur;
+  struct target_ops *tmp;
 
   if (t->to_close)
     t->to_close (0);		/* Let it clean up */
@@ -718,21 +706,19 @@
   /* Look for the specified target.  Note that we assume that a target
      can only occur once in the target stack. */
 
-  for (cur = target_stack, prev = NULL; cur; prev = cur, cur = cur->next)
-    if (cur->target_ops == t)
-      break;
+  for (cur = &target_stack; (*cur) != NULL; cur = &(*cur)->beneath)
+    {
+      if ((*cur) == t)
+	break;
+    }
 
-  if (!cur)
+  if ((*cur) == NULL)
     return 0;			/* Didn't find target_ops, quit now */
 
   /* Unchain the target */
-
-  if (!prev)
-    target_stack = cur->next;
-  else
-    prev->next = cur->next;
-
-  xfree (cur);			/* Release the target_stack_item */
+  tmp = (*cur);
+  (*cur) = (*cur)->beneath;
+  tmp->beneath = NULL;
 
   update_current_target ();
   cleanup_target (&current_target);
@@ -744,7 +730,7 @@
 pop_target (void)
 {
   (current_target.to_close) (0);	/* Let it clean up */
-  if (unpush_target (target_stack->target_ops) == 1)
+  if (unpush_target (target_stack) == 1)
     return;
 
   fprintf_unfiltered (gdb_stderr,
@@ -865,7 +851,6 @@
   int res;
   int done = 0;
   struct target_ops *t;
-  struct target_stack_item *item;
 
   /* Zero length requests are ok and require no work.  */
   if (len == 0)
@@ -902,9 +887,8 @@
   /* If res <= 0 then we call it again in the loop.  Ah well. */
   if (res <= 0)
     {
-      for (item = target_stack; item; item = item->next)
+      for (t = target_stack; t != NULL; t = t->beneath)
 	{
-	  t = item->target_ops;
 	  if (!t->to_has_memory)
 	    continue;
 
@@ -1076,7 +1060,6 @@
 target_info (char *args, int from_tty)
 {
   struct target_ops *t;
-  struct target_stack_item *item;
   int has_all_mem = 0;
 
   if (symfile_objfile != NULL)
@@ -1087,10 +1070,8 @@
     return;
 #endif
 
-  for (item = target_stack; item; item = item->next)
+  for (t = target_stack; t != NULL; t = t->beneath)
     {
-      t = item->target_ops;
-
       if (!t->to_has_memory)
 	continue;
 
@@ -1385,16 +1366,7 @@
 struct target_ops *
 find_target_beneath (struct target_ops *t)
 {
-  struct target_stack_item *cur;
-
-  for (cur = target_stack; cur; cur = cur->next)
-    if (cur->target_ops == t)
-      break;
-
-  if (cur == NULL || cur->next == NULL)
-    return NULL;
-  else
-    return cur->next->target_ops;
+  return t->beneath;
 }
 
 \f
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.42
diff -u -r1.42 target.h
--- target.h	15 Oct 2003 21:10:55 -0000	1.42
+++ target.h	15 Oct 2003 22:17:47 -0000
@@ -188,6 +188,7 @@
 
 struct target_ops
   {
+    struct target_ops *beneath;	/* To the target under this one.  */
     char *to_shortname;		/* Name this target type */
     char *to_longname;		/* Name for printing */
     char *to_doc;		/* Documentation.  Does not include trailing
@@ -325,18 +326,6 @@
    never be NULL.  If there is no target, it points to the dummy_target.  */
 
 extern struct target_ops current_target;
-
-/* An item on the target stack.  */
-
-struct target_stack_item
-  {
-    struct target_stack_item *next;
-    struct target_ops *target_ops;
-  };
-
-/* The target stack.  */
-
-extern struct target_stack_item *target_stack;
 
 /* Define easy words for doing these operations on our current target.  */
 

Re: [patch/rfc] Simplify target stack

[Daniel Jacobowitz]

On Wed, Oct 15, 2003 at 06:37:46PM -0400, Andrew Cagney wrote:
> Hello,
> 
> The attached patch simplifies the target-stack by folding the "struct 
> target_stack_item" into the "struct target_ops".  The field "struct 
> target_ops . beneath" being added.
> 
> This in turn greatly simplifies the logic needed to walk the target 
> stack (target_beneath becomes a one-liner), and that in turn lets me 
> correctly implement the new target read/write partial methods I just posted.
> 
> Note that this implementation is still limited to a single target stack 
> (due to all the target_ops vectors being static).  Follow-on changes can 
> eliminate that restriction.
> 
> Once I've finished testing, I'll look to commit it in a day or so,
> Andrew

You're moving beneath into target_ops, but aren't you going to have to
either move it out again or move everything else from target_ops?  It
seems to me that we want the method vector to be constant eventually
(kill the INHERIT mess), but the target to have local data.  Just seems
like this is happening in the wrong order.  Another way would be:
  - rename target_ops and target_item
  - make access to target go through the renamed version of target_item
  - add a target_data member to the renamed target_data

Other than that seems good.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Simplify target stack

[Andrew Cagney]

> On Wed, Oct 15, 2003 at 06:37:46PM -0400, Andrew Cagney wrote:
> 
>> Hello,
>> 
>> The attached patch simplifies the target-stack by folding the "struct 
>> target_stack_item" into the "struct target_ops".  The field "struct 
>> target_ops . beneath" being added.
>> 
>> This in turn greatly simplifies the logic needed to walk the target 
>> stack (target_beneath becomes a one-liner), and that in turn lets me 
>> correctly implement the new target read/write partial methods I just posted.
>> 
>> Note that this implementation is still limited to a single target stack 
>> (due to all the target_ops vectors being static).  Follow-on changes can 
>> eliminate that restriction.
>> 
>> Once I've finished testing, I'll look to commit it in a day or so,
>> Andrew
> 
> 
> You're moving beneath into target_ops, but aren't you going to have to
> either move it out again or move everything else from target_ops?  It
> seems to me that we want the method vector to be constant eventually
> (kill the INHERIT mess), but the target to have local data.  Just seems
> like this is happening in the wrong order.  Another way would be:
>   - rename target_ops and target_item
>   - make access to target go through the renamed version of target_item
>   - add a target_data member to the renamed target_data

I believe that the objectives here are:

1. being able to directly walk the target chain
Makes it possible to eliminate the INHERIT mess.  Lets targets 
efficiently/directly interact with target-beneath.

2. allow multiple instances of a specific target
So that more than one target stack is possible.

3. strict separation of target instance and target ops
See below.

In terms of priority, I rank them as above.

My immediate objective is the first one - make the target stack directly 
accessible.  That way I can implement target read/write partial in a 
relatively clean way (the INHERIT in the current patch can go).  It also 
comes with significant bang for the buck - it opens the way for an 
incremental cleanup of the target vectors.

The second objective has to be considered medium term.  Any direct 
references to static variables, and the currently static target ops will 
need to go.  Fortunatly, it can be done on an as-needed basis.

This change can be implemented out without worrying about the third 
objective - just clone the existing registered target vector when a new 
instance is required.

For the last one, some background is needed.  The original target code 
implemented the target ops using completly static structures 
Unfortunatly this proved to be un-maintainable - adding and removing 
fields broke things - and was replaced by runtime initialization (this 
is also one of the reasons behind the architecture vector using run-time 
initialization).   While the code migh eventually be massaged into 
run-time initializing a separate "struct target_ops", I don't think its 
exactly urgent.

enjoy,
Andrew

Re: [patch/rfc] Simplify target stack

[Daniel Jacobowitz]

On Thu, Oct 16, 2003 at 11:27:52AM -0400, Andrew Cagney wrote:
> >On Wed, Oct 15, 2003 at 06:37:46PM -0400, Andrew Cagney wrote:
> >
> >>Hello,
> >>
> >>The attached patch simplifies the target-stack by folding the "struct 
> >>target_stack_item" into the "struct target_ops".  The field "struct 
> >>target_ops . beneath" being added.
> >>
> >>This in turn greatly simplifies the logic needed to walk the target 
> >>stack (target_beneath becomes a one-liner), and that in turn lets me 
> >>correctly implement the new target read/write partial methods I just 
> >>posted.
> >>
> >>Note that this implementation is still limited to a single target stack 
> >>(due to all the target_ops vectors being static).  Follow-on changes can 
> >>eliminate that restriction.
> >>
> >>Once I've finished testing, I'll look to commit it in a day or so,
> >>Andrew
> >
> >
> >You're moving beneath into target_ops, but aren't you going to have to
> >either move it out again or move everything else from target_ops?  It
> >seems to me that we want the method vector to be constant eventually
> >(kill the INHERIT mess), but the target to have local data.  Just seems
> >like this is happening in the wrong order.  Another way would be:
> >  - rename target_ops and target_item
> >  - make access to target go through the renamed version of target_item
> >  - add a target_data member to the renamed target_data
> 
> I believe that the objectives here are:
> 
> 1. being able to directly walk the target chain
> Makes it possible to eliminate the INHERIT mess.  Lets targets 
> efficiently/directly interact with target-beneath.
> 
> 2. allow multiple instances of a specific target
> So that more than one target stack is possible.
> 
> 3. strict separation of target instance and target ops
> See below.
> 
> In terms of priority, I rank them as above.

But we already _have_ a separation of target instance and target ops. 
It's struct target_stack_item.  It's your cleanup right there, waiting
to happen.  Removing it is not a step forwards; you can just change to
passing that item around instead of struct target_ops.  If you want a
different name, rename it.  Not everywhere will need to be converted,
obviously - only things which want the new data.

> My immediate objective is the first one - make the target stack directly 
> accessible.  That way I can implement target read/write partial in a 
> relatively clean way (the INHERIT in the current patch can go).  It also 
> comes with significant bang for the buck - it opens the way for an 
> incremental cleanup of the target vectors.
> 
> The second objective has to be considered medium term.  Any direct 
> references to static variables, and the currently static target ops will 
> need to go.  Fortunatly, it can be done on an as-needed basis.
> 
> This change can be implemented out without worrying about the third 
> objective - just clone the existing registered target vector when a new 
> instance is required.
> 
> For the last one, some background is needed.  The original target code 
> implemented the target ops using completly static structures 
> Unfortunatly this proved to be un-maintainable - adding and removing 
> fields broke things - and was replaced by runtime initialization (this 
> is also one of the reasons behind the architecture vector using run-time 
> initialization).   While the code migh eventually be massaged into 
> run-time initializing a separate "struct target_ops", I don't think its 
> exactly urgent.

I'm not sure what you mean by run-time initialization in this context,
since we already have run-time initializers for struct target_ops -
they initialize the static structures right now.  Almost (not quite,
but pretty close) to everything in that initialized structure is ops. 
Eventually, with low urgency, the non-ops should be moved out of it. 
But for now that's no excuse to move things into it that you will have
to move out of it again.

Just because you can avoid doing it now doesn't mean that's OK.  You
tell that to other developers at every opportunity.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Simplify target stack

[Andrew Cagney]

> I believe that the objectives here are:
>> 
>> 1. being able to directly walk the target chain
>> Makes it possible to eliminate the INHERIT mess.  Lets targets 
>> efficiently/directly interact with target-beneath.
>> 
>> 2. allow multiple instances of a specific target
>> So that more than one target stack is possible.
>> 
>> 3. strict separation of target instance and target ops
>> See below.
>> 
>> In terms of priority, I rank them as above.
> 
> 
> But we already _have_ a separation of target instance and target ops. 
> It's struct target_stack_item.  It's your cleanup right there, waiting
> to happen.  Removing it is not a step forwards; you can just change to
> passing that item around instead of struct target_ops.  If you want a
> different name, rename it.  Not everywhere will need to be converted,
> obviously - only things which want the new data.

What you're casually dismissing as trivial: "Not everywhere will need to 
be converted" and "Eventually, with low urgency, the non-ops should be 
moved out of it" are exactly the things I also need *now*.

Given this, folding the two structures into-one provides me with the 
shortest path to this objective.

> Just because you can avoid doing it now doesn't mean that's OK.  You
> tell that to other developers at every opportunity.

And given a set of alternatives I'll take the one with the greatest bang 
for the buck.

Andrew

Re: [patch/rfc] Simplify target stack

[Andrew Cagney]

> 2003-10-15  Andrew Cagney  <cagney@redhat.com>
> 
> 	* target.c (target_stack): Change to a static target_ops.
> 	(update_current_target): Walk the "struct target_ops" stack.
> 	(pop_target, do_xfer_memory, target_info): Ditto.
> 	(find_target_beneath): Ditto.
> 	(push_target): Rewrite to use the "struct target_ops" stack.
> 	(unpush_target): Ditto.
> 	* target.h (struct target_stack_item): Delete definition.
> 	(target_stack): Delete declaration.
> 	(struct target_ops): Add field "beneath".
> 
I've checked this in.

Andrew

Re: [patch/rfc] Simplify target stack

[Daniel Jacobowitz]

On Thu, Oct 16, 2003 at 08:21:36PM -0400, Andrew Cagney wrote:
> >I believe that the objectives here are:
> >>
> >>1. being able to directly walk the target chain
> >>Makes it possible to eliminate the INHERIT mess.  Lets targets 
> >>efficiently/directly interact with target-beneath.
> >>
> >>2. allow multiple instances of a specific target
> >>So that more than one target stack is possible.
> >>
> >>3. strict separation of target instance and target ops
> >>See below.
> >>
> >>In terms of priority, I rank them as above.
> >
> >
> >But we already _have_ a separation of target instance and target ops. 
> >It's struct target_stack_item.  It's your cleanup right there, waiting
> >to happen.  Removing it is not a step forwards; you can just change to
> >passing that item around instead of struct target_ops.  If you want a
> >different name, rename it.  Not everywhere will need to be converted,
> >obviously - only things which want the new data.
> 
> What you're casually dismissing as trivial: "Not everywhere will need to 
> be converted" and "Eventually, with low urgency, the non-ops should be 
> moved out of it" are exactly the things I also need *now*.
> 
> Given this, folding the two structures into-one provides me with the 
> shortest path to this objective.
> 
> >Just because you can avoid doing it now doesn't mean that's OK.  You
> >tell that to other developers at every opportunity.
> 
> And given a set of alternatives I'll take the one with the greatest bang 
> for the buck.

Please don't pretend I'm the only one on this list who has asked for a
contributor to take the longer way to a goal, in light of later
cleanups.  This is standard practice for keeping the code maintainable,
and evolving towards improved organization.

I see that you've checked this patch in despite my objections.  Please
let me know when you're done with modifying the target vector for a few
days and I'll just separate instance and ops myself, including moving
the new to_beneath and to_data fields out of struct target_ops.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Simplify target stack

[Andrew Cagney]

>> > Just because you can avoid doing it now doesn't mean that's OK.  You
>> >tell that to other developers at every opportunity.
> 
>> 
>> And given a set of alternatives I'll take the one with the greatest bang 
>> for the buck.
> 
> 
> Please don't pretend I'm the only one on this list who has asked for a
> contributor to take the longer way to a goal, in light of later
> cleanups.

I'm not.  I try to apply the `bang for the buck' rule evenly to both 
myself and to others.

For instance, given the choice of asking JeffJ to add a new 
LESS_THAN_SPECIAL architecture method (+ test + doco + ...) xor just 
tighten comments in frame.h, I chose the latter.  A full analysis of the 
problem revealed that the former and zero bang for the buck!

> This is standard practice for keeping the code maintainable,
> and evolving towards improved organization.

There's a balance.  Er to much towards features and code quality goes 
down.  Er to much towards structural perfection, and new features are 
never added.

You'll note that so far, as part of getting PPC64 linux working, I've 
also: fixed struct return, and elimininated the need for a redundant 
architecture method.

> I see that you've checked this patch in despite my objections.

I thought that I had clearly stated the rationale for ordering the 
development the way I had.  That left me, as target vector maintainer, 
and the person willing to do the immediate work, with a judgment call.

> Please
> let me know when you're done with modifying the target vector for a few
> days and I'll just separate instance and ops myself, including moving
> the new to_beneath and to_data fields out of struct target_ops.

It will be a lot more than a few days :-(  And if you wait long enough, 
I'll likely to do it myself!

Andrew