Re: [RFA]: New function terminal_save_ours()

Re: [RFA]: New function terminal_save_ours()

[Andrew Cagney]

> Hi!
> 
> When using gdb-tui and switching to or leaving the TUI mode, the terminal
> settings are changed.  This is part of the curses management (endwin).
> 
> Gdb keeps in a static variable in inflow.c for the tty setting of himself.
> It switches it with the inferior terminal (terminal_inferior, terminal_ours).
> The gdb terminal settings are fetched only once, the first time it is
> used/checked (so we don't know when).
> 
> For the TUI to work correctly, it needs to update the gdb knowledge of
> its terminal.  The patch below proposes a new function `terminal_save_ours'
> which is called by TUI each time the TUI changes from mode;  that is
> after we enter in TUI mode (curses), and after we left the TUI mode.
> 
> Can you approve this patch ?


I wish it were this simple.

target_terminal_ours(), target_terminal_inferior() and 
target_terminal_ours_for_output() all go through the target vector.  The 
, er, obvious (?) thing to do would be to add 
target_terminal_save_our_tty_state(), or what ever, to that target 
vector.  I'm, to be honest, not sure if it is ``obvious'' or not.

Think about a remote target that has implemented console I/O.

When GDB hands off to the target it calls target_terminal_inferior(), 
that in turn causes remote.c to steal the input from GDB so that it can 
funnel all console input down to the remote target.  When the inferior 
stops, GDB calls target_terminal_ours(), and the target relinquishes 
that control.  At present, remote.c (and possibly all other remote 
targets) assumes that this operation doesn't require any get/set tty 
state manipulation.  I don't think the patch, as it currently stands 
addresses this.

How to best address it is where things get tricky.  I'm not sure if the 
targets should all be updated to do the set/get tty tweeking or if the 
set/get tty stuff should be brought closer to GDB and always done.

	Andrew


> Index: inflow.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/inflow.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 inflow.c
> --- inflow.c	2001/07/15 20:34:13	1.11
> +++ inflow.c	2001/07/19 22:48:49
> @@ -200,6 +200,23 @@ terminal_init_inferior_with_pgrp (int pg
>      }
>  }
>  
> +/* Save the terminal settings again.  This is necessary for the TUI
> +   when it switches to TUI or non-TUI mode;  curses changes the terminal
> +   and gdb must be able to restore it correctly.  */
> +
> +void
> +terminal_save_ours ()


I'd recommend a more explicit name - ..._save_our_state?

> +{
> +  if (gdb_has_a_terminal ())
> +    {
> +      /* We could just as well copy our_ttystate (if we felt like adding
> +         a new function serial_copy_tty_state).  */
> +      if (our_ttystate)
> +	xfree (our_ttystate);
> +      our_ttystate = serial_get_tty_state (stdin_serial);
> +    }
> +}


Should all existing ``our_ttystate = serial_get_tty_state()'' calls be 
replaced with calls to this function?

	Andrew

Re: [RFA]: New function terminal_save_ours()

[Stephane Carrez]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6753 bytes --]

Hi!

Andrew Cagney a écrit :
> 
> > Hi!
> >
> > When using gdb-tui and switching to or leaving the TUI mode, the terminal
> > settings are changed.  This is part of the curses management (endwin).
> >
> > Gdb keeps in a static variable in inflow.c for the tty setting of himself.
> > It switches it with the inferior terminal (terminal_inferior, terminal_ours).
> > The gdb terminal settings are fetched only once, the first time it is
> > used/checked (so we don't know when).
> >
> > For the TUI to work correctly, it needs to update the gdb knowledge of
> > its terminal.  The patch below proposes a new function `terminal_save_ours'
> > which is called by TUI each time the TUI changes from mode;  that is
> > after we enter in TUI mode (curses), and after we left the TUI mode.
> >
> > Can you approve this patch ?
> 
> I wish it were this simple.
> 
so do I...

> target_terminal_ours(), target_terminal_inferior() and
> target_terminal_ours_for_output() all go through the target vector.  The
> , er, obvious (?) thing to do would be to add
> target_terminal_save_our_tty_state(), or what ever, to that target
> vector.  I'm, to be honest, not sure if it is ``obvious'' or not.
> 
> Think about a remote target that has implemented console I/O.
> 
> When GDB hands off to the target it calls target_terminal_inferior(),
> that in turn causes remote.c to steal the input from GDB so that it can
> funnel all console input down to the remote target.  When the inferior
> stops, GDB calls target_terminal_ours(), and the target relinquishes
> that control.  At present, remote.c (and possibly all other remote
> targets) assumes that this operation doesn't require any get/set tty
> state manipulation.  I don't think the patch, as it currently stands
> addresses this.

Agree.  It does not address that.

> 
> How to best address it is where things get tricky.  I'm not sure if the
> targets should all be updated to do the set/get tty tweeking or if the
> set/get tty stuff should be brought closer to GDB and always done.
> 
>         Andrew
> 

Can we see this as a first step?

I would like to avoid to change the target vector before 5.1.
We can introduce the target function after 5.1 and let it point to
the terminal_save_our_state() as for other terminal_xxx().
The TUI uses terminal_save_our_state() and we will switch it
to the target_xxx().


> > Index: inflow.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/inflow.c,v
> > retrieving revision 1.11
> > diff -u -p -r1.11 inflow.c
> > --- inflow.c  2001/07/15 20:34:13     1.11
> > +++ inflow.c  2001/07/19 22:48:49
> > @@ -200,6 +200,23 @@ terminal_init_inferior_with_pgrp (int pg
> >      }
> >  }
> >
> > +/* Save the terminal settings again.  This is necessary for the TUI
> > +   when it switches to TUI or non-TUI mode;  curses changes the terminal
> > +   and gdb must be able to restore it correctly.  */
> > +
> > +void
> > +terminal_save_ours ()
> 
> I'd recommend a more explicit name - ..._save_our_state?
> 
Ok

