* [PATCH] Fix the crash at the end of the runtest
@ 2021-07-22 11:10 Bernd Edlinger
2021-07-22 12:44 ` Andrew Burgess
0 siblings, 1 reply; 8+ messages in thread
From: Bernd Edlinger @ 2021-07-22 11:10 UTC (permalink / raw)
To: gdb-patches, Simon Marchi
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix the crash at the end of the runtest
2021-07-22 11:10 [PATCH] Fix the crash at the end of the runtest Bernd Edlinger
@ 2021-07-22 12:44 ` Andrew Burgess
2021-07-22 12:59 ` Bernd Edlinger
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Burgess @ 2021-07-22 12:44 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: gdb-patches, Simon Marchi
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix the crash at the end of the runtest
2021-07-22 12:44 ` Andrew Burgess
@ 2021-07-22 12:59 ` Bernd Edlinger
2021-07-22 13:20 ` Andrew Burgess
0 siblings, 1 reply; 8+ messages in thread
From: Bernd Edlinger @ 2021-07-22 12:59 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches, Simon Marchi
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix the crash at the end of the runtest
2021-07-22 12:59 ` Bernd Edlinger
@ 2021-07-22 13:20 ` Andrew Burgess
2021-07-22 15:14 ` Simon Marchi
2021-07-22 16:53 ` [PATCHv2] " Andrew Burgess
0 siblings, 2 replies; 8+ messages in thread
From: Andrew Burgess @ 2021-07-22 13:20 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: gdb-patches, Simon Marchi
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
}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix the crash at the end of the runtest
2021-07-22 13:20 ` Andrew Burgess
@ 2021-07-22 15:14 ` Simon Marchi
2021-07-22 15:16 ` Bernd Edlinger
2021-07-22 16:53 ` [PATCHv2] " Andrew Burgess
1 sibling, 1 reply; 8+ messages in thread
From: Simon Marchi @ 2021-07-22 15:14 UTC (permalink / raw)
To: Andrew Burgess, Bernd Edlinger; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix the crash at the end of the runtest
2021-07-22 15:14 ` Simon Marchi
@ 2021-07-22 15:16 ` Bernd Edlinger
0 siblings, 0 replies; 8+ messages in thread
From: Bernd Edlinger @ 2021-07-22 15:16 UTC (permalink / raw)
To: Simon Marchi, Andrew Burgess; +Cc: gdb-patches
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv2] Fix the crash at the end of the runtest
2021-07-22 13:20 ` Andrew Burgess
2021-07-22 15:14 ` Simon Marchi
@ 2021-07-22 16:53 ` Andrew Burgess
2021-07-22 19:24 ` Simon Marchi
1 sibling, 1 reply; 8+ messages in thread
From: Andrew Burgess @ 2021-07-22 16:53 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: gdb-patches, Simon Marchi
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
}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2] Fix the crash at the end of the runtest
2021-07-22 16:53 ` [PATCHv2] " Andrew Burgess
@ 2021-07-22 19:24 ` Simon Marchi
0 siblings, 0 replies; 8+ messages in thread
From: Simon Marchi @ 2021-07-22 19:24 UTC (permalink / raw)
To: Andrew Burgess, Bernd Edlinger; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-22 19:24 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-22 11:10 [PATCH] Fix the crash at the end of the runtest Bernd Edlinger
2021-07-22 12:44 ` Andrew Burgess
2021-07-22 12:59 ` Bernd Edlinger
2021-07-22 13:20 ` Andrew Burgess
2021-07-22 15:14 ` Simon Marchi
2021-07-22 15:16 ` Bernd Edlinger
2021-07-22 16:53 ` [PATCHv2] " Andrew Burgess
2021-07-22 19:24 ` Simon Marchi
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).