[PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Simon Marchi]

The TUI test gdb.tui/empty.exp fails with the native-extended-gdbserver
board, and takes a very long time to run due to numerous timeouts.  The
symptom, when looking at the logs, is that the TUI windows that we
expect to be resized are not resized.  Digging down, I found that GDB
didn't receive any SIGWINCH that should have resulted from
Term::resize's stty calls.

The reason for this is:

- The native-extended-gdbserver overrides gdb_start to first start GDB,
  then start GDBserver with --multi, then connect GDB to GDBserver.
  This means that two TCL "spawn"s are done, one for GDB and one for
  GDBserver.

- The TUI test framework  wants to know GDB's TTY name, so it can pass
  it to stty, to fake terminal resizes.  To do so, it overrides the
  spawn built-in proc to capture the tty name from the internals of the
  built-in proc.  It saves the TTY name to the gdb_spawn_name global
  variable.

- Because the native-extended-gdbserver boards starts both GDB and
  GDBserver, the final value of gdb_spawn_name is the name of
  GDBserver's TTY.

- When the TUI test framework attempts to resize GDB's terminal, it in
  fact resizes GDBserver's terminal.  So obviously, GDB doesn't get the
  SIGWINCH, and we don't get the expected TUI redraw.

Fix that by moving the hack to the default_gdb_spawn proc instead, so
that we only record GDB's TTY name.  That name is saved in the
gdb_tty_name global variable.

I tried to use with_override for this, but it doesn't seem possible to
use with_override to override a builtin proc like "spawn", because "info
args" and "info body" fail for it.  So, manually rename the procs for
the time of the spawn.

Remove tuiterm_env_init and tuiterm_env_finish, since they are now
empty.  In turn, the gdb_finish_hooks mechanism is now unused, remove it
as well.  It would be easy to add them back if needed.

gdb/ChangeLog:

	* lib/gdb.exp (default_gdb_exit): Unset gdb_tty_name.
	(spawn_capture_tty_name): New.
	(default_gdb_spawn): Capture GDB's TTY name.
	* lib/tuiterm.exp (tuiterm_spawn): Remove.
	(tuiterm_env_init, tuiterm_env_finish): Remove spawn override.
	(Term) <resize>: Use new variable name.
	(tuiterm_env_init, tuiterm_env_finish): Remove.
	(tuiterm_env): Don't call tuiterm_env_init and register
	tuiterm_env_finish in gdb_finish_hooks.
	(gdb_finish_hooks): Remove.
	(gdb_finish): Don't call finish hooks.

    # Schedule finalization.
    global gdb_finish_hooks
    lappend gdb_finish_hooks tuiterm_env_finish

Change-Id: Ia5ab74184a52a996416022308f8d0cc523355a78
---
 gdb/testsuite/lib/gdb.exp     | 44 ++++++++++++++++++++++++++++-------
 gdb/testsuite/lib/tuiterm.exp | 35 ++--------------------------
 2 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 8469ec9801c0..34882362cc9d 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1909,6 +1909,7 @@ proc default_gdb_exit {} {
 	remote_close host
     }
     unset gdb_spawn_id
+    unset ::gdb_tty_name
     unset inferior_spawn_id
 }
 
@@ -2037,6 +2038,25 @@ proc gdb_file_cmd { arg } {
     }
 }
 
+# The expect "spawn" function puts the tty name into the spawn_out
+# array; but dejagnu doesn't export this globally.  So, we have to
+# wrap spawn with our own function and poke in the built-in spawn
+# so that we can capture this value.
+#
+# If available, the TTY name is saved to the LAST_SPAWN_TTY_NAME global.
+# Otherwise, LAST_SPAWN_TTY_NAME is unset.
+
+proc spawn_capture_tty_name { args } {
+    set result [uplevel builtin_spawn $args]
+    upvar spawn_out spawn_out
+    if { [info exists spawn_out] } {
+	set ::last_spawn_tty_name $spawn_out(slave,name)
+    } else {
+	unset ::last_spawn_tty_name
+    }
+    return $result
+}
+
 # Default gdb_spawn procedure.
 
 proc default_gdb_spawn { } {
@@ -2067,13 +2087,28 @@ proc default_gdb_spawn { } {
 	    exit 1
 	}
     }
-    set res [remote_spawn host "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts]"]
+
+    # Capture GDB's TTY name, so we can save it below.
+    rename spawn builtin_spawn
+    rename spawn_capture_tty_name spawn
+    set code [catch {uplevel 1 {
+        remote_spawn host "$::GDB $::INTERNAL_GDBFLAGS $::GDBFLAGS [host_info gdb_opts]"
+    }} res]
+    rename spawn spawn_capture_tty_name
+    rename builtin_spawn spawn
+
+    # If remote_spawn threw an error, propagate it.
+    if { $code == 1 } {
+	return -code $code -errorinfo $::errorInfo -errorcode $::errorCode $res
+    }
+
     if { $res < 0 || $res == "" } {
 	perror "Spawning $GDB failed."
 	return 1
     }
 
     set gdb_spawn_id $res
