Re: [PATCH][gdb/testsuite] Warn about leaked global array

[Tom de Vries <tdevries@suse.de>]

On 18-05-2020 12:41, Pedro Alves wrote:
> On 5/18/20 7:18 AM, Tom de Vries wrote:
> 
>> I managed to find a better way of dealing with this, using "upvar #0".
>> I've also added more comments to the patch.
> 
> Cool.
> 
>> +# Check global variables not in gdb_global_vars.
>> +
>> +proc check_global_vars { } {
>> +    # Sample state after running test.
>> +    global gdb_global_vars
>> +    set vars [info globals]
>> +
>> +    # I'm not sure these two should actually be global, but at least there
>> +    # seems to be no harm in having these as globals, given that we don't
>> +    # expect to reuse these names as scalars.
>> +    set skip [list "expect_out" "spawn_out"]
> 
> They can be local or global.  See man expect:
> 
>  Expect takes a rather liberal view of scoping. In particular, variables read 
>  by commands specific to the Expect program will be sought first from the local
>  scope, and if not found, in the global scope. For example, this obviates the need to
>  place "global timeout" in every procedure you   write that uses expect. On the other hand,
>  variables written are always in the local scope (unless a "global" command has been issued).
>  The most common problem this causes is when spawn is executed in a procedure.
>  Outside the procedure, spawn_id no longer exists, so the spawned process is no
>  longer accessible simply because of scoping. Add a "global spawn_id" to such a procedure. 
> 
> For example, mi_gdb_test does:
> 
> proc mi_gdb_test { args } {
>     global verbose
>     global mi_gdb_prompt
>     global GDB expect_out
>                ^^^^^^^^^^
>     global inferior_exited_re async
>     upvar timeout timeout
> 
> and then gdb.mi/mi-basics.exp does:
> 
> proc test_path_specification {} {
> ...
>     global expect_out
> 
> ...
>     mi_gdb_test "-environment-path" "\\\^done,path=\"(.*)\"" "environment-path"
>     set orig_path $expect_out(3,string)
> ...
> 
> So making expect_out global allows gdb.mi/mi-basics.exp to reference it.
> 
> We could alternatively switch mi_gdb_test (and whatever other procedure
> does a similar thing) to doing "upvar expect_out expect_out" and
> get rid of "global expect_out".  Not sure it's worth the bother.
> 

OK, that's helpful, thanks for the info.

>> +	global gdb_test_file_name
>> +	warning "$gdb_test_file_name.exp defined global array $var"
>> +
>> +	# If the variable remains set, we won't warn for the next test where
>> +	# it's leaked, so unset.
>> +        global_unset $var
> 
> Tabs / spaces above.
> 

Fixed, and resubmitted as part of a patch series that also includes
warning fixes here (
https://sourceware.org/pipermail/gdb-patches/2020-May/168771.html ).

Also, fixing the warnings turned out to be more complicated than I
thought, so I ended up moving the bit about how to fix the warning into
a gdb/testsuite/README section (part of patch 2/3).

Thanks,
- Tom