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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

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
>