[RFA] Remove unnecessary target defaults.

[RFA] Remove unnecessary target defaults.

[Vladimir Prus]

In current GDB, at the bottom of the target stack, we always have dummy
target, which defines some target methods, including to_create_inferiour,
and to_attach. Those are implemented by calling find_default_attach
and find_default_create_inferior, which will call 'error' when nothing
is found.

On the other hand, update_current_target tries to set a default value
for those functions -- but because dummy target is always there, such
defaulting is not necessary. This patch removes it. No regressions on x86.

OK?

- Volodya

	* target.c (maybe_kill_then_attach)
	(maybe_kill_then_create_inferior): Remove.
	(update_current_target): Do not default to_attach,
	to_create_inferiour, to_is_async_p.
---
 gdb/target.c |   27 ---------------------------
 1 files changed, 0 insertions(+), 27 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index 0dbe49b..050f71b 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -44,8 +44,6 @@
 
 static void target_info (char *, int);
 
-static void maybe_kill_then_attach (char *, int);
-
 static void kill_or_be_killed (int);
 
 static void default_terminal_info (char *, int);
@@ -358,21 +356,6 @@ kill_or_be_killed (int from_tty)
   tcomplain ();
 }
 
-static void
-maybe_kill_then_attach (char *args, int from_tty)
-{
-  kill_or_be_killed (from_tty);
-  target_attach (args, from_tty);
-}
-
-static void
-maybe_kill_then_create_inferior (char *exec, char *args, char **env,
-				 int from_tty)
-{
-  kill_or_be_killed (0);
-  target_create_inferior (exec, args, env, from_tty);
-}
-
 /* Go through the target stack from top to bottom, copying over zero
    entries in current_target, then filling in still empty entries.  In
    effect, we are doing class inheritance through the pushed target
@@ -500,8 +483,6 @@ update_current_target (void)
   de_fault (to_close,
 	    (void (*) (int))
 	    target_ignore);
-  de_fault (to_attach,
-	    maybe_kill_then_attach);
   de_fault (to_post_attach,
 	    (void (*) (int))
 	    target_ignore);
@@ -584,8 +565,6 @@ update_current_target (void)
   de_fault (to_lookup_symbol,
 	    (int (*) (char *, CORE_ADDR *))
 	    nosymbol);
-  de_fault (to_create_inferior,
-	    maybe_kill_then_create_inferior);
   de_fault (to_post_startup_inferior,
 	    (void (*) (ptid_t))
 	    target_ignore);
@@ -640,12 +619,6 @@ update_current_target (void)
   de_fault (to_pid_to_exec_file,
 	    (char *(*) (int))
 	    return_zero);
-  de_fault (to_can_async_p,
-	    (int (*) (void))
-	    return_zero);
-  de_fault (to_is_async_p,
-	    (int (*) (void))
-	    return_zero);
   de_fault (to_async,
 	    (void (*) (void (*) (enum inferior_event_type, void*), void*))
 	    tcomplain);
-- 
1.5.3.5

Re: [RFA] Remove unnecessary target defaults.

[Pedro Alves]

On Friday 15 August 2008 13:45:30, Vladimir Prus wrote:

> -  de_fault (to_can_async_p,
> -	    (int (*) (void))
> -	    return_zero);
> -  de_fault (to_is_async_p,
> -	    (int (*) (void))
> -	    return_zero);

I believe this is incorrect.  You don't notice it when connected
to remote, because those methods are implemented in remote_ops.  But,
when connected to e.g., remote-sim.c, and because gdbsim_ops doesn't define
those, you'll inherit the dummy target's implementation, which looks
for asyncness in the default run target instead --- while you should be
looking for asyncness in the remote-sim target.

-- 
Pedro Alves

Re: [RFA] Remove unnecessary target defaults.

[Vladimir Prus]

On Friday 15 August 2008 17:08:42 Pedro Alves wrote:
> On Friday 15 August 2008 13:45:30, Vladimir Prus wrote:
> 
> > -  de_fault (to_can_async_p,
> > -	    (int (*) (void))
> > -	    return_zero);
> > -  de_fault (to_is_async_p,
> > -	    (int (*) (void))
> > -	    return_zero);
> 
> I believe this is incorrect.  You don't notice it when connected
> to remote, because those methods are implemented in remote_ops.  But,
> when connected to e.g., remote-sim.c, and because gdbsim_ops doesn't define
> those, you'll inherit the dummy target's implementation, which looks
> for asyncness in the default run target instead --- while you should be
> looking for asyncness in the remote-sim target.

As we seem to have agreed on IRC, this is pre-existing problem. We inherit
a method before applying defaults, so we'll always inherit dummy's version
of to_is_async_p, and the code I remove above will never manage to change
method to return_zero.

The problem you raise is real, however. I think one approach to solve it
is to just make remote-sim define to_can_async_p. More generic solution
would be to arrange so that if we have a target on process stratum, it
never goes to targets below for to_can_async_p. For example, if we have

   - exec
   - dummy

and you do run, then target_can_async_p will look at exec, then at dummy,
then find_default_can_async_p will return some results. But if we have


   - remote-sim
   - exec
   - dummy

then target_can_async_p will either invoke a method in remote-sim (if present),
or return 0.

How does this sound?

- Volodya

Re: [RFA] Remove unnecessary target defaults.

[Pedro Alves]

On Friday 15 August 2008 15:15:59, Vladimir Prus wrote:

> The problem you raise is real, however. I think one approach to solve it
> is to just make remote-sim define to_can_async_p. More generic solution
> would be to arrange so that if we have a target on process stratum, it
> never goes to targets below for to_can_async_p. For example, if we have
>
>    - exec
>    - dummy
>
> and you do run, then target_can_async_p will look at exec, then at dummy,
> then find_default_can_async_p will return some results. But if we have
>
>
>    - remote-sim
>    - exec
>    - dummy
>
> then target_can_async_p will either invoke a method in remote-sim (if
> present), or return 0.
>
> How does this sound?

It sounds sound to me.  This would be like a generalised version of
inf-child.c, from which all targets that can have execution
inherit.

-- 
Pedro Alves

Re: [RFA] Remove unnecessary target defaults.

[Vladimir Prus]

On Friday 15 August 2008 18:30:59 Pedro Alves wrote:
> On Friday 15 August 2008 15:15:59, Vladimir Prus wrote:
> 
> > The problem you raise is real, however. I think one approach to solve it
> > is to just make remote-sim define to_can_async_p. More generic solution
> > would be to arrange so that if we have a target on process stratum, it
> > never goes to targets below for to_can_async_p. For example, if we have
> >
> >    - exec
> >    - dummy
> >
> > and you do run, then target_can_async_p will look at exec, then at dummy,
> > then find_default_can_async_p will return some results. But if we have
> >
> >
> >    - remote-sim
> >    - exec
> >    - dummy
> >
> > then target_can_async_p will either invoke a method in remote-sim (if
> > present), or return 0.
> >
> > How does this sound?
> 
> It sounds sound to me.  This would be like a generalised version of
> inf-child.c, from which all targets that can have execution
> inherit.

Well, actually, I meant implementing target_can_async_p in such a way that it:

- iterates over all targets
- breaks after it examines a target at process stratum

Of course, we can also have a common target, but it would require manually adjusting
all target on process stratum.

- Volodya

Re: [RFA] Remove unnecessary target defaults.

[Pedro Alves]

On Friday 15 August 2008 15:40:53, Vladimir Prus wrote:

> > It sounds sound to me.  This would be like a generalised version of
> > inf-child.c, from which all targets that can have execution
> > inherit.
>
> Well, actually, I meant implementing target_can_async_p in such a way that
> it:
>
> - iterates over all targets
> - breaks after it examines a target at process stratum
>

Should work too...

> Of course, we can also have a common target, but it would require manually
> adjusting all target on process stratum.

... yeah.  I keep meaning to try adding a base target from which all
targets inherit, so we could perhaps get rid of this INHERIT + 
de_fault business, and I somehow thought you could read minds.  Well,
you do, but you set it to block dumb ideas.  :-)

-- 
Pedro Alves

Re: [RFA] Remove unnecessary target defaults.

[Vladimir Prus]

On Friday 15 August 2008 18:52:19 Pedro Alves wrote:
> On Friday 15 August 2008 15:40:53, Vladimir Prus wrote:
> 
> > > It sounds sound to me.  This would be like a generalised version of
> > > inf-child.c, from which all targets that can have execution
> > > inherit.
> >
> > Well, actually, I meant implementing target_can_async_p in such a way that
> > it:
> >
> > - iterates over all targets
> > - breaks after it examines a target at process stratum
> >
> 
> Should work too...
> 
> > Of course, we can also have a common target, but it would require manually
> > adjusting all target on process stratum.
> 
> ... yeah.  I keep meaning to try adding a base target from which all
> targets inherit, so we could perhaps get rid of this INHERIT + 
> de_fault business, and I somehow thought you could read minds.  Well,
> you do, but you set it to block dumb ideas.  :-)

In fact, I cannot shake the feeling that current target stack is several
designs lumped together, and it would be beneficial to switch to a more coherent
design, where, in particular:

- there's base target every other target derives from and most methods of the base
target forward to the target beneath
- dummy target catches all methods
- all INHERIT and de_fault code is removed
- all target_xxx methods (iterating over target stack) are removed, or make just call
current_target->xxx

But this is fairly large thing to do.

- Volodya

Re: [RFA] Remove unnecessary target defaults.

[Pedro Alves]

On Friday 15 August 2008 16:02:07, Vladimir Prus wrote:

> > ... yeah.  I keep meaning to try adding a base target from which all
> > targets inherit, so we could perhaps get rid of this INHERIT +
> > de_fault business, and I somehow thought you could read minds.  Well,
> > you do, but you set it to block dumb ideas.  :-)
>
> In fact, I cannot shake the feeling that current target stack is several
> designs lumped together, and it would be beneficial to switch to a more
> coherent design, where, in particular:
>
> - there's base target every other target derives from and most methods of
> the base target forward to the target beneath
> - dummy target catches all methods
> - all INHERIT and de_fault code is removed
> - all target_xxx methods (iterating over target stack) are removed, or make
> just call current_target->xxx
>
> But this is fairly large thing to do.
>

Eh, you *do* read minds!

-- 
Pedro Alves

Re: [RFA] Remove unnecessary target defaults.

[Daniel Jacobowitz]

On Fri, Aug 15, 2008 at 04:45:30PM +0400, Vladimir Prus wrote:
> 
> In current GDB, at the bottom of the target stack, we always have dummy
> target, which defines some target methods, including to_create_inferiour,
> and to_attach. Those are implemented by calling find_default_attach
> and find_default_create_inferior, which will call 'error' when nothing
> is found.
> 
> On the other hand, update_current_target tries to set a default value
> for those functions -- but because dummy target is always there, such
> defaulting is not necessary. This patch removes it. No regressions on x86.
> 
> OK?
> 
> - Volodya
> 
> 	* target.c (maybe_kill_then_attach)
> 	(maybe_kill_then_create_inferior): Remove.
> 	(update_current_target): Do not default to_attach,
> 	to_create_inferiour, to_is_async_p.

Based on the discussion, this is OK.

-- 
Daniel Jacobowitz
CodeSourcery