[1/3] Pass inferior to terminal_save_inferior

  Instead of relying on the current inferior, pass an inferior pointer to
the target implementing terminal_save_inferior.  There should be no
change in behavior.

I added documentation to terminal_save_inferior, as I understand it
(maybe I understood it wrong, so please take a look).

gdb/ChangeLog:

	* target.h (struct target_ops) <terminal_save_inferior>: Add
	inferior pointer parameter.
	* target.c (target_terminal_is_ours_kind): Pass inferior to
	terminal_save_inferior instead of changing current inferior.
	* target-delegates.c: Re-generate.
	* inf-child.h (inf_child_target) <terminal_save_inferior>: Add
	inferior pointer parameter.
	* inf-child.c (inf_child_target::terminal_save_inferior):
	Likewise.
	* inferior.h (child_terminal_save_inferior): Likewise.
	* inflow.c (child_terminal_save_inferior): Likewise.
---
 gdb/inf-child.c        |  4 ++--
 gdb/inf-child.h        |  2 +-
 gdb/inferior.h         |  3 ++-
 gdb/inflow.c           |  3 +--
 gdb/target-delegates.c | 15 ++++++++-------
 gdb/target.c           |  5 +----
 gdb/target.h           |  5 ++++-
 7 files changed, 19 insertions(+), 18 deletions(-)
  

On 10/16/2018 04:38 AM, Simon Marchi wrote:
> Instead of relying on the current inferior, pass an inferior pointer to
> the target implementing terminal_save_inferior.  There should be no
> change in behavior.
> 

Your original patch 3/3 ended up not using the inferior pointer.  :-)

I suppose that this doesn't hurt, like the equivalent target_detach
change.

> I added documentation to terminal_save_inferior, as I understand it
> (maybe I understood it wrong, so please take a look).

That looks good.  (please recall to update commit log.)

> diff --git a/gdb/target.c b/gdb/target.c
> index 2d98954b54ac..93d16b90f179 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -511,10 +511,7 @@ target_terminal_is_ours_kind (target_terminal_state desired_state)
>    ALL_INFERIORS (inf)
>      {
>        if (inf->terminal_state == target_terminal_state::is_inferior)
> -	{
> -	  set_current_inferior (inf);
> -	  current_top_target ()->terminal_save_inferior ();
> -	}
> +	current_top_target ()->terminal_save_inferior (inf);
>      }
With multi-target, current_top_target() will depend on 
the current inferior, so I'll need to put that
set_current_inferior call back.

Thanks,
Pedro Alves
  

On 2018-10-22 16:54, Pedro Alves wrote:
> On 10/16/2018 04:38 AM, Simon Marchi wrote:
>> Instead of relying on the current inferior, pass an inferior pointer 
>> to
>> the target implementing terminal_save_inferior.  There should be no
>> change in behavior.
>> 
> 
> Your original patch 3/3 ended up not using the inferior pointer.  :-)
> 
> I suppose that this doesn't hurt, like the equivalent target_detach
> change.

Well, the original 3/3 patch doesn't change the implementation of 
terminal_save_inferior, it changes the inferior_exit observer, so it's 
not really related to this change.

>> I added documentation to terminal_save_inferior, as I understand it
>> (maybe I understood it wrong, so please take a look).
> 
> That looks good.  (please recall to update commit log.)
> 
>> diff --git a/gdb/target.c b/gdb/target.c
>> index 2d98954b54ac..93d16b90f179 100644
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -511,10 +511,7 @@ target_terminal_is_ours_kind 
>> (target_terminal_state desired_state)
>>    ALL_INFERIORS (inf)
>>      {
>>        if (inf->terminal_state == target_terminal_state::is_inferior)
>> -	{
>> -	  set_current_inferior (inf);
>> -	  current_top_target ()->terminal_save_inferior ();
>> -	}
>> +	current_top_target ()->terminal_save_inferior (inf);
>>      }
> With multi-target, current_top_target() will depend on
> the current inferior, so I'll need to put that
> set_current_inferior call back.

Can't you access the inferior's target stack directly instead of 
changing the current inferior?

Simon
  

On 10/23/2018 10:00 AM, Simon Marchi wrote:
> On 2018-10-22 16:54, Pedro Alves wrote:
>> On 10/16/2018 04:38 AM, Simon Marchi wrote:
>>> Instead of relying on the current inferior, pass an inferior pointer to
>>> the target implementing terminal_save_inferior.  There should be no
>>> change in behavior.
>>>
>>
>> Your original patch 3/3 ended up not using the inferior pointer.  :-)
>>
>> I suppose that this doesn't hurt, like the equivalent target_detach
>> change.
> 
> Well, the original 3/3 patch doesn't change the implementation of terminal_save_inferior, it changes the inferior_exit observer, so it's not really related to this change.

