[PATCHv2] Fix the crash at the end of the runtest

Message ID 20210722165334.GE1872618@embecosm.com
State New
Headers show
Series
  • [PATCHv2] Fix the crash at the end of the runtest
Related show

Commit Message

Andrew Burgess July 22, 2021, 4:53 p.m.
Thanks for the feedback.  Below is the patch I intend to push first
thing tomorrow unless anyone objects.

Thanks,
Andrew

---

commit 41424acf01830d3cafb838a7d865985182edf47a
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Thu Jul 22 14:07:15 2021 +0100

    gdb/testsuite: don't error when trying to unset last_spawn_tty_name
    
    In spawn_capture_tty_name (lib/gdb.exp) we either set or unset
    last_spawn_tty_name depending on whether spawn_out(slave,name) exists
    or not.
    
    One situation that might cause spawn_out(slave,name) to not exists is
    if the spawn function is called with the argument -leaveopen, which is
    how it is called when processes are created as part of a pipeline, the
    created process has no tty, instead its output is written to a file
    descriptor.
    
    If a pipeline is created consisting of multiple processes then there
    will be multiple sequential calls to spawn, all using -leaveopen.  The
    first of these calls is fine, spawn_out(slave,name) is not set, and so
    in spawn_capture_tty_name we unset last_spawn_tty_name.  However, on
    the second call to spawn, spawn_out(slave,name) is still not set and
    so in spawn_capture_tty_name we again try to unset
    last_spawn_tty_name, this now throws an error (as last_spawn_tty_name
    is already unset).
    
    Fix this issue by using -nocomplain with the call to unset in
    spawn_capture_tty_name.
    
    Before this commit I was seeing gdb.base/gnu-debugdata.exp report 1
    pass, and 1 unsupported test.  After this commit I now see 16 passes
    from this test script.
    
    I have also improved the code that used to do this:
    
        if { [info exists spawn_out] } {
            set ::last_spawn_tty_name $spawn_out(slave,name)
        } else {
            ...
        }
    
    The problem here is that we check for the existence of spawn_out, and
    then unconditionally read spawn_out(slave,name).  A situation could
    arise where some other element of spawn_out is set,
    e.g. spawn_out(foo), in which case we would enter the if block and try
    to read a non-existent variable.  After this commit we now check
    specifically for spawn_out(slave,name).
    
    Finally, it is worth noting that before this issue was fixed runtest
    itself, or rather the expect process behind runtest, would segfault
    while exiting.  I haven't looked at all into what the problem is here
    that caused expect to crash, as fixing the bug in GDB's testing
    scripts made the segfault go away.

Comments

Tom de Vries via Gdb-patches July 22, 2021, 7:24 p.m. | #1
On 2021-07-22 12:53 p.m., Andrew Burgess wrote:
> Thanks for the feedback.  Below is the patch I intend to push first

> thing tomorrow unless anyone objects.

> 

> Thanks,

> Andrew

> 

> ---

> 

> commit 41424acf01830d3cafb838a7d865985182edf47a

> Author: Andrew Burgess <andrew.burgess@embecosm.com>

> Date:   Thu Jul 22 14:07:15 2021 +0100

> 

>     gdb/testsuite: don't error when trying to unset last_spawn_tty_name

>     

>     In spawn_capture_tty_name (lib/gdb.exp) we either set or unset

>     last_spawn_tty_name depending on whether spawn_out(slave,name) exists

>     or not.

>     

>     One situation that might cause spawn_out(slave,name) to not exists is

>     if the spawn function is called with the argument -leaveopen, which is

>     how it is called when processes are created as part of a pipeline, the

>     created process has no tty, instead its output is written to a file

>     descriptor.

>     

>     If a pipeline is created consisting of multiple processes then there

>     will be multiple sequential calls to spawn, all using -leaveopen.  The

>     first of these calls is fine, spawn_out(slave,name) is not set, and so

