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

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

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