Re: [PATCH 1/3] Pass inferior to terminal_save_inferior

[Pedro Alves <palves@redhat.com>]

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