+    set ::gdb_tty_name $::last_spawn_tty_name
     return 0
 }
 
@@ -7800,13 +7835,6 @@ proc with_override { name override body } {
 # finalization function.
 proc tuiterm_env { } {
     load_lib tuiterm.exp
-
-    # Do initialization.
-    tuiterm_env_init
-
-    # Schedule finalization.
-    global gdb_finish_hooks
-    lappend gdb_finish_hooks tuiterm_env_finish
 }
 
 # Dejagnu has a version of note, but usage is not allowed outside of dejagnu.
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index fdd9f4d2188f..222583f291fb 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -15,36 +15,6 @@
 
 # An ANSI terminal emulator for expect.
 
-# The expect "spawn" function puts the tty name into the spawn_out
-# array; but dejagnu doesn't export this globally.  So, we have to
-# wrap spawn with our own function, so that we can capture this value.
-# The value is later used in calls to stty.
-proc tuiterm_spawn { args } {
-    set result [uplevel builtin_spawn $args]
-    global gdb_spawn_name
-    upvar spawn_out spawn_out
-    if { [info exists spawn_out] } {
-	set gdb_spawn_name $spawn_out(slave,name)
-    } else {
-	unset gdb_spawn_name
-    }
-    return $result
-}
-
-# Initialize tuiterm.exp environment.
-proc tuiterm_env_init { } {
-    # Override spawn with tui_spawn.
-    rename spawn builtin_spawn
-    rename tuiterm_spawn spawn
-}
-
-# Finalize tuiterm.exp environment.
-proc tuiterm_env_finish { } {
-    # Restore spawn.
-    rename spawn tuiterm_spawn
-    rename builtin_spawn spawn
-}
-
 namespace eval Term {
     # Size of the terminal.
     variable _rows
@@ -890,13 +860,12 @@ namespace eval Term {
 	variable _cols
 	variable _resize_count
 
-	global gdb_spawn_name
 	# expect handles each argument to stty separately.  This means
 	# that gdb will see SIGWINCH twice.  Rather than rely on this
 	# behavior (which, after all, could be changed), we make it
 	# explicit here.  This also simplifies waiting for the redraw.
 	_do_resize $rows $_cols
-	stty rows $_rows < $gdb_spawn_name
+	stty rows $_rows < $::gdb_tty_name
 	# Due to the strange column resizing behavior, and because we
 	# don't care about this intermediate resize, we don't check
 	# the size here.
@@ -906,7 +875,7 @@ namespace eval Term {
 	# than what we request from expect.  We hide this weird
 	# details from the caller.
 	_do_resize $_rows $cols
-	stty columns [expr {$_cols + 1}] < $gdb_spawn_name
+	stty columns [expr {$_cols + 1}] < $::gdb_tty_name
 	wait_for "@@ resize done $_resize_count, size = ${_cols}x${rows}"
 	incr _resize_count
     }
-- 
2.32.0

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> gdb/ChangeLog:

Simon> 	* lib/gdb.exp (default_gdb_exit): Unset gdb_tty_name.
Simon> 	(spawn_capture_tty_name): New.
Simon> 	(default_gdb_spawn): Capture GDB's TTY name.
Simon> 	* lib/tuiterm.exp (tuiterm_spawn): Remove.
Simon> 	(tuiterm_env_init, tuiterm_env_finish): Remove spawn override.
Simon> 	(Term) <resize>: Use new variable name.
Simon> 	(tuiterm_env_init, tuiterm_env_finish): Remove.
Simon> 	(tuiterm_env): Don't call tuiterm_env_init and register
Simon> 	tuiterm_env_finish in gdb_finish_hooks.
Simon> 	(gdb_finish_hooks): Remove.
Simon> 	(gdb_finish): Don't call finish hooks.

This looks good to me.

Simon> +    # Capture GDB's TTY name, so we can save it below.
Simon> +    rename spawn builtin_spawn
Simon> +    rename spawn_capture_tty_name spawn
Simon> +    set code [catch {uplevel 1 {
Simon> +        remote_spawn host "$::GDB $::INTERNAL_GDBFLAGS $::GDBFLAGS [host_info gdb_opts]"
Simon> +    }} res]
Simon> +    rename spawn spawn_capture_tty_name
Simon> +    rename builtin_spawn spawn
Simon> +
Simon> +    # If remote_spawn threw an error, propagate it.
Simon> +    if { $code == 1 } {
Simon> +	return -code $code -errorinfo $::errorInfo -errorcode $::errorCode $res
Simon> +    }

It would be simpler to just always rename+wrap spawn, and then only
stash the result in this particular case.  However, it's also fine this
way.

Tom

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Simon Marchi]

On 2021-06-09 11:09 a.m., Tom Tromey wrote:
> Simon> +    # Capture GDB's TTY name, so we can save it below.
> Simon> +    rename spawn builtin_spawn
> Simon> +    rename spawn_capture_tty_name spawn
> Simon> +    set code [catch {uplevel 1 {
> Simon> +        remote_spawn host "$::GDB $::INTERNAL_GDBFLAGS $::GDBFLAGS [host_info gdb_opts]"
> Simon> +    }} res]
> Simon> +    rename spawn spawn_capture_tty_name
> Simon> +    rename builtin_spawn spawn
> Simon> +
> Simon> +    # If remote_spawn threw an error, propagate it.
> Simon> +    if { $code == 1 } {
> Simon> +	return -code $code -errorinfo $::errorInfo -errorcode $::errorCode $res
> Simon> +    }
> 
> It would be simpler to just always rename+wrap spawn, and then only
> stash the result in this particular case.  However, it's also fine this
> way.

Yeah, I wanted to avoid overriding it all the time, to avoid bad
suprises.  Although if we don't introduce bugs, it should be fine ;).

Simon

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Simon Marchi]

On 2021-06-09 11:15 a.m., Simon Marchi via Gdb-patches wrote:
> On 2021-06-09 11:09 a.m., Tom Tromey wrote:
>> Simon> +    # Capture GDB's TTY name, so we can save it below.
>> Simon> +    rename spawn builtin_spawn
>> Simon> +    rename spawn_capture_tty_name spawn
>> Simon> +    set code [catch {uplevel 1 {
>> Simon> +        remote_spawn host "$::GDB $::INTERNAL_GDBFLAGS $::GDBFLAGS [host_info gdb_opts]"
>> Simon> +    }} res]
>> Simon> +    rename spawn spawn_capture_tty_name
>> Simon> +    rename builtin_spawn spawn
>> Simon> +
>> Simon> +    # If remote_spawn threw an error, propagate it.
>> Simon> +    if { $code == 1 } {
>> Simon> +	return -code $code -errorinfo $::errorInfo -errorcode $::errorCode $res
>> Simon> +    }
>>
>> It would be simpler to just always rename+wrap spawn, and then only
>> stash the result in this particular case.  However, it's also fine this
>> way.
> 
> Yeah, I wanted to avoid overriding it all the time, to avoid bad
> suprises.  Although if we don't introduce bugs, it should be fine ;).

Here's the version that overrides all the time.

From abc7ae3e7cb93aa723db58beb07a677686831466 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Tue, 8 Jun 2021 17:21:26 -0400
Subject: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

The TUI test gdb.tui/empty.exp fails with the native-extended-gdbserver
board, and takes a very long time to run due to numerous timeouts.  The
symptom, when looking at the logs, is that the TUI windows that we
expect to be resized are not resized.  Digging down, I found that GDB
didn't receive any SIGWINCH that should have resulted from
Term::resize's stty calls.

The reason for this is:

- The native-extended-gdbserver overrides gdb_start to first start GDB,
  then start GDBserver with --multi, then connect GDB to GDBserver.
  This means that two TCL "spawn"s are done, one for GDB and one for
  GDBserver.

- The TUI test framework  wants to know GDB's TTY name, so it can pass
  it to stty, to fake terminal resizes.  To do so, it overrides the
  spawn built-in proc to capture the tty name from the internals of the
  built-in proc.  It saves the TTY name to the gdb_spawn_name global
  variable.

- Because the native-extended-gdbserver boards starts both GDB and
  GDBserver, the final value of gdb_spawn_name is the name of
  GDBserver's TTY.

- When the TUI test framework attempts to resize GDB's terminal, it in
  fact resizes GDBserver's terminal.  So obviously, GDB doesn't get the
  SIGWINCH, and we don't get the expected TUI redraw.

Fix that by moving the hack to lib/gdb.exp, overriding the builtin spawn
all the time.  The override saves the TTY name in the
last_spawn_tty_name global.  The default_gdb_spawn proc then saves it in
the gdb_tty_name global.  This way, we specifically capture GDB's TTY
name in gdb_tty_name, not the TTY name of other spawned processes.

Remove tuiterm_env_init and tuiterm_env_finish, since they are now
empty.  In turn, the gdb_finish_hooks mechanism is now unused, remove it
as well.  It would be easy to add them back if needed.

gdb/ChangeLog:

	* lib/gdb.exp (default_gdb_exit): Unset gdb_tty_name.
	(spawn_capture_tty_name): New, override builtin spawn.
	(default_gdb_spawn): Capture GDB's TTY name.
	* lib/tuiterm.exp (tuiterm_spawn): Remove.
	(tuiterm_env_init, tuiterm_env_finish): Remove spawn override.
	(Term) <resize>: Use new variable name.
	(tuiterm_env_init, tuiterm_env_finish): Remove.
	(tuiterm_env): Don't call tuiterm_env_init and register
	tuiterm_env_finish in gdb_finish_hooks.
	(gdb_finish_hooks): Remove.
	(gdb_finish): Don't call finish hooks.

Change-Id: Ia5ab74184a52a996416022308f8d0cc523355a78
---
 gdb/testsuite/lib/gdb.exp     | 31 ++++++++++++++++++++++++-------
 gdb/testsuite/lib/tuiterm.exp | 35 ++---------------------------------
 2 files changed, 26 insertions(+), 40 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 8469ec9801c0..d8c684c72389 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1909,6 +1909,7 @@ proc default_gdb_exit {} {
 	remote_close host
     }
     unset gdb_spawn_id
+    unset ::gdb_tty_name
     unset inferior_spawn_id
 }
 
