Fix restoring of inferior terminal settings

Message ID 20180724021643.16041-1-simon.marchi@polymtl.ca
State New, archived
Headers

Commit Message

Simon Marchi July 24, 2018, 2:16 a.m. UTC
  I noticed that the child_terminal_save_inferior function was not used
since the commit f6ac5f3d63e0 ("Convert struct target_ops to C++").  I
was able to make a little test program to illustrate the problem (see
test case).

I think we're just missing the override of the terminal_save_inferior
method in inf_child_target (along with the other terminal-related
methods).

Instead of creating a new test, I thought that gdb.base/term.exp was a
good candidate for testing that gdb restores properly the inferior's
terminal settings.

gdb/ChangeLog:

	* inf-child.h (inf_child_target) <terminal_save_inferior>: New.
	* inf-child.c (inf_child_target::terminal_save_inferior): New.

gdb/testsuite/ChangeLog:

	* gdb.base/term.exp: Compare terminal settings with values from
	the inferior.
	* gdb.base/term.c: Get and set terminal settings.
---
 gdb/inf-child.c                 |  6 +++
 gdb/inf-child.h                 |  1 +
 gdb/testsuite/gdb.base/term.c   | 22 +++++++++
 gdb/testsuite/gdb.base/term.exp | 81 ++++++++++++++++++++++++++++-----
 4 files changed, 99 insertions(+), 11 deletions(-)
  

Comments

Tom Tromey July 25, 2018, 8:16 p.m. UTC | #1
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> I noticed that the child_terminal_save_inferior function was not used
Simon> since the commit f6ac5f3d63e0 ("Convert struct target_ops to C++").  I
Simon> was able to make a little test program to illustrate the problem (see
Simon> test case).

Simon> I think we're just missing the override of the terminal_save_inferior
Simon> method in inf_child_target (along with the other terminal-related
Simon> methods).

Simon> Instead of creating a new test, I thought that gdb.base/term.exp was a
Simon> good candidate for testing that gdb restores properly the inferior's
Simon> terminal settings.

Thanks, this patch looks good to me.

I wonder if this is related to the suspend bug that Tom de Vries referred to.

Tom
  
Simon Marchi Aug. 22, 2018, 3:12 p.m. UTC | #2
On 2018-07-25 16:16, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> I noticed that the child_terminal_save_inferior function was not 
> used
> Simon> since the commit f6ac5f3d63e0 ("Convert struct target_ops to 
> C++").  I
> Simon> was able to make a little test program to illustrate the problem 
> (see
> Simon> test case).
> 
> Simon> I think we're just missing the override of the 
> terminal_save_inferior
> Simon> method in inf_child_target (along with the other 
> terminal-related
> Simon> methods).
> 
> Simon> Instead of creating a new test, I thought that gdb.base/term.exp 
> was a
> Simon> good candidate for testing that gdb restores properly the 
> inferior's
> Simon> terminal settings.
> 
> Thanks, this patch looks good to me.

Thanks, I pushed it.

> I wonder if this is related to the suspend bug that Tom de Vries 
> referred to.

I don't know, I have seen a few cases of GDB getting suspended like 
that, especially in error cases.  I think it's more that we miss calling 
target_terminal::{ours,ours_for_output} in these situations.

Simon
  

Patch

diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index 2f5babebce7f..44aa2f66fbfe 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -113,6 +113,12 @@  inf_child_target::terminal_inferior ()
   child_terminal_inferior (this);
 }
 
