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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

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