>     in spawn_capture_tty_name we unset last_spawn_tty_name.  However, on

>     the second call to spawn, spawn_out(slave,name) is still not set and

>     so in spawn_capture_tty_name we again try to unset

>     last_spawn_tty_name, this now throws an error (as last_spawn_tty_name

>     is already unset).

>     

>     Fix this issue by using -nocomplain with the call to unset in

>     spawn_capture_tty_name.

>     

>     Before this commit I was seeing gdb.base/gnu-debugdata.exp report 1

>     pass, and 1 unsupported test.  After this commit I now see 16 passes

>     from this test script.

>     

>     I have also improved the code that used to do this:

>     

>         if { [info exists spawn_out] } {

>             set ::last_spawn_tty_name $spawn_out(slave,name)

>         } else {

>             ...

>         }

>     

>     The problem here is that we check for the existence of spawn_out, and

>     then unconditionally read spawn_out(slave,name).  A situation could

>     arise where some other element of spawn_out is set,

>     e.g. spawn_out(foo), in which case we would enter the if block and try

>     to read a non-existent variable.  After this commit we now check

>     specifically for spawn_out(slave,name).

>     

>     Finally, it is worth noting that before this issue was fixed runtest

>     itself, or rather the expect process behind runtest, would segfault

>     while exiting.  I haven't looked at all into what the problem is here

>     that caused expect to crash, as fixing the bug in GDB's testing

>     scripts made the segfault go away.

> 

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp

> index e79e0622f9d..aeb8585bab9 100644

> --- a/gdb/testsuite/lib/gdb.exp

> +++ b/gdb/testsuite/lib/gdb.exp

> @@ -2029,10 +2029,20 @@ proc gdb_file_cmd { arg } {

>  proc spawn_capture_tty_name { args } {

>      set result [uplevel builtin_spawn $args]

>      upvar spawn_out spawn_out

> -    if { [info exists spawn_out] } {

> +    if { [info exists spawn_out(slave,name)] } {

>  	set ::last_spawn_tty_name $spawn_out(slave,name)

>      } else {

> -	unset ::last_spawn_tty_name

> +	# If a process is spawned as part of a pipe line (e.g. passing

> +	# -leaveopen to the spawn proc) then the spawned process is no

> +	# assigned a tty and spawn_out(slave,name) will not be set.

> +	# In that case we want to ensure that last_spawn_tty_name is

> +	# not set.

> +	#

> +	# If the previous process spawned was also not assigned a tty

> +	# (e.g. multiple processed chained in a pipeline) then

> +	# last_spawn_tty_name will already be unset, so, if we don't

> +	# use -nocomplain here we would otherwise get an error.

> +	unset -nocomplain ::last_spawn_tty_name

>      }

>      return $result

>  }

> 


LGTM, thanks!

Simon

Patch

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e79e0622f9d..aeb8585bab9 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2029,10 +2029,20 @@  proc gdb_file_cmd { arg } {
 proc spawn_capture_tty_name { args } {
     set result [uplevel builtin_spawn $args]
     upvar spawn_out spawn_out
-    if { [info exists spawn_out] } {
+    if { [info exists spawn_out(slave,name)] } {
 	set ::last_spawn_tty_name $spawn_out(slave,name)
     } else {
-	unset ::last_spawn_tty_name
+	# If a process is spawned as part of a pipe line (e.g. passing
+	# -leaveopen to the spawn proc) then the spawned process is no
+	# assigned a tty and spawn_out(slave,name) will not be set.
+	# In that case we want to ensure that last_spawn_tty_name is
+	# not set.
+	#
+	# If the previous process spawned was also not assigned a tty
+	# (e.g. multiple processed chained in a pipeline) then
+	# last_spawn_tty_name will already be unset, so, if we don't
+	# use -nocomplain here we would otherwise get an error.
+	unset -nocomplain ::last_spawn_tty_name
     }
     return $result
 }