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

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

[Bernd Edlinger]

due to unknown reasons the runtest process crashes
at the end of the testrun when this script is
executed.

    #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

This started after
commit 9edb1e0191e ("gdb/testsuite: capture GDB tty name in default_gdb_spawn")

Temporarily renaming builtin_spawn to spawn fixes the issue.

2021-07-22  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gdb.base/gnu-debugdata.exp (pipeline): Temporarily undo the effect of
	commit 9edb1e0191e in this function.
---
Hi,

Is this OK for trunk, and the gdb-11-branch?


Thanks
Bernd.

 gdb/testsuite/gdb.base/gnu-debugdata.exp | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdb/testsuite/gdb.base/gnu-debugdata.exp b/gdb/testsuite/gdb.base/gnu-debugdata.exp
index f6d0cd7..0dc8191 100644
--- a/gdb/testsuite/gdb.base/gnu-debugdata.exp
+++ b/gdb/testsuite/gdb.base/gnu-debugdata.exp
@@ -52,11 +52,20 @@ proc pipeline {test args} {
 	}
 	verbose "cooked args are [list $program $arguments $input $output]"
 
+	# undo the effect of spawn_capture_tty_name
+	# this avoids a crash at the end of the test run
+	rename spawn dummy_spawn
+	rename builtin_spawn spawn
+
 	if {[run_on_host "$test - invoke $program" $program $arguments \
 		 $input $output]} {
 	    return -1
 	}
 
+	# restore the original spawn script
+	rename spawn builtin_spawn
+	rename dummy_spawn spawn
+
 	set input_file $output
     }
     return 0
-- 
1.9.1

Re: [PATCH] Fix the crash at the end of the runtest

[Andrew Burgess]

* Bernd Edlinger <bernd.edlinger@hotmail.de> [2021-07-22 13:10:11 +0200]:

> due to unknown reasons the runtest process crashes
> at the end of the testrun when this script is
> executed.
> 
>     #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
> 
> This started after
> commit 9edb1e0191e ("gdb/testsuite: capture GDB tty name in default_gdb_spawn")
> 
> Temporarily renaming builtin_spawn to spawn fixes the issue.
> 
> 2021-07-22  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	* gdb.base/gnu-debugdata.exp (pipeline): Temporarily undo the effect of
> 	commit 9edb1e0191e in this function.

Weirdly, when I try to run this test (without your patch) I see this
error (passing -v in RUNTESTFLAGS):

  spawn -open file11 failed, 1 can't unset "::last_spawn_tty_name": no such variable, can't unset "::last_spawn_tty_name": no such variable
      while executing
  "unset ::last_spawn_tty_name"
      (procedure "spawn" line 7)
      invoked from within
  "spawn -ignore SIGHUP -leaveopen file11"
      invoked from within
  "catch "spawn -ignore SIGHUP -leaveopen $id" result2"
  result is -1 {spawn failed}
  run_on_host failed: spawn failed

It appears to be choking on the call to unset in:

  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
  }

as ::last_spawn_tty_name has never previous been set.

And indeed, I am seeing runtest crash as you say.

However, I applied this patch:

----- PATCH START -----

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e79e0622f9d..deab2420075 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2031,7 +2031,7 @@ proc spawn_capture_tty_name { args } {
     upvar spawn_out spawn_out
     if { [info exists spawn_out] } {
 	set ::last_spawn_tty_name $spawn_out(slave,name)
-    } else {
+    } elseif { [info exists ::last_spawn_tty_name] } {
 	unset ::last_spawn_tty_name
     }
     return $result

----- PATCH END -----

And I can now run the test successfully, and I'm not seeing runtest
crash any more.

Can you confirm you see the same behaviour?

Thanks,
Andrew



> ---
> Hi,
> 
> Is this OK for trunk, and the gdb-11-branch?
> 
> 
> Thanks
> Bernd.
> 
>  gdb/testsuite/gdb.base/gnu-debugdata.exp | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.base/gnu-debugdata.exp b/gdb/testsuite/gdb.base/gnu-debugdata.exp
> index f6d0cd7..0dc8191 100644
> --- a/gdb/testsuite/gdb.base/gnu-debugdata.exp
> +++ b/gdb/testsuite/gdb.base/gnu-debugdata.exp
> @@ -52,11 +52,20 @@ proc pipeline {test args} {
>  	}
>  	verbose "cooked args are [list $program $arguments $input $output]"
>  
> +	# undo the effect of spawn_capture_tty_name
> +	# this avoids a crash at the end of the test run
> +	rename spawn dummy_spawn
> +	rename builtin_spawn spawn
> +
>  	if {[run_on_host "$test - invoke $program" $program $arguments \
>  		 $input $output]} {
>  	    return -1
>  	}
>  
> +	# restore the original spawn script
> +	rename spawn builtin_spawn
> +	rename dummy_spawn spawn
> +
>  	set input_file $output
>      }
>      return 0
> -- 
> 1.9.1