@@ -2037,6 +2038,28 @@ proc gdb_file_cmd { arg } {
     }
 }
 
+# The expect "spawn" function puts the tty name into the spawn_out
+# array; but dejagnu doesn't export this globally.  So, we have to
+# wrap spawn with our own function and poke in the built-in spawn
+# so that we can capture this value.
+#
+# If available, the TTY name is saved to the LAST_SPAWN_TTY_NAME global.
+# Otherwise, LAST_SPAWN_TTY_NAME is unset.
+
+proc spawn_capture_tty_name { args } {
+    set result [uplevel builtin_spawn $args]
+    upvar spawn_out spawn_out
+    if { [info exists spawn_out] } {
+	set ::last_spawn_tty_name $spawn_out(slave,name)
+    } else {
+	unset ::last_spawn_tty_name
+    }
+    return $result
+}
+
+rename spawn builtin_spawn
+rename spawn_capture_tty_name spawn
+
 # Default gdb_spawn procedure.
 
 proc default_gdb_spawn { } {
@@ -2074,6 +2097,7 @@ proc default_gdb_spawn { } {
     }
 
     set gdb_spawn_id $res
+    set ::gdb_tty_name $::last_spawn_tty_name
     return 0
 }
 
@@ -7800,13 +7824,6 @@ proc with_override { name override body } {
 # finalization function.
 proc tuiterm_env { } {
     load_lib tuiterm.exp
-
-    # Do initialization.
-    tuiterm_env_init
-
-    # Schedule finalization.
-    global gdb_finish_hooks
-    lappend gdb_finish_hooks tuiterm_env_finish
 }
 
 # Dejagnu has a version of note, but usage is not allowed outside of dejagnu.
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index fdd9f4d2188f..222583f291fb 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -15,36 +15,6 @@
 
 # An ANSI terminal emulator for expect.
 
-# The expect "spawn" function puts the tty name into the spawn_out
-# array; but dejagnu doesn't export this globally.  So, we have to
-# wrap spawn with our own function, so that we can capture this value.
-# The value is later used in calls to stty.
-proc tuiterm_spawn { args } {
-    set result [uplevel builtin_spawn $args]
-    global gdb_spawn_name
-    upvar spawn_out spawn_out
-    if { [info exists spawn_out] } {
-	set gdb_spawn_name $spawn_out(slave,name)
-    } else {
-	unset gdb_spawn_name
-    }
-    return $result
-}
-
-# Initialize tuiterm.exp environment.
-proc tuiterm_env_init { } {
-    # Override spawn with tui_spawn.
-    rename spawn builtin_spawn
-    rename tuiterm_spawn spawn
-}
-
-# Finalize tuiterm.exp environment.
-proc tuiterm_env_finish { } {
-    # Restore spawn.
-    rename spawn tuiterm_spawn
-    rename builtin_spawn spawn
-}
-
 namespace eval Term {
     # Size of the terminal.
     variable _rows
@@ -890,13 +860,12 @@ namespace eval Term {
 	variable _cols
 	variable _resize_count
 
-	global gdb_spawn_name
 	# expect handles each argument to stty separately.  This means
 	# that gdb will see SIGWINCH twice.  Rather than rely on this
 	# behavior (which, after all, could be changed), we make it
 	# explicit here.  This also simplifies waiting for the redraw.
 	_do_resize $rows $_cols
-	stty rows $_rows < $gdb_spawn_name
+	stty rows $_rows < $::gdb_tty_name
 	# Due to the strange column resizing behavior, and because we
 	# don't care about this intermediate resize, we don't check
 	# the size here.
@@ -906,7 +875,7 @@ namespace eval Term {
 	# than what we request from expect.  We hide this weird
 	# details from the caller.
 	_do_resize $_rows $cols
-	stty columns [expr {$_cols + 1}] < $gdb_spawn_name
+	stty columns [expr {$_cols + 1}] < $::gdb_tty_name
 	wait_for "@@ resize done $_resize_count, size = ${_cols}x${rows}"
 	incr _resize_count
     }
-- 
2.32.0

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Tom Tromey]

Simon> Here's the version that overrides all the time.

Looks good.  Thank you for doing this.

Tom

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Simon Marchi]

On 2021-06-10 2:26 p.m., Tom Tromey wrote:
> Simon> Here's the version that overrides all the time.
> 
> Looks good.  Thank you for doing this.
> 
> Tom
> 

Thanks, pushed.

Simon

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Bernd Edlinger]

