* [PATCH] [RFC] Don't propagate our current terminal state to the inferior @ 2014-11-22 20:39 Patrick Palka 2014-11-23 10:25 ` Joel Brobecker ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Patrick Palka @ 2014-11-22 20:39 UTC (permalink / raw) To: gdb-patches; +Cc: Patrick Palka Currently when we start an inferior we have the inferior inherit our terminal state. Under TUI, our terminal is highly modified by ncurses and readline. So when starting an inferior under TUI, the inferior will have a highly modified terminal state which will interfere with standard I/O. For example, $ gdb gdb (gdb) break main (gdb) run (gdb) print puts ("a\nb") a b $1 = 4 (gdb) [enter TUI mode] (gdb) run (gdb) [exit TUI mode] (gdb) print puts ("a\nb") a b $2 = 4 (gdb) print puts ("a\r\nb\r") a b $3 = 6 As you can see, when we start the inferior under the regular interface, puts() prints the text properly. But when we start the inferior under TUI, puts() does not print the text properly. This is because when we start the inferior under TUI it inherits our current terminal state which has been modified by ncurses to, among other things, require an explicit \r\n to print a new line. As a result the inferior performs standard I/O in an unexpected way. Because of this discrepancy, it doesn't seem like a good idea to have the inferior inherit our _current_ terminal state for it may have been modified by readline and/or ncurses. Instead, we should have the inferior inherit a pristine snapshot of our terminal state taken before readline or ncurses have had a chance to alter it. This enables the inferior to run in a more accurate way, more closely mimicking its behavior had the program run standalone. And it fixes the above mentioned issue. I wonder, does this change make sense? What do others think? Tested on x86_64-unknown-linux-gnu. * terminal.h (set_initial_inferior_ttystate): Declare. * inflow.c (initial_inferior_ttystate): New static variable. (set_initial_inferior_ttystate): New setter. (child_terminal_init_with_pgrp): Copy initial_inferior_ttystate instead of our current terminal state. * top.c (gdb_init): Call set_initial_inferior_ttystate. --- gdb/inflow.c | 13 ++++++++++++- gdb/terminal.h | 2 ++ gdb/top.c | 4 ++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/gdb/inflow.c b/gdb/inflow.c index 8902174..7b432ad 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -79,6 +79,10 @@ struct terminal_info unimportant. */ static struct terminal_info our_terminal_info; +/* The initial tty state given to each new inferior. It is a snapshot of our + own tty state taken during initialization of GDB. */ +static serial_ttystate initial_inferior_ttystate; + static struct terminal_info *get_inflow_inferior_data (struct inferior *); #ifdef PROCESS_GROUP_TYPE @@ -156,6 +160,13 @@ show_interactive_mode (struct ui_file *file, int from_tty, fprintf_filtered (file, "Debugger's interactive mode is %s.\n", value); } +/* Set the initial tty state that is to be inherited by new inferiors. */ +void +set_initial_inferior_ttystate (void) +{ + initial_inferior_ttystate = serial_get_tty_state (stdin_serial); +} + /* Does GDB have a terminal (on stdin)? */ int gdb_has_a_terminal (void) @@ -227,7 +238,7 @@ child_terminal_init_with_pgrp (int pgrp) { xfree (tinfo->ttystate); tinfo->ttystate = serial_copy_tty_state (stdin_serial, - our_terminal_info.ttystate); + initial_inferior_ttystate); /* Make sure that next time we call terminal_inferior (which will be before the program runs, as it needs to be), we install the new diff --git a/gdb/terminal.h b/gdb/terminal.h index 433aa7d..186bce2 100644 --- a/gdb/terminal.h +++ b/gdb/terminal.h @@ -103,6 +103,8 @@ extern int gdb_has_a_terminal (void); extern void gdb_save_tty_state (void); +extern void set_initial_inferior_ttystate (void); + /* Set the process group of the caller to its own pid, or do nothing if we lack job control. */ extern int gdb_setpgid (void); diff --git a/gdb/top.c b/gdb/top.c index 83d858a..c4b5c2c 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -1886,6 +1886,10 @@ gdb_init (char *argv0) initialize_stdin_serial (); + /* Take a snapshot of our tty state before readline/ncurses have had a chance + to alter it. */ + set_initial_inferior_ttystate (); + async_init_signals (); /* We need a default language for parsing expressions, so simple -- 2.2.0.rc1.23.gf570943 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RFC] Don't propagate our current terminal state to the inferior 2014-11-22 20:39 [PATCH] [RFC] Don't propagate our current terminal state to the inferior Patrick Palka @ 2014-11-23 10:25 ` Joel Brobecker 2015-01-07 13:09 ` Patrick Palka 2015-01-07 13:39 ` Pedro Alves 2 siblings, 0 replies; 9+ messages in thread From: Joel Brobecker @ 2014-11-23 10:25 UTC (permalink / raw) To: Patrick Palka; +Cc: gdb-patches > Currently when we start an inferior we have the inferior inherit our > terminal state. Under TUI, our terminal is highly modified by ncurses > and readline. So when starting an inferior under TUI, the inferior will > have a highly modified terminal state which will interfere with standard > I/O. For example, > > $ gdb gdb > (gdb) break main > (gdb) run > (gdb) print puts ("a\nb") > a > b > $1 = 4 > (gdb) [enter TUI mode] > (gdb) run > (gdb) [exit TUI mode] > (gdb) print puts ("a\nb") > a > b > $2 = 4 > (gdb) print puts ("a\r\nb\r") > a > b > $3 = 6 > > As you can see, when we start the inferior under the regular interface, > puts() prints the text properly. But when we start the inferior under > TUI, puts() does not print the text properly. This is because when we > start the inferior under TUI it inherits our current terminal state > which has been modified by ncurses to, among other things, require an > explicit \r\n to print a new line. As a result the inferior performs > standard I/O in an unexpected way. > > Because of this discrepancy, it doesn't seem like a good idea to have > the inferior inherit our _current_ terminal state for it may have been > modified by readline and/or ncurses. Instead, we should have the > inferior inherit a pristine snapshot of our terminal state taken before > readline or ncurses have had a chance to alter it. This enables the > inferior to run in a more accurate way, more closely mimicking its > behavior had the program run standalone. And it fixes the above > mentioned issue. > > I wonder, does this change make sense? What do others think? FWIW: It does make sense to me, but I have been known to have a really superficial view or how terminals work. I'd be more comfortable if you waited for someone like Pedro to give it a second look. If no one reviews it by, say, Dec 8th, can you ping me again? Thank you, -- Joel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RFC] Don't propagate our current terminal state to the inferior 2014-11-22 20:39 [PATCH] [RFC] Don't propagate our current terminal state to the inferior Patrick Palka 2014-11-23 10:25 ` Joel Brobecker @ 2015-01-07 13:09 ` Patrick Palka 2015-01-07 13:39 ` Pedro Alves 2 siblings, 0 replies; 9+ messages in thread From: Patrick Palka @ 2015-01-07 13:09 UTC (permalink / raw) To: gdb-patches; +Cc: Patrick Palka On Sat, Nov 22, 2014 at 3:39 PM, Patrick Palka <patrick@parcs.ath.cx> wrote: > Currently when we start an inferior we have the inferior inherit our > terminal state. Under TUI, our terminal is highly modified by ncurses > and readline. So when starting an inferior under TUI, the inferior will > have a highly modified terminal state which will interfere with standard > I/O. For example, > > $ gdb gdb > (gdb) break main > (gdb) run > (gdb) print puts ("a\nb") > a > b > $1 = 4 > (gdb) [enter TUI mode] > (gdb) run > (gdb) [exit TUI mode] > (gdb) print puts ("a\nb") > a > b > $2 = 4 > (gdb) print puts ("a\r\nb\r") > a > b > $3 = 6 > > As you can see, when we start the inferior under the regular interface, > puts() prints the text properly. But when we start the inferior under > TUI, puts() does not print the text properly. This is because when we > start the inferior under TUI it inherits our current terminal state > which has been modified by ncurses to, among other things, require an > explicit \r\n to print a new line. As a result the inferior performs > standard I/O in an unexpected way. > > Because of this discrepancy, it doesn't seem like a good idea to have > the inferior inherit our _current_ terminal state for it may have been > modified by readline and/or ncurses. Instead, we should have the > inferior inherit a pristine snapshot of our terminal state taken before > readline or ncurses have had a chance to alter it. This enables the > inferior to run in a more accurate way, more closely mimicking its > behavior had the program run standalone. And it fixes the above > mentioned issue. > > I wonder, does this change make sense? What do others think? > > Tested on x86_64-unknown-linux-gnu. > > * terminal.h (set_initial_inferior_ttystate): Declare. > * inflow.c (initial_inferior_ttystate): New static variable. > (set_initial_inferior_ttystate): New setter. > (child_terminal_init_with_pgrp): Copy initial_inferior_ttystate > instead of our current terminal state. > * top.c (gdb_init): Call set_initial_inferior_ttystate. > --- > gdb/inflow.c | 13 ++++++++++++- > gdb/terminal.h | 2 ++ > gdb/top.c | 4 ++++ > 3 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/gdb/inflow.c b/gdb/inflow.c > index 8902174..7b432ad 100644 > --- a/gdb/inflow.c > +++ b/gdb/inflow.c > @@ -79,6 +79,10 @@ struct terminal_info > unimportant. */ > static struct terminal_info our_terminal_info; > > +/* The initial tty state given to each new inferior. It is a snapshot of our > + own tty state taken during initialization of GDB. */ > +static serial_ttystate initial_inferior_ttystate; > + > static struct terminal_info *get_inflow_inferior_data (struct inferior *); > > #ifdef PROCESS_GROUP_TYPE > @@ -156,6 +160,13 @@ show_interactive_mode (struct ui_file *file, int from_tty, > fprintf_filtered (file, "Debugger's interactive mode is %s.\n", value); > } > > +/* Set the initial tty state that is to be inherited by new inferiors. */ > +void > +set_initial_inferior_ttystate (void) > +{ > + initial_inferior_ttystate = serial_get_tty_state (stdin_serial); > +} > + > /* Does GDB have a terminal (on stdin)? */ > int > gdb_has_a_terminal (void) > @@ -227,7 +238,7 @@ child_terminal_init_with_pgrp (int pgrp) > { > xfree (tinfo->ttystate); > tinfo->ttystate = serial_copy_tty_state (stdin_serial, > - our_terminal_info.ttystate); > + initial_inferior_ttystate); > > /* Make sure that next time we call terminal_inferior (which will be > before the program runs, as it needs to be), we install the new > diff --git a/gdb/terminal.h b/gdb/terminal.h > index 433aa7d..186bce2 100644 > --- a/gdb/terminal.h > +++ b/gdb/terminal.h > @@ -103,6 +103,8 @@ extern int gdb_has_a_terminal (void); > > extern void gdb_save_tty_state (void); > > +extern void set_initial_inferior_ttystate (void); > + > /* Set the process group of the caller to its own pid, or do nothing > if we lack job control. */ > extern int gdb_setpgid (void); > diff --git a/gdb/top.c b/gdb/top.c > index 83d858a..c4b5c2c 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -1886,6 +1886,10 @@ gdb_init (char *argv0) > > initialize_stdin_serial (); > > + /* Take a snapshot of our tty state before readline/ncurses have had a chance > + to alter it. */ > + set_initial_inferior_ttystate (); > + > async_init_signals (); > > /* We need a default language for parsing expressions, so simple > -- > 2.2.0.rc1.23.gf570943 > Ping. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RFC] Don't propagate our current terminal state to the inferior 2014-11-22 20:39 [PATCH] [RFC] Don't propagate our current terminal state to the inferior Patrick Palka 2014-11-23 10:25 ` Joel Brobecker 2015-01-07 13:09 ` Patrick Palka @ 2015-01-07 13:39 ` Pedro Alves 2015-01-07 14:13 ` Patrick Palka 2015-01-07 21:44 ` Patrick Palka 2 siblings, 2 replies; 9+ messages in thread From: Pedro Alves @ 2015-01-07 13:39 UTC (permalink / raw) To: Patrick Palka, gdb-patches On 11/22/2014 08:39 PM, Patrick Palka wrote: > Currently when we start an inferior we have the inferior inherit our > terminal state. Under TUI, our terminal is highly modified by ncurses > and readline. So when starting an inferior under TUI, the inferior will > have a highly modified terminal state which will interfere with standard > I/O. For example, > > $ gdb gdb > (gdb) break main > (gdb) run > (gdb) print puts ("a\nb") > a > b > $1 = 4 > (gdb) [enter TUI mode] > (gdb) run > (gdb) [exit TUI mode] > (gdb) print puts ("a\nb") > a > b > $2 = 4 > (gdb) print puts ("a\r\nb\r") > a > b > $3 = 6 > > As you can see, when we start the inferior under the regular interface, > puts() prints the text properly. But when we start the inferior under > TUI, puts() does not print the text properly. This is because when we > start the inferior under TUI it inherits our current terminal state > which has been modified by ncurses to, among other things, require an > explicit \r\n to print a new line. As a result the inferior performs > standard I/O in an unexpected way. > > Because of this discrepancy, it doesn't seem like a good idea to have > the inferior inherit our _current_ terminal state for it may have been > modified by readline and/or ncurses. Instead, we should have the > inferior inherit a pristine snapshot of our terminal state taken before > readline or ncurses have had a chance to alter it. This enables the > inferior to run in a more accurate way, more closely mimicking its > behavior had the program run standalone. And it fixes the above > mentioned issue. > > I wonder, does this change make sense? What do others think? Thanks. This makes sense to me. > > +/* The initial tty state given to each new inferior. It is a snapshot of our > + own tty state taken during initialization of GDB. */ > +static serial_ttystate initial_inferior_ttystate; > + > static struct terminal_info *get_inflow_inferior_data (struct inferior *); > > #ifdef PROCESS_GROUP_TYPE > @@ -156,6 +160,13 @@ show_interactive_mode (struct ui_file *file, int from_tty, > fprintf_filtered (file, "Debugger's interactive mode is %s.\n", value); > } > > +/* Set the initial tty state that is to be inherited by new inferiors. */ > +void > +set_initial_inferior_ttystate (void) > +{ > + initial_inferior_ttystate = serial_get_tty_state (stdin_serial); > +} "initial inferior" reminded me of the initial inferior that GDB always creates. E.g., in main.c: /* Now that gdb_init has created the initial inferior, we're in position to set args for that inferior. */ Could you rename the function and the variable? Like: set_initial_gdb_ttystate (void) { initial_gdb_ttystate = serial_get_tty_state (stdin_serial); } ... /* Snapshot of our own tty state taken during initialization of GDB. This is used as tty state given to each new inferior. */ static serial_ttystate initial_gdb_ttystate; Calling the variable that way calls it for what it holds, not for how it is used, which is friendlier to reuse in other situations. This would be OK with me with that change. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RFC] Don't propagate our current terminal state to the inferior 2015-01-07 13:39 ` Pedro Alves @ 2015-01-07 14:13 ` Patrick Palka 2015-01-07 14:56 ` Joel Brobecker 2015-01-07 21:44 ` Patrick Palka 1 sibling, 1 reply; 9+ messages in thread From: Patrick Palka @ 2015-01-07 14:13 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On Wed, 7 Jan 2015, Pedro Alves wrote: > On 11/22/2014 08:39 PM, Patrick Palka wrote: >> Currently when we start an inferior we have the inferior inherit our >> terminal state. Under TUI, our terminal is highly modified by ncurses >> and readline. So when starting an inferior under TUI, the inferior will >> have a highly modified terminal state which will interfere with standard >> I/O. For example, >> >> $ gdb gdb >> (gdb) break main >> (gdb) run >> (gdb) print puts ("a\nb") >> a >> b >> $1 = 4 >> (gdb) [enter TUI mode] >> (gdb) run >> (gdb) [exit TUI mode] >> (gdb) print puts ("a\nb") >> a >> b >> $2 = 4 >> (gdb) print puts ("a\r\nb\r") >> a >> b >> $3 = 6 >> >> As you can see, when we start the inferior under the regular interface, >> puts() prints the text properly. But when we start the inferior under >> TUI, puts() does not print the text properly. This is because when we >> start the inferior under TUI it inherits our current terminal state >> which has been modified by ncurses to, among other things, require an >> explicit \r\n to print a new line. As a result the inferior performs >> standard I/O in an unexpected way. >> >> Because of this discrepancy, it doesn't seem like a good idea to have >> the inferior inherit our _current_ terminal state for it may have been >> modified by readline and/or ncurses. Instead, we should have the >> inferior inherit a pristine snapshot of our terminal state taken before >> readline or ncurses have had a chance to alter it. This enables the >> inferior to run in a more accurate way, more closely mimicking its >> behavior had the program run standalone. And it fixes the above >> mentioned issue. >> >> I wonder, does this change make sense? What do others think? > > Thanks. This makes sense to me. > >> >> +/* The initial tty state given to each new inferior. It is a snapshot of our >> + own tty state taken during initialization of GDB. */ >> +static serial_ttystate initial_inferior_ttystate; >> + >> static struct terminal_info *get_inflow_inferior_data (struct inferior *); >> >> #ifdef PROCESS_GROUP_TYPE >> @@ -156,6 +160,13 @@ show_interactive_mode (struct ui_file *file, int from_tty, >> fprintf_filtered (file, "Debugger's interactive mode is %s.\n", value); >> } >> >> +/* Set the initial tty state that is to be inherited by new inferiors. */ >> +void >> +set_initial_inferior_ttystate (void) >> +{ >> + initial_inferior_ttystate = serial_get_tty_state (stdin_serial); >> +} > > "initial inferior" reminded me of the initial inferior that > GDB always creates. E.g., in main.c: > > /* Now that gdb_init has created the initial inferior, we're in > position to set args for that inferior. */ > > > Could you rename the function and the variable? Like: > > set_initial_gdb_ttystate (void) > { > initial_gdb_ttystate = serial_get_tty_state (stdin_serial); > } > > ... > > /* Snapshot of our own tty state taken during initialization > of GDB. This is used as tty state given to each new inferior. */ > static serial_ttystate initial_gdb_ttystate; > > > > Calling the variable that way calls it for what it holds, not > for how it is used, which is friendlier to reuse in > other situations. > > This would be OK with me with that change. > > Thanks, > Pedro Alves > > Thanks Pedro. I have changed the names of the new function and variable and committed the patch. Please let me know if I have messed up the commit procedure. Final patch: -- >8 -- Subject: [PATCH] Don't propagate our current terminal state to the inferior Currently when we start an inferior we have the inferior inherit our terminal state. Under TUI, our terminal is highly modified by ncurses and readline. So when starting an inferior under TUI, the inferior will have a highly modified terminal state which will interfere with standard I/O. For example, $ gdb gdb (gdb) break main (gdb) run (gdb) print puts ("a\nb") a b $1 = 4 (gdb) [enter TUI mode] (gdb) run (gdb) [exit TUI mode] (gdb) print puts ("a\nb") a b $2 = 4 (gdb) print puts ("a\r\nb\r") a b $3 = 6 As you can see, when we start the inferior under the regular interface, puts() prints the text properly. But when we start the inferior under TUI, puts() does not print the text properly. This is because when we start the inferior under TUI it inherits our current terminal state which has been modified by ncurses to, among other things, require an explicit \r\n to print a new line. As a result the inferior performs standard I/O in an unexpected way. Because of this discrepancy, it doesn't seem like a good idea to have the inferior inherit our _current_ terminal state for it may have been modified by readline and/or ncurses. Instead, we should have the inferior inherit a pristine snapshot of our terminal state taken before readline or ncurses have had a chance to alter it. This enables the inferior to run in a more accurate way, more closely mimicking the program's behavior had it run standalone. And it fixes the above mentioned issue. Tested on x86_64-unknown-linux-gnu. gdb/ChangeLog: * terminal.h (set_initial_gdb_ttystate): Declare. * inflow.c (initial_gdb_ttystate): New static variable. (set_initial_gdb_ttystate): New setter. (child_terminal_init_with_pgrp): Copy initial_gdb_ttystate instead of our current terminal state. * top.c (gdb_init): Call set_initial_gdb_ttystate. --- gdb/ChangeLog | 9 +++++++++ gdb/inflow.c | 13 ++++++++++++- gdb/terminal.h | 2 ++ gdb/top.c | 4 ++++ 4 files changed, 27 insertions(+), 1 deletion(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 0b63d34..6477dc1 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,12 @@ +2015-01-07 Patrick Palka <patrick@parcs.ath.cx> + + * terminal.h (set_initial_gdb_ttystate): Declare. + * inflow.c (initial_gdb_ttystate): New static variable. + (set_initial_gdb_ttystate): New setter. + (child_terminal_init_with_pgrp): Copy initial_gdb_ttystate + instead of our current terminal state. + * top.c (gdb_init): Call set_initial_gdb_ttystate. + 2015-01-07 Joel Brobecker <brobecker@adacore.com> * guile/scm-type.c (tyscm_array_1): Add comment. diff --git a/gdb/inflow.c b/gdb/inflow.c index 8947e63..3c121a3 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -79,6 +79,10 @@ struct terminal_info unimportant. */ static struct terminal_info our_terminal_info; +/* The initial tty state given to each new inferior. It is a snapshot of our + own tty state taken during initialization of GDB. */ +static serial_ttystate initial_gdb_ttystate; + static struct terminal_info *get_inflow_inferior_data (struct inferior *); #ifdef PROCESS_GROUP_TYPE @@ -156,6 +160,13 @@ show_interactive_mode (struct ui_file *file, int from_tty, fprintf_filtered (file, "Debugger's interactive mode is %s.\n", value); } +/* Set the initial tty state that is to be inherited by new inferiors. */ +void +set_initial_gdb_ttystate (void) +{ + initial_gdb_ttystate = serial_get_tty_state (stdin_serial); +} + /* Does GDB have a terminal (on stdin)? */ int gdb_has_a_terminal (void) @@ -227,7 +238,7 @@ child_terminal_init_with_pgrp (int pgrp) { xfree (tinfo->ttystate); tinfo->ttystate = serial_copy_tty_state (stdin_serial, - our_terminal_info.ttystate); + initial_gdb_ttystate); /* Make sure that next time we call terminal_inferior (which will be before the program runs, as it needs to be), we install the new diff --git a/gdb/terminal.h b/gdb/terminal.h index ae25c98..17acde2 100644 --- a/gdb/terminal.h +++ b/gdb/terminal.h @@ -103,6 +103,8 @@ extern int gdb_has_a_terminal (void); extern void gdb_save_tty_state (void); +extern void set_initial_gdb_ttystate (void); + /* Set the process group of the caller to its own pid, or do nothing if we lack job control. */ extern int gdb_setpgid (void); diff --git a/gdb/top.c b/gdb/top.c index 524a92b..b85ea1a 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -1890,6 +1890,10 @@ gdb_init (char *argv0) initialize_stdin_serial (); + /* Take a snapshot of our tty state before readline/ncurses have had a chance + to alter it. */ + set_initial_gdb_ttystate (); + async_init_signals (); /* We need a default language for parsing expressions, so simple -- 2.2.1.212.gc5b9256 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RFC] Don't propagate our current terminal state to the inferior 2015-01-07 14:13 ` Patrick Palka @ 2015-01-07 14:56 ` Joel Brobecker 2015-01-07 14:57 ` Joel Brobecker 0 siblings, 1 reply; 9+ messages in thread From: Joel Brobecker @ 2015-01-07 14:56 UTC (permalink / raw) To: Patrick Palka; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 801 bytes --] > gdb/ChangeLog: > > * terminal.h (set_initial_gdb_ttystate): Declare. > * inflow.c (initial_gdb_ttystate): New static variable. > (set_initial_gdb_ttystate): New setter. > (child_terminal_init_with_pgrp): Copy initial_gdb_ttystate > instead of our current terminal state. > * top.c (gdb_init): Call set_initial_gdb_ttystate. Small nit that I noticed just as I glanced at the patch... > +/* Set the initial tty state that is to be inherited by new inferiors. */ > +void > +set_initial_gdb_ttystate (void) > +{ We ask that an empty line between the function's documentation and its implementation be inserted. I have pushed the attached patch as obvious... gdb/ChangeLog: * inflow.c (set_initial_gdb_ttystate): Add empty line after comment documenting function. -- Joel [-- Attachment #2: 0001-Empty-line-after-comment-documenting-set_initial_gdb.patch --] [-- Type: text/x-diff, Size: 1151 bytes --] From ea42d6f8d1e24403e533e5dfea18e94c47ac534b Mon Sep 17 00:00:00 2001 From: Joel Brobecker <brobecker@adacore.com> Date: Wed, 7 Jan 2015 18:49:49 +0400 Subject: [PATCH] Empty line after comment documenting set_initial_gdb_ttystate. gdb/ChangeLog: * inflow.c (set_initial_gdb_ttystate): Add empty line after comment documenting function. --- gdb/ChangeLog | 5 +++++ gdb/inflow.c | 1 + 2 files changed, 6 insertions(+) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 6477dc1..e9b0377 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2015-01-07 Joel Brobecker <brobecker@adacore.com> + + * inflow.c (set_initial_gdb_ttystate): Add empty line after + comment documenting function. + 2015-01-07 Patrick Palka <patrick@parcs.ath.cx> * terminal.h (set_initial_gdb_ttystate): Declare. diff --git a/gdb/inflow.c b/gdb/inflow.c index 3c121a3..4c81a68 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -161,6 +161,7 @@ show_interactive_mode (struct ui_file *file, int from_tty, } /* Set the initial tty state that is to be inherited by new inferiors. */ + void set_initial_gdb_ttystate (void) { -- 1.9.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RFC] Don't propagate our current terminal state to the inferior 2015-01-07 14:56 ` Joel Brobecker @ 2015-01-07 14:57 ` Joel Brobecker 2015-01-07 15:18 ` Patrick Palka 0 siblings, 1 reply; 9+ messages in thread From: Joel Brobecker @ 2015-01-07 14:57 UTC (permalink / raw) To: Patrick Palka; +Cc: gdb-patches > We ask that an empty line between the function's documentation > and its implementation be inserted. And I meant to actually paste the reference: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Document_Every_Subprogram -- Joel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RFC] Don't propagate our current terminal state to the inferior 2015-01-07 14:57 ` Joel Brobecker @ 2015-01-07 15:18 ` Patrick Palka 0 siblings, 0 replies; 9+ messages in thread From: Patrick Palka @ 2015-01-07 15:18 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches On Wed, Jan 7, 2015 at 9:57 AM, Joel Brobecker <brobecker@adacore.com> wrote: >> We ask that an empty line between the function's documentation >> and its implementation be inserted. > > And I meant to actually paste the reference: > https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Document_Every_Subprogram Good to know. Thanks Joel. > > -- > Joel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RFC] Don't propagate our current terminal state to the inferior 2015-01-07 13:39 ` Pedro Alves 2015-01-07 14:13 ` Patrick Palka @ 2015-01-07 21:44 ` Patrick Palka 1 sibling, 0 replies; 9+ messages in thread From: Patrick Palka @ 2015-01-07 21:44 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On Wed, 7 Jan 2015, Pedro Alves wrote: > On 11/22/2014 08:39 PM, Patrick Palka wrote: >> Currently when we start an inferior we have the inferior inherit our >> terminal state. Under TUI, our terminal is highly modified by ncurses >> and readline. So when starting an inferior under TUI, the inferior will >> have a highly modified terminal state which will interfere with standard >> I/O. For example, >> >> $ gdb gdb >> (gdb) break main >> (gdb) run >> (gdb) print puts ("a\nb") >> a >> b >> $1 = 4 >> (gdb) [enter TUI mode] >> (gdb) run >> (gdb) [exit TUI mode] >> (gdb) print puts ("a\nb") >> a >> b >> $2 = 4 >> (gdb) print puts ("a\r\nb\r") >> a >> b >> $3 = 6 >> >> As you can see, when we start the inferior under the regular interface, >> puts() prints the text properly. But when we start the inferior under >> TUI, puts() does not print the text properly. This is because when we >> start the inferior under TUI it inherits our current terminal state >> which has been modified by ncurses to, among other things, require an >> explicit \r\n to print a new line. As a result the inferior performs >> standard I/O in an unexpected way. >> >> Because of this discrepancy, it doesn't seem like a good idea to have >> the inferior inherit our _current_ terminal state for it may have been >> modified by readline and/or ncurses. Instead, we should have the >> inferior inherit a pristine snapshot of our terminal state taken before >> readline or ncurses have had a chance to alter it. This enables the >> inferior to run in a more accurate way, more closely mimicking its >> behavior had the program run standalone. And it fixes the above >> mentioned issue. >> >> I wonder, does this change make sense? What do others think? > > Thanks. This makes sense to me. > >> >> +/* The initial tty state given to each new inferior. It is a snapshot of our >> + own tty state taken during initialization of GDB. */ >> +static serial_ttystate initial_inferior_ttystate; >> + >> static struct terminal_info *get_inflow_inferior_data (struct inferior *); >> >> #ifdef PROCESS_GROUP_TYPE >> @@ -156,6 +160,13 @@ show_interactive_mode (struct ui_file *file, int from_tty, >> fprintf_filtered (file, "Debugger's interactive mode is %s.\n", value); >> } >> >> +/* Set the initial tty state that is to be inherited by new inferiors. */ >> +void >> +set_initial_inferior_ttystate (void) >> +{ >> + initial_inferior_ttystate = serial_get_tty_state (stdin_serial); >> +} > > "initial inferior" reminded me of the initial inferior that > GDB always creates. E.g., in main.c: > > /* Now that gdb_init has created the initial inferior, we're in > position to set args for that inferior. */ > > > Could you rename the function and the variable? Like: > > set_initial_gdb_ttystate (void) > { > initial_gdb_ttystate = serial_get_tty_state (stdin_serial); > } > > ... > > /* Snapshot of our own tty state taken during initialization > of GDB. This is used as tty state given to each new inferior. */ I noticed that I forgot to accordingly update the comment for initial_gdb_ttystate as you suggested. I have committed this trivial patch under the "obvious" rule: -- >8 -- Subject: [PATCH] Trivially tweak the comment documenting initial_gdb_ttystate gdb/ChangeLog: * inflow.c (initial_gdb_ttystate): Tweak comment. --- gdb/ChangeLog | 4 ++++ gdb/inflow.c | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index e9b0377..b188988 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,7 @@ +2015-01-07 Patrick Palka <patrick@parcs.ath.cx> + + * inflow.c (initial_gdb_ttystate): Tweak comment. + 2015-01-07 Joel Brobecker <brobecker@adacore.com> * inflow.c (set_initial_gdb_ttystate): Add empty line after diff --git a/gdb/inflow.c b/gdb/inflow.c index 4c81a68..1456fd8 100644 --- a/gdb/inflow.c +++ b/gdb/inflow.c @@ -79,8 +79,8 @@ struct terminal_info unimportant. */ static struct terminal_info our_terminal_info; -/* The initial tty state given to each new inferior. It is a snapshot of our - own tty state taken during initialization of GDB. */ +/* Snapshot of our own tty state taken during initialization of GDB. + This is used as the initial tty state given to each new inferior. */ static serial_ttystate initial_gdb_ttystate; static struct terminal_info *get_inflow_inferior_data (struct inferior *); -- 2.2.1.212.gc5b9256 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-01-07 21:44 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-22 20:39 [PATCH] [RFC] Don't propagate our current terminal state to the inferior Patrick Palka 2014-11-23 10:25 ` Joel Brobecker 2015-01-07 13:09 ` Patrick Palka 2015-01-07 13:39 ` Pedro Alves 2015-01-07 14:13 ` Patrick Palka 2015-01-07 14:56 ` Joel Brobecker 2015-01-07 14:57 ` Joel Brobecker 2015-01-07 15:18 ` Patrick Palka 2015-01-07 21:44 ` Patrick Palka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).