[1/3] Pass inferior to terminal_save_inferior

Message ID 20181016033835.17594-1-simon.marchi@polymtl.ca
State New
Headers show
Series
  • [1/3] Pass inferior to terminal_save_inferior
Related show

Commit Message

Simon Marchi Oct. 16, 2018, 3:38 a.m.
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(-)

-- 
2.19.1

Comments

Pedro Alves Oct. 22, 2018, 8:54 p.m. | #1
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
Simon Marchi Oct. 23, 2018, 9 a.m. | #2
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
Pedro Alves Oct. 23, 2018, 11:11 a.m. | #3
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
Simon Marchi Oct. 23, 2018, 8:56 p.m. | #4
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

Patch

diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index 44aa2f66fbfe..94f42ec2adc0 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -114,9 +114,9 @@  inf_child_target::terminal_inferior ()
 }
 
 void
-inf_child_target::terminal_save_inferior ()
+inf_child_target::terminal_save_inferior (inferior *inf)
 {
-  child_terminal_save_inferior (this);
+  child_terminal_save_inferior (this, inf);
 }
 
 void
diff --git a/gdb/inf-child.h b/gdb/inf-child.h
index 98969bc5fa31..b0625391ebb2 100644
--- a/gdb/inf-child.h
+++ b/gdb/inf-child.h
@@ -46,7 +46,7 @@  public:
   bool supports_terminal_ours () override;
   void terminal_init () override;
   void terminal_inferior () override;
-  void terminal_save_inferior () override;
+  void terminal_save_inferior (inferior *inf) override;
   void terminal_ours_for_output () override;
   void terminal_ours () override;
   void terminal_info (const char *, int) override;
diff --git a/gdb/inferior.h b/gdb/inferior.h
index af5e92019618..d09263e34bef 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -154,7 +154,8 @@  extern void child_terminal_ours_for_output (struct target_ops *self);
 
 extern void child_terminal_inferior (struct target_ops *self);
 
-extern void child_terminal_save_inferior (struct target_ops *self);
+extern void child_terminal_save_inferior (struct target_ops *self,
+					  inferior *inf);
 
 extern void child_terminal_init (struct target_ops *self);
 
diff --git a/gdb/inflow.c b/gdb/inflow.c
index caff64620709..372ef7844f01 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -454,13 +454,12 @@  child_terminal_ours (struct target_ops *self)
    cache.  */
 
 void
-child_terminal_save_inferior (struct target_ops *self)
+child_terminal_save_inferior (struct target_ops *self, inferior *inf)
 {
   /* Avoid attempting all the ioctl's when running in batch.  */
   if (!gdb_has_a_terminal ())
     return;
 
-  inferior *inf = current_inferior ();
   terminal_info *tinfo = get_inflow_inferior_data (inf);
 
   /* No need to save/restore if the inferior is not sharing GDB's
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 03136dfacf25..f2b352c44888 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -45,7 +45,7 @@  struct dummy_target : public target_ops
   bool supports_terminal_ours () override;
   void terminal_init () override;
   void terminal_inferior () override;
-  void terminal_save_inferior () override;
+  void terminal_save_inferior (inferior *arg0) override;
   void terminal_ours_for_output () override;
   void terminal_ours () override;
   void terminal_info (const char *arg0, int arg1) override;
@@ -212,7 +212,7 @@  struct debug_target : public target_ops
   bool supports_terminal_ours () override;
   void terminal_init () override;
   void terminal_inferior () override;
-  void terminal_save_inferior () override;
+  void terminal_save_inferior (inferior *arg0) override;
   void terminal_ours_for_output () override;
   void terminal_ours () override;
   void terminal_info (const char *arg0, int arg1) override;
@@ -1249,22 +1249,23 @@  debug_target::terminal_inferior ()
 }
 
 void
-target_ops::terminal_save_inferior ()
+target_ops::terminal_save_inferior (inferior *arg0)
 {
-  this->beneath ()->terminal_save_inferior ();
+  this->beneath ()->terminal_save_inferior (arg0);
 }
 
 void
-dummy_target::terminal_save_inferior ()
+dummy_target::terminal_save_inferior (inferior *arg0)
 {
 }
 
 void
-debug_target::terminal_save_inferior ()
+debug_target::terminal_save_inferior (inferior *arg0)
 {
   fprintf_unfiltered (gdb_stdlog, "-> %s->terminal_save_inferior (...)\n", this->beneath ()->shortname ());
-  this->beneath ()->terminal_save_inferior ();
+  this->beneath ()->terminal_save_inferior (arg0);
   fprintf_unfiltered (gdb_stdlog, "<- %s->terminal_save_inferior (", this->beneath ()->shortname ());
+  target_debug_print_inferior_p (arg0);
   fputs_unfiltered (")\n", gdb_stdlog);
 }
 
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);
     }
 
   ALL_INFERIORS (inf)
diff --git a/gdb/target.h b/gdb/target.h
index a3000c80c641..c37405205a0a 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -583,7 +583,10 @@  struct target_ops
       TARGET_DEFAULT_IGNORE ();
     virtual void terminal_inferior ()
       TARGET_DEFAULT_IGNORE ();
-    virtual void terminal_save_inferior ()
+
+    /* If INF shares a terminal with GDB, save the current terminal settings
+       in a data structure associated to INF.  */
+    virtual void terminal_save_inferior (inferior *inf)
       TARGET_DEFAULT_IGNORE ();
     virtual void terminal_ours_for_output ()
       TARGET_DEFAULT_IGNORE ();