On 6/10/21 8:32 PM, Simon Marchi via Gdb-patches wrote:
> 
> 
> On 2021-06-10 2:26 p.m., Tom Tromey wrote:
>> Simon> Here's the version that overrides all the time.
>>
>> Looks good.  Thank you for doing this.
>>
>> Tom
>>
> 
> Thanks, pushed.
> 
> Simon
> 
Hi,

every time I do "make check-gdb" since this patch,
I see a SIGSEGV in /usr/bin/expect at the end:

The symptoms are:

		=== gdb Summary ===

# of expected passes		80145
# of unexpected failures	867
# of expected failures		72
# of unknown successes		3
# of known failures		121
# of unresolved testcases	5
# of untested testcases		41
# of unsupported tests		257
# of duplicate test names	100
/home/ed/gnu/gdb-build-2/gdb/testsuite/../../gdb/gdb version  11.0.50.20210610-git -nw -nx -data-directory /home/ed/gnu/gdb-build-2/gdb/testsuite/../data-directory -iex "set height 0" -iex "set width 0" 

/bin/bash: line 1:  8471 Segmentation fault      (core dumped) runtest --status
make[3]: *** [check-single] Error 139
make[3]: Leaving directory `/home/ed/gnu/gdb-build-2/gdb/testsuite'
make[2]: *** [check] Error 2
make[2]: Leaving directory `/home/ed/gnu/gdb-build-2/gdb/testsuite'
make[1]: *** [check] Error 2
make[1]: Leaving directory `/home/ed/gnu/gdb-build-2/gdb'
make: *** [check-gdb] Error 2


the crash is always:

"expect -- /usr/share/dejagnu/runtest.exp --status"

crashed with SIGSEGV in Tcl_GetChannelName()

Do you have any idea what is wrong here, and how to fix it?


Bernd.

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Simon Marchi]

> every time I do "make check-gdb" since this patch,
> I see a SIGSEGV in /usr/bin/expect at the end:
> 
> The symptoms are:
> 
> 		=== gdb Summary ===
> 
> # of expected passes		80145
> # of unexpected failures	867
> # of expected failures		72
> # of unknown successes		3
> # of known failures		121
> # of unresolved testcases	5
> # of untested testcases		41
> # of unsupported tests		257
> # of duplicate test names	100
> /home/ed/gnu/gdb-build-2/gdb/testsuite/../../gdb/gdb version  11.0.50.20210610-git -nw -nx -data-directory /home/ed/gnu/gdb-build-2/gdb/testsuite/../data-directory -iex "set height 0" -iex "set width 0" 
> 
> /bin/bash: line 1:  8471 Segmentation fault      (core dumped) runtest --status
> make[3]: *** [check-single] Error 139
> make[3]: Leaving directory `/home/ed/gnu/gdb-build-2/gdb/testsuite'
> make[2]: *** [check] Error 2
> make[2]: Leaving directory `/home/ed/gnu/gdb-build-2/gdb/testsuite'
> make[1]: *** [check] Error 2
> make[1]: Leaving directory `/home/ed/gnu/gdb-build-2/gdb'
> make: *** [check-gdb] Error 2
> 
> 
> the crash is always:
> 
> "expect -- /usr/share/dejagnu/runtest.exp --status"
> 
> crashed with SIGSEGV in Tcl_GetChannelName()
> 
> Do you have any idea what is wrong here, and how to fix it?