Re: [PATCH] Fix the crash at the end of the runtest

[Bernd Edlinger]

On 7/22/21 2:44 PM, Andrew Burgess wrote:
> * Bernd Edlinger <bernd.edlinger@hotmail.de> [2021-07-22 13:10:11 +0200]:
> 
>> due to unknown reasons the runtest process crashes
>> at the end of the testrun when this script is
>> executed.
>>
>>     #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
>>
>> This started after
>> commit 9edb1e0191e ("gdb/testsuite: capture GDB tty name in default_gdb_spawn")
>>
>> Temporarily renaming builtin_spawn to spawn fixes the issue.
>>
>> 2021-07-22  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	* gdb.base/gnu-debugdata.exp (pipeline): Temporarily undo the effect of
>> 	commit 9edb1e0191e in this function.
> 
> Weirdly, when I try to run this test (without your patch) I see this
> error (passing -v in RUNTESTFLAGS):
> 
>   spawn -open file11 failed, 1 can't unset "::last_spawn_tty_name": no such variable, can't unset "::last_spawn_tty_name": no such variable
>       while executing
>   "unset ::last_spawn_tty_name"
>       (procedure "spawn" line 7)
>       invoked from within
>   "spawn -ignore SIGHUP -leaveopen file11"
>       invoked from within
>   "catch "spawn -ignore SIGHUP -leaveopen $id" result2"
>   result is -1 {spawn failed}
>   run_on_host failed: spawn failed
> 
> It appears to be choking on the call to unset in:
> 
>   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
>   }
> 
> as ::last_spawn_tty_name has never previous been set.
> 
> And indeed, I am seeing runtest crash as you say.
> 
> However, I applied this patch:
> 
> ----- PATCH START -----
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index e79e0622f9d..deab2420075 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -2031,7 +2031,7 @@ proc spawn_capture_tty_name { args } {
>      upvar spawn_out spawn_out
>      if { [info exists spawn_out] } {
>  	set ::last_spawn_tty_name $spawn_out(slave,name)
> -    } else {
> +    } elseif { [info exists ::last_spawn_tty_name] } {
>  	unset ::last_spawn_tty_name
>      }
>      return $result
> 
> ----- PATCH END -----
> 
> And I can now run the test successfully, and I'm not seeing runtest
> crash any more.
> 
> Can you confirm you see the same behaviour?
> 

Yes, I can confirm.

I do see the same behavior.


Thanks
Bernd.

> Thanks,
> Andrew
> 
> 
> 
>> ---
>> Hi,
>>
>> Is this OK for trunk, and the gdb-11-branch?
>>
>>
>> Thanks
>> Bernd.
>>
>>  gdb/testsuite/gdb.base/gnu-debugdata.exp | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/gdb/testsuite/gdb.base/gnu-debugdata.exp b/gdb/testsuite/gdb.base/gnu-debugdata.exp
>> index f6d0cd7..0dc8191 100644
>> --- a/gdb/testsuite/gdb.base/gnu-debugdata.exp
>> +++ b/gdb/testsuite/gdb.base/gnu-debugdata.exp
>> @@ -52,11 +52,20 @@ proc pipeline {test args} {
>>  	}
>>  	verbose "cooked args are [list $program $arguments $input $output]"
>>  
>> +	# undo the effect of spawn_capture_tty_name
>> +	# this avoids a crash at the end of the test run
>> +	rename spawn dummy_spawn
>> +	rename builtin_spawn spawn
>> +
>>  	if {[run_on_host "$test - invoke $program" $program $arguments \
>>  		 $input $output]} {
>>  	    return -1
>>  	}
>>  
>> +	# restore the original spawn script
>> +	rename spawn builtin_spawn
>> +	rename dummy_spawn spawn
>> +
>>  	set input_file $output
>>      }
>>      return 0
>> -- 
>> 1.9.1

Re: [PATCH] Fix the crash at the end of the runtest

[Andrew Burgess]

Here's my proposed fix, along with an explanation of what's going on.