> > +{
> > +  if (gdb_has_a_terminal ())
> > +    {
> > +      /* We could just as well copy our_ttystate (if we felt like adding
> > +         a new function serial_copy_tty_state).  */
> > +      if (our_ttystate)
> > +     xfree (our_ttystate);
> > +      our_ttystate = serial_get_tty_state (stdin_serial);
> > +    }
> > +}
> 
> Should all existing ``our_ttystate = serial_get_tty_state()'' calls be
> replaced with calls to this function?
> 
>         Andrew

There is only one such call and it is in gdb_has_a_terminal().
I don't see any benefit in doing so.

	Stephane

2001-07-24  Stephane Carrez  <Stephane.Carrez@worldnet.fr>

        * inferior.h (terminal_save_our_state): Declare.
        * inflow.c (terminal_save_our_state): New function.
Index: inflow.c
===================================================================
RCS file: /cvs/src/src/gdb/inflow.c,v
retrieving revision 1.11
diff -u -p -r1.11 inflow.c
--- inflow.c	2001/07/15 20:34:13	1.11
+++ inflow.c	2001/07/24 20:42:22
@@ -200,6 +200,23 @@ terminal_init_inferior_with_pgrp (int pg
     }
 }
 
+/* Save the terminal settings again.  This is necessary for the TUI
+   when it switches to TUI or non-TUI mode;  curses changes the terminal
+   and gdb must be able to restore it correctly.  */
+
+void
+terminal_save_our_state ()
+{
+  if (gdb_has_a_terminal ())
+    {
+      /* We could just as well copy our_ttystate (if we felt like adding
+         a new function serial_copy_tty_state).  */
+      if (our_ttystate)
+	xfree (our_ttystate);
+      our_ttystate = serial_get_tty_state (stdin_serial);
+    }
+}
+
 void
 terminal_init_inferior (void)
 {
Index: inferior.h
===================================================================
RCS file: /cvs/src/src/gdb/inferior.h,v
retrieving revision 1.23
diff -u -p -r1.23 inferior.h
--- inferior.h	2001/05/15 00:03:36	1.23
+++ inferior.h	2001/07/24 20:42:28
@@ -151,6 +151,8 @@ extern void generic_mourn_inferior (void
 
 extern void terminal_ours (void);
 
+extern void terminal_save_our_state (void);
+
 extern int run_stack_dummy (CORE_ADDR, char *);
 
 extern CORE_ADDR read_pc (void);
From ac131313@cygnus.com Tue Jul 24 14:09:00 2001
From: Andrew Cagney <ac131313@cygnus.com>
To: Stephane Carrez <Stephane.Carrez@worldnet.fr>
Cc: gdb-patches@sources.redhat.com
Subject: Re: [RFA]: New function terminal_save_ours()
Date: Tue, 24 Jul 2001 14:09:00 -0000
Message-id: <3B5DE3F1.8010605@cygnus.com>
References: <3B576530.13178D50@worldnet.fr> <3B5DABFE.2010209@cygnus.com> <3B5DDF72.3A59A477@worldnet.fr>
X-SW-Source: 2001-07/msg00606.html
Content-length: 1108

> 
> Agree.  It does not address that.
> 
> 
>> 
>> How to best address it is where things get tricky.  I'm not sure if the
>> targets should all be updated to do the set/get tty tweeking or if the
>> set/get tty stuff should be brought closer to GDB and always done.
>> 
>> Andrew
>> 
> 
> 
> Can we see this as a first step?
> 
> I would like to avoid to change the target vector before 5.1.
> We can introduce the target function after 5.1 and let it point to
> the terminal_save_our_state() as for other terminal_xxx().
> The TUI uses terminal_save_our_state() and we will switch it
> to the target_xxx().


Sorry, but no, I'd rather see the probem fixed :-/  You're just, 
unfortunatly, the one to draw the short straw.

The target vector has a very sordid history in this regard.  Too often 
have people allowed things to slip through that should have, instead, 
been included in the target vector.

For the TUI, I was thinking more of a 5.2 timeframe.  Regarding the 
target vector, I might cook something (gdbarch.sh like) up - but what 
ever it is it wouldn't go in until 5.1 is branched.

	Andrew