I don't know, probably an Expect bug?  Does it happen when running any
test or when running a specific test?

What version do you have?  For reference, I have:

 - expect 5.45.4
 - tcl 8.6.11
 - dejagnu 1.6.2

And I don't see that.

 Simon

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Bernd Edlinger]

On 7/19/21 8:53 PM, Simon Marchi wrote:
>> every time I do "make check-gdb" since this patch,
>> I see a SIGSEGV in /usr/bin/expect at the end:
>>
>> The symptoms are:
>>
>> 		=== gdb Summary ===
>>
>> # of expected passes		80145
>> # of unexpected failures	867
>> # of expected failures		72
>> # of unknown successes		3
>> # of known failures		121
>> # of unresolved testcases	5
>> # of untested testcases		41
>> # of unsupported tests		257
>> # of duplicate test names	100
>> /home/ed/gnu/gdb-build-2/gdb/testsuite/../../gdb/gdb version  11.0.50.20210610-git -nw -nx -data-directory /home/ed/gnu/gdb-build-2/gdb/testsuite/../data-directory -iex "set height 0" -iex "set width 0" 
>>
>> /bin/bash: line 1:  8471 Segmentation fault      (core dumped) runtest --status
>> make[3]: *** [check-single] Error 139
>> make[3]: Leaving directory `/home/ed/gnu/gdb-build-2/gdb/testsuite'
>> make[2]: *** [check] Error 2
>> make[2]: Leaving directory `/home/ed/gnu/gdb-build-2/gdb/testsuite'
>> make[1]: *** [check] Error 2
>> make[1]: Leaving directory `/home/ed/gnu/gdb-build-2/gdb'
>> make: *** [check-gdb] Error 2
>>
>>
>> the crash is always:
>>
>> "expect -- /usr/share/dejagnu/runtest.exp --status"
>>
>> crashed with SIGSEGV in Tcl_GetChannelName()
>>
>> Do you have any idea what is wrong here, and how to fix it?
> 
> I don't know, probably an Expect bug?  Does it happen when running any
> test or when running a specific test?
> 
> What version do you have?  For reference, I have:
> 
>  - expect 5.45.4
>  - tcl 8.6.11
>  - dejagnu 1.6.2
> 
> And I don't see that.
> 

Hmm,

I see the same effect with

expect                                      5.45-5ubuntu1 
tcl                                         8.6.0+6ubuntu3
dejagnu                                     1.5-3ubuntu1

and 

expect                                        5.45.4-2build1
tcl                                           8.6.9+1
dejagnu                                       1.6.2-1


not sure what I made wrong, does anybody else have similar issues?

Bernd.

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Simon Marchi]

On 2021-07-19 3:05 p.m., Bernd Edlinger wrote:
> expect                                        5.45.4-2build1
> tcl                                           8.6.9+1
> dejagnu                                       1.6.2-1

For the record, I do see it on my Ubuntu 20.04 machine.  It can be
reproduced by running `runtest --status` manually in gdb/testsuite, that
could make it a bit easier to debug.

I attached GDB to runtest while it was doing its thing and waited for it
to segfault, that gave me the following backtrace (after installing the
appropriate -dbgsym packages):

    #0  0x00007fe176cd8b69 in exp_close (interp=interp@entry=0x5595b70269d0, esPtr=0x5595bb6968b0) at exp_command.c:370
    #1  0x00007fe176ceef9a in exp_close_all (interp=0x5595b70269d0) at exp_chan.c:582
    #2  0x00007fe176bd5b72 in InvokeExitHandlers () at ./generic/tclEvent.c:911
    #3  0x00007fe176bd5c0a in Tcl_Exit (status=1) at ./generic/tclEvent.c:976
    #4  0x00007fe176cdb959 in Exp_ExitObjCmd (clientData=<optimized out>, interp=0x5595b70269d0, objc=<optimized out>, objv=<optimized out>) at exp_command.c:2531
    #5  0x00007fe176b4d5f2 in TclNRRunCallbacks (interp=interp@entry=0x5595b70269d0, result=0, rootPtr=0x0) at ./generic/tclBasic.c:4492
    #6  0x00007fe176b4cfc5 in Tcl_EvalObjv (interp=interp@entry=0x5595b70269d0, objc=objc@entry=1, objv=objv@entry=0x5595b70342d0, flags=flags@entry=2097168) at ./generic/tclBasic.c:4215
    #7  0x00007fe176b4e924 in TclEvalEx (interp=interp@entry=0x5595b70269d0, script=0x5595b70ade40 "# runtest.exp -- Test framework driver\n# Copyright (C) 1992-2016 Free Software Foundation, Inc.\n#\n# This file is part of DejaGnu.\n#\n# DejaGnu is free software; you can redistribute it and/or modify it"..., numBytes=<optimized out>, flags=flags@entry=0, line=1908, line@entry=1, clNextOuter=clNextOuter@entry=0x0, outerScript=0x5595b70ade40 "# runtest.exp -- Test framework driver\n# Copyright (C) 1992-2016 Free Software Foundation, Inc.\n#\n# This file is part of DejaGnu.\n#\n# DejaGnu is free software; you can redistribute it and/or modify it"...) at ./generic/tclBasic.c:5361
    #8  0x00007fe176c08f79 in Tcl_FSEvalFileEx (encodingName=<optimized out>, pathPtr=0x5595b7063510, interp=0x5595b70269d0) at ./generic/tclIOUtil.c:1824
    #9  Tcl_FSEvalFileEx (interp=0x5595b70269d0, pathPtr=0x5595b7063510, encodingName=<optimized out>) at ./generic/tclIOUtil.c:1724
    #10 0x00007fe176c0784c in Tcl_EvalFile (interp=0x5595b70269d0, fileName=<optimized out>) at ./generic/tclIOUtil.c:424
    #11 0x00007fe176ce856e in exp_interpret_cmdfilename (interp=0x5595b70269d0, filename=0x7fff61eef573 "/usr/share/dejagnu/runtest.exp") at exp_main_sub.c:953
    #12 0x00005595b6cad2e9 in main (argc=4, argv=0x7fff61eeee38) at exp_main_exp.c:48

Simon

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Bernd Edlinger]

On 7/20/21 7:14 AM, Simon Marchi wrote:
> On 2021-07-19 3:05 p.m., Bernd Edlinger wrote:
>> expect                                        5.45.4-2build1
>> tcl                                           8.6.9+1
>> dejagnu                                       1.6.2-1
> 
> For the record, I do see it on my Ubuntu 20.04 machine.  It can be
> reproduced by running `runtest --status` manually in gdb/testsuite, that
> could make it a bit easier to debug.
> 
> I attached GDB to runtest while it was doing its thing and waited for it
> to segfault, that gave me the following backtrace (after installing the
> appropriate -dbgsym packages):
> 
>     #0  0x00007fe176cd8b69 in exp_close (interp=interp@entry=0x5595b70269d0, esPtr=0x5595bb6968b0) at exp_command.c:370
>     #1  0x00007fe176ceef9a in exp_close_all (interp=0x5595b70269d0) at exp_chan.c:582
>     #2  0x00007fe176bd5b72 in InvokeExitHandlers () at ./generic/tclEvent.c:911
>     #3  0x00007fe176bd5c0a in Tcl_Exit (status=1) at ./generic/tclEvent.c:976
>     #4  0x00007fe176cdb959 in Exp_ExitObjCmd (clientData=<optimized out>, interp=0x5595b70269d0, objc=<optimized out>, objv=<optimized out>) at exp_command.c:2531
>     #5  0x00007fe176b4d5f2 in TclNRRunCallbacks (interp=interp@entry=0x5595b70269d0, result=0, rootPtr=0x0) at ./generic/tclBasic.c:4492
>     #6  0x00007fe176b4cfc5 in Tcl_EvalObjv (interp=interp@entry=0x5595b70269d0, objc=objc@entry=1, objv=objv@entry=0x5595b70342d0, flags=flags@entry=2097168) at ./generic/tclBasic.c:4215
>     #7  0x00007fe176b4e924 in TclEvalEx (interp=interp@entry=0x5595b70269d0, script=0x5595b70ade40 "# runtest.exp -- Test framework driver\n# Copyright (C) 1992-2016 Free Software Foundation, Inc.\n#\n# This file is part of DejaGnu.\n#\n# DejaGnu is free software; you can redistribute it and/or modify it"..., numBytes=<optimized out>, flags=flags@entry=0, line=1908, line@entry=1, clNextOuter=clNextOuter@entry=0x0, outerScript=0x5595b70ade40 "# runtest.exp -- Test framework driver\n# Copyright (C) 1992-2016 Free Software Foundation, Inc.\n#\n# This file is part of DejaGnu.\n#\n# DejaGnu is free software; you can redistribute it and/or modify it"...) at ./generic/tclBasic.c:5361
>     #8  0x00007fe176c08f79 in Tcl_FSEvalFileEx (encodingName=<optimized out>, pathPtr=0x5595b7063510, interp=0x5595b70269d0) at ./generic/tclIOUtil.c:1824
>     #9  Tcl_FSEvalFileEx (interp=0x5595b70269d0, pathPtr=0x5595b7063510, encodingName=<optimized out>) at ./generic/tclIOUtil.c:1724
>     #10 0x00007fe176c0784c in Tcl_EvalFile (interp=0x5595b70269d0, fileName=<optimized out>) at ./generic/tclIOUtil.c:424
>     #11 0x00007fe176ce856e in exp_interpret_cmdfilename (interp=0x5595b70269d0, filename=0x7fff61eef573 "/usr/share/dejagnu/runtest.exp") at exp_main_sub.c:953
>     #12 0x00005595b6cad2e9 in main (argc=4, argv=0x7fff61eeee38) at exp_main_exp.c:48
> 

I narrowed it down to the following test case:

make check-gdb RUNTESTFLAGS="gdb.base/gnu-debugdata.exp"


Bernd.

Re: [PATCH] gdb/testsuite: capture GDB tty name in default_gdb_spawn

[Bernd Edlinger]

On 7/20/21 10:28 PM, Bernd Edlinger wrote:
> On 7/20/21 7:14 AM, Simon Marchi wrote:
>> On 2021-07-19 3:05 p.m., Bernd Edlinger wrote:
>>> expect                                        5.45.4-2build1
>>> tcl                                           8.6.9+1
>>> dejagnu                                       1.6.2-1
>>
>> For the record, I do see it on my Ubuntu 20.04 machine.  It can be
>> reproduced by running `runtest --status` manually in gdb/testsuite, that
>> could make it a bit easier to debug.
>>
>> I attached GDB to runtest while it was doing its thing and waited for it
>> to segfault, that gave me the following backtrace (after installing the
>> appropriate -dbgsym packages):
>>
>>     #0  0x00007fe176cd8b69 in exp_close (interp=interp@entry=0x5595b70269d0, esPtr=0x5595bb6968b0) at exp_command.c:370
>>     #1  0x00007fe176ceef9a in exp_close_all (interp=0x5595b70269d0) at exp_chan.c:582
>>     #2  0x00007fe176bd5b72 in InvokeExitHandlers () at ./generic/tclEvent.c:911
>>     #3  0x00007fe176bd5c0a in Tcl_Exit (status=1) at ./generic/tclEvent.c:976
>>     #4  0x00007fe176cdb959 in Exp_ExitObjCmd (clientData=<optimized out>, interp=0x5595b70269d0, objc=<optimized out>, objv=<optimized out>) at exp_command.c:2531
>>     #5  0x00007fe176b4d5f2 in TclNRRunCallbacks (interp=interp@entry=0x5595b70269d0, result=0, rootPtr=0x0) at ./generic/tclBasic.c:4492
>>     #6  0x00007fe176b4cfc5 in Tcl_EvalObjv (interp=interp@entry=0x5595b70269d0, objc=objc@entry=1, objv=objv@entry=0x5595b70342d0, flags=flags@entry=2097168) at ./generic/tclBasic.c:4215
>>     #7  0x00007fe176b4e924 in TclEvalEx (interp=interp@entry=0x5595b70269d0, script=0x5595b70ade40 "# runtest.exp -- Test framework driver\n# Copyright (C) 1992-2016 Free Software Foundation, Inc.\n#\n# This file is part of DejaGnu.\n#\n# DejaGnu is free software; you can redistribute it and/or modify it"..., numBytes=<optimized out>, flags=flags@entry=0, line=1908, line@entry=1, clNextOuter=clNextOuter@entry=0x0, outerScript=0x5595b70ade40 "# runtest.exp -- Test framework driver\n# Copyright (C) 1992-2016 Free Software Foundation, Inc.\n#\n# This file is part of DejaGnu.\n#\n# DejaGnu is free software; you can redistribute it and/or modify it"...) at ./generic/tclBasic.c:5361
>>     #8  0x00007fe176c08f79 in Tcl_FSEvalFileEx (encodingName=<optimized out>, pathPtr=0x5595b7063510, interp=0x5595b70269d0) at ./generic/tclIOUtil.c:1824
>>     #9  Tcl_FSEvalFileEx (interp=0x5595b70269d0, pathPtr=0x5595b7063510, encodingName=<optimized out>) at ./generic/tclIOUtil.c:1724
>>     #10 0x00007fe176c0784c in Tcl_EvalFile (interp=0x5595b70269d0, fileName=<optimized out>) at ./generic/tclIOUtil.c:424
>>     #11 0x00007fe176ce856e in exp_interpret_cmdfilename (interp=0x5595b70269d0, filename=0x7fff61eef573 "/usr/share/dejagnu/runtest.exp") at exp_main_sub.c:953
>>     #12 0x00005595b6cad2e9 in main (argc=4, argv=0x7fff61eeee38) at exp_main_exp.c:48
>>
> 
> I narrowed it down to the following test case:
> 
> make check-gdb RUNTESTFLAGS="gdb.base/gnu-debugdata.exp"
> 

This seems to fix the problem for me:

--- a/gdb/testsuite/gdb.base/gnu-debugdata.exp
+++ b/gdb/testsuite/gdb.base/gnu-debugdata.exp
@@ -56,6 +56,7 @@ proc pipeline {test args} {
                 $input $output]} {
            return -1
        }
+       unset ::last_spawn_tty_name
 
        set input_file $output
     }


No Idea what is going on here,
there are other places where run_on_host
is called, and I don't see how run_on_host
can call spawn, it calls [eval remote_exec ...]
but I don't see how that is implemented.

How can last_spawn_tty_name cause an expect crash?

It appears to me, that changing the builtin spawn command
causes some kind of undefined behavior.


Bernd.