Thanks,
Andrew

---

commit c6d3cd6e265dd39a27d13428eba48df8c1d50c95
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 exists or not.
    
    One situation that might cause spawn_out 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 pipe line 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, no spawn_out is set, and so in
    spawn_capture_tty_name we unset last_spawn_tty_name.  However, on the
    second call to spawn there is still no spawn_out and so in
    spawn_capture_tty_name we again try to unset last_spawn_tty_name, this
    now throws an error.
    
    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.

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e79e0622f9d..8712669bdce 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2032,7 +2032,7 @@ proc spawn_capture_tty_name { args } {
     if { [info exists spawn_out] } {
 	set ::last_spawn_tty_name $spawn_out(slave,name)
     } else {
-	unset ::last_spawn_tty_name
+	unset -nocomplain ::last_spawn_tty_name
     }
     return $result
 }

Re: [PATCH] Fix the crash at the end of the runtest

[Simon Marchi]

On 2021-07-22 9:20 a.m., Andrew Burgess wrote:
> Here's my proposed fix, along with an explanation of what's going on.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit c6d3cd6e265dd39a27d13428eba48df8c1d50c95
> 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 exists or not.
>     
>     One situation that might cause spawn_out 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 pipe line 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, no spawn_out is set, and so in
>     spawn_capture_tty_name we unset last_spawn_tty_name.  However, on the
>     second call to spawn there is still no spawn_out and so in
>     spawn_capture_tty_name we again try to unset last_spawn_tty_name, this
>     now throws an error.
>     
>     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.
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index e79e0622f9d..8712669bdce 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -2032,7 +2032,7 @@ proc spawn_capture_tty_name { args } {
>      if { [info exists spawn_out] } {
>  	set ::last_spawn_tty_name $spawn_out(slave,name)

Not caused by your patch, but: pedantically, the "if" should perhaps
verify that "spawn_out(slave,name)" specifically exists, not just
"spawn_out".  Currently (according to "man expect"),
"spawn_out(slave,name)" is the only possible "spawn_out" contents.  But
let's say that the next Expect version sets "spawn_out(foo)", then that
code will break (we will enter the "if" and try to read
"spawn_out(slave,name)", which may not exist).

>      } else {
> -	unset ::last_spawn_tty_name
> +	unset -nocomplain ::last_spawn_tty_name

Perhaps a small comment above this line would be nice.

Otherwise, LGTM with or without changes, thanks for looking into it.

Simon

Re: [PATCH] Fix the crash at the end of the runtest

[Bernd Edlinger]

On 7/22/21 5:14 PM, Simon Marchi wrote:
> On 2021-07-22 9:20 a.m., Andrew Burgess wrote:
>> Here's my proposed fix, along with an explanation of what's going on.
>>
>> Thanks,
>> Andrew
>>
>> ---
>>
>> commit c6d3cd6e265dd39a27d13428eba48df8c1d50c95
>> 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 exists or not.
>>     
>>     One situation that might cause spawn_out 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 pipe line 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, no spawn_out is set, and so in
>>     spawn_capture_tty_name we unset last_spawn_tty_name.  However, on the
>>     second call to spawn there is still no spawn_out and so in
>>     spawn_capture_tty_name we again try to unset last_spawn_tty_name, this
>>     now throws an error.
>>     
>>     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.
>>

this looks quite good,
but should we mention the fixed crash here?

Thanks
Bernd.


>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index e79e0622f9d..8712669bdce 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -2032,7 +2032,7 @@ proc spawn_capture_tty_name { args } {
>>      if { [info exists spawn_out] } {
>>  	set ::last_spawn_tty_name $spawn_out(slave,name)
> 
> Not caused by your patch, but: pedantically, the "if" should perhaps
> verify that "spawn_out(slave,name)" specifically exists, not just
> "spawn_out".  Currently (according to "man expect"),
> "spawn_out(slave,name)" is the only possible "spawn_out" contents.  But
> let's say that the next Expect version sets "spawn_out(foo)", then that
> code will break (we will enter the "if" and try to read
> "spawn_out(slave,name)", which may not exist).
> 
>>      } else {
>> -	unset ::last_spawn_tty_name
>> +	unset -nocomplain ::last_spawn_tty_name
> 
> Perhaps a small comment above this line would be nice.
> 
> Otherwise, LGTM with or without changes, thanks for looking into it.
> 
> Simon
> 

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

[Andrew Burgess]

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
 }

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

[Simon Marchi]

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