Oh, in that case it'd be clearer to not bundle it in the same series then.

> 
>>> I added documentation to terminal_save_inferior, as I understand it
>>> (maybe I understood it wrong, so please take a look).
>>
>> That looks good.  (please recall to update commit log.)
>>
>>> diff --git a/gdb/target.c b/gdb/target.c
>>> index 2d98954b54ac..93d16b90f179 100644
>>> --- a/gdb/target.c
>>> +++ b/gdb/target.c
>>> @@ -511,10 +511,7 @@ target_terminal_is_ours_kind (target_terminal_state desired_state)
>>>    ALL_INFERIORS (inf)
>>>      {
>>>        if (inf->terminal_state == target_terminal_state::is_inferior)
>>> -    {
>>> -      set_current_inferior (inf);
>>> -      current_top_target ()->terminal_save_inferior ();
>>> -    }
>>> +    current_top_target ()->terminal_save_inferior (inf);
>>>      }
>> With multi-target, current_top_target() will depend on
>> the current inferior, so I'll need to put that
>> set_current_inferior call back.
> 
> Can't you access the inferior's target stack directly instead of changing the current inferior?

I can for the call to the top target, but then the problem is the beneath()
calls in all the target-delegates.c delegating implementations.  E.g. here:

  void
 -target_ops::terminal_save_inferior ()
 +target_ops::terminal_save_inferior (inferior *arg0)
  {
 -  this->beneath ()->terminal_save_inferior ();
 +  this->beneath ()->terminal_save_inferior (arg0);
          ^^^^^^^^^
  }

because beneath() looks at the target stack of the current
inferior.  It would need to look for the target beneath in
the target stack of the arg0 inferior instead.  Otherwise
you start the top target call in inferior B, and then
cross the the beneath target of inferior A (the current inferior).
Whoops.  At some point in the branch I made target_ops::beneath
take an optional inferior pointer, but when I stumbled on this
issue in target-delegates.c I ended up reverting it, as it
wasn't easy to fix.  I think that we could maybe teach
make-target-delegates to automatically emit

  void
  target_ops::method (inferior *arg0)
  {
    this->beneath (arg0)->method (arg0);
  }

and:

  void
  target_ops::method (thread_info *arg0)
  {
    this->beneath (arg0->inf)->method (arg0);
  }

iff the method's first parameter is an inferior or thread_info
pointer.  But that was just an idea, I never toyed with it,
because it would be a detour.  So I gave up on the inferior
parameter to target beneath, and thought I'd better focus instead
of getting the multi-target basics in first, even if that means we
need to swap current inferior/thread here and there, as usual.

Thanks,
Pedro Alves
  

On 2018-10-23 7:11 a.m., Pedro Alves wrote:
> Oh, in that case it'd be clearer to not bundle it in the same series then.

Right, I happened to write it at the same time (when trying to figure out
how things worked) but it's not actually related.

>> Can't you access the inferior's target stack directly instead of changing the current inferior?
> 
> I can for the call to the top target, but then the problem is the beneath()
> calls in all the target-delegates.c delegating implementations.  E.g. here:
> 
>   void
>  -target_ops::terminal_save_inferior ()
>  +target_ops::terminal_save_inferior (inferior *arg0)
>   {
>  -  this->beneath ()->terminal_save_inferior ();
>  +  this->beneath ()->terminal_save_inferior (arg0);
>           ^^^^^^^^^
>   }
> 
> because beneath() looks at the target stack of the current
> inferior.  It would need to look for the target beneath in
> the target stack of the arg0 inferior instead.  Otherwise
> you start the top target call in inferior B, and then
> cross the the beneath target of inferior A (the current inferior).
> Whoops.  At some point in the branch I made target_ops::beneath
> take an optional inferior pointer, but when I stumbled on this
> issue in target-delegates.c I ended up reverting it, as it
> wasn't easy to fix.  I think that we could maybe teach
> make-target-delegates to automatically emit
> 
>   void
>   target_ops::method (inferior *arg0)
>   {
>     this->beneath (arg0)->method (arg0);
>   }
> 
> and:
> 
>   void
>   target_ops::method (thread_info *arg0)
>   {
>     this->beneath (arg0->inf)->method (arg0);
>   }
> 
> iff the method's first parameter is an inferior or thread_info
> pointer.  But that was just an idea, I never toyed with it,
> because it would be a detour.  So I gave up on the inferior
> parameter to target beneath, and thought I'd better focus instead
> of getting the multi-target basics in first, even if that means we
> need to swap current inferior/thread here and there, as usual.

Ok I understand.  At some point I think it would be very nice if it
"just worked", but I totally understand the need to go by small
increment.  In that case, I think I'll drop patches 1 and 2, since
they don't really provide any benefit at the moment, and just cause
complications going further.

Simon