+void
+inf_child_target::terminal_save_inferior ()
+{
+  child_terminal_save_inferior (this);
+}
+
 void
 inf_child_target::terminal_ours_for_output ()
 {
diff --git a/gdb/inf-child.h b/gdb/inf-child.h
index 6316f3062da1..98969bc5fa31 100644
--- a/gdb/inf-child.h
+++ b/gdb/inf-child.h
@@ -46,6 +46,7 @@  public:
   bool supports_terminal_ours () override;
   void terminal_init () override;
   void terminal_inferior () override;
+  void terminal_save_inferior () override;
   void terminal_ours_for_output () override;
   void terminal_ours () override;
   void terminal_info (const char *, int) override;
diff --git a/gdb/testsuite/gdb.base/term.c b/gdb/testsuite/gdb.base/term.c
index 26d2c006ee4d..40ca62bb98cb 100644
--- a/gdb/testsuite/gdb.base/term.c
+++ b/gdb/testsuite/gdb.base/term.c
@@ -15,7 +15,29 @@ 
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+#include <termios.h>
+#include <unistd.h>
+
+static struct termios t;
+
+static void
+break_here ()
+{
+}
+
 int main ()
 {
+  tcgetattr (0, &t);
+  break_here ();
+
+  /* Disable ECHO.  */
+  t.c_lflag &= ~ECHO;
+  tcsetattr (0, TCSANOW, &t);
+  tcgetattr (0, &t);
+  break_here ();
+
+  tcgetattr (0, &t);
+  break_here ();
+
   return 0;
 }
diff --git a/gdb/testsuite/gdb.base/term.exp b/gdb/testsuite/gdb.base/term.exp
index 031a50b703b5..77307894e040 100644
--- a/gdb/testsuite/gdb.base/term.exp
+++ b/gdb/testsuite/gdb.base/term.exp
@@ -13,6 +13,9 @@ 
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+# Test that GDB saves and restores terminal settings correctly.  Also check
+# the output of the "info terminal" command.
+
 if { [prepare_for_testing "failed to prepare" term term.c] } {
     return -1
 }
@@ -20,28 +23,84 @@  if { [prepare_for_testing "failed to prepare" term term.c] } {
 # Once before running the program.
 gdb_test "info terminal" \
     "No saved terminal information.*" \
-    "test info terminal"
+    "test info terminal pre-execution"
 
-if ![runto_main] then {
-    fail "can't run to main"
+if ![runto break_here] then {
+    fail "can't run to break_here"
     return 0
 }
 
-# Once while the program is running and stopped.
+# Read the inferior's terminal settings, saved in the T global variable.
+
+proc read_term_settings_from_inferior {} {
+    set termios(c_iflag) [get_hexadecimal_valueof "t.c_iflag" oops]
+    set termios(c_oflag) [get_hexadecimal_valueof "t.c_oflag" oops]
+    set termios(c_cflag) [get_hexadecimal_valueof "t.c_cflag" oops]
+    set termios(c_lflag) [get_hexadecimal_valueof "t.c_lflag" oops]
+
+    return [array get termios]
+}
+
+# Validate that gdb's notion of the inferior's terminal settings are consistent
+# with the values read from the inferior.
+
+proc compare_gdb_and_inferior_settings { t } {
+    global decimal
+    array set termios $t
+
+    gdb_test "info terminal" \
+    [multi_line "Inferior's terminal status .currently saved by GDB.:" \
+                "File descriptor flags = .*" \
+                "Process group = $decimal" \
+                "c_iflag = ${termios(c_iflag)}, c_oflag = ${termios(c_oflag)}," \
+                "c_cflag = ${termios(c_cflag)}, c_lflag = ${termios(c_lflag)}.*" ]
+}
 
-# While only native targets save terminal status, we still test
-# everywhere to make sure that the command doesn't misbehave.
 if {[target_info gdb_protocol] == ""} {
-    set term_re "Inferior's terminal status .currently saved by GDB.:.*"
+    # Record the initial terminal settings.  Verify that GDB's version of the
+    # inferior's terminal settings is right.
+    with_test_prefix "initial" {
+        array set termios1 [read_term_settings_from_inferior]
+        compare_gdb_and_inferior_settings [array get termios1]
+    }
+
+    # Continue until after the inferior removes ECHO from its terminal settings.
+    gdb_continue_to_breakpoint "continue until after tcsetattr"
+
+    # After the inferior has changed its terminal settings, check that GDB's
+    # saved version reflects the new settings correctly.
+    with_test_prefix "post tcsetattr" {
+        array set termios2 [read_term_settings_from_inferior]
+        compare_gdb_and_inferior_settings [array get termios2]
+
+        # Make sure that the current settings are different than the initial
+        # settings... otherwise this test is meaningless.
+        gdb_assert {${termios1(c_lflag)} != ${termios2(c_lflag)}}
+    }
+
+    # Continue again...
+    gdb_continue_to_breakpoint "continue again"
+
+    # ... and verify again, to validate that when resuming, GDB restored the
+    # inferior's terminal settings correctly.
+    with_test_prefix "after last resume" {
+        array set termios3 [read_term_settings_from_inferior]
+        compare_gdb_and_inferior_settings [array get termios3]
+        gdb_assert {${termios2(c_iflag)} == ${termios3(c_iflag)}}
+        gdb_assert {${termios2(c_oflag)} == ${termios3(c_oflag)}}
+        gdb_assert {${termios2(c_cflag)} == ${termios3(c_cflag)}}
+        gdb_assert {${termios2(c_lflag)} == ${termios3(c_lflag)}}
+    }
 } else {
-    set term_re "No saved terminal information\\."
+    # While only native targets save terminal status, we still test
+    # that the command doesn't misbehave.
+    gdb_test "info terminal" "No saved terminal information\\." "info terminal at breakpoint"
 }
 
-gdb_test "info terminal" $term_re "info terminal at breakpoint"
-
+delete_breakpoints
 gdb_continue_to_end
 
 # One last time after the program having exited.
 gdb_test "info terminal" \
     "No saved terminal information.*" \
-    "test info terminal #2"
+    "test info terminal post-execution"