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

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

On 03-06-2020 11:49, Pedro Alves wrote:
> On 6/2/20 4:38 PM, Andrew Burgess wrote:
> 
>> If we're going to add code to loop over all globals anyway, then why
>> not, instead of warning about bad cases, and then wrapping tests in a
>> namespace, just have this code "fix" the leaked globals by deleting
>> them?
>>
>> My thinking is that any global that exists when we start a test should
>> continue to exist at the end of the test.  Any other global should
>> just be unset when the test script finishes.
>>
>> If there really is some global state that is lazily created by a
>> particular test script then (a) this seems like a bug anyway, and (b)
>> this is easy to fix by giving it an earlier creation / initialisation.
>>
>> In this way, folk can just write test scripts, dump their junk all
>> over the global namespace as they like, and we'll just clean up for
>> them.
>>
>> Thoughts?
> 
> Hmm, I thought of this when I saw Tom's detection patch the first time,
> but dismissed it, perhaps too quickly, thinking that it wouldn't
> be safe.  You mention global state lazily created by test scripts, but I was
> thinking of lazy initialized globals within Dejagnu itself, or whatever Tcl
> library we might end up using the future, which might need to be carried
> over from testcase to testcase.  Imagine, this in a Dejagnu function:
> 
> proc fail {} {
>    global number_of_fails
> 
>    # Hadn't been set before.  (valid since Tcl 8.5; otherwise, imagine it wrapped in 'info exits')
>    incr number_of_fails
> }
> 
> This is obviously not the real "fail", it is just to
> show the point, or the risk.  I imagined there might be similar
> functions in random Dejagnu boards.
> 

Agreed, that is a risk.

A less aggressive approach is also possible though: only prevent leaked
global arrays, and leave the rest of the global state unmodified.

> (
> Now that I look, it seems like Dejagnu wants to be sure to initialize
> all global variables it uses in runtest.exp:
> 
>  # Initialize a few global variables used by all tests.
>  # `reset_vars' resets several of these, we define them here to document their
>  # existence.  In fact, it would be nice if all globals used by some interface
>  # of dejagnu proper were documented here.
> 
> And I guess we can always initialize some ourselves, if we find that missing.
> )
>

Right, or, in terms of the latest proposed patch, call
gdb_persistent_global with the varname as argument.

> So if people think the benefits outweigh the risks, then I guess
> we should give it a go.
> 

I guess it's worth a try, it could be easily reverted if it cause
trouble.  If it causes trouble, it will take somebody's time, but OTOH
it will also take time to manually fix up test-cases to prevent the
originally proposed warning.

> BTW, global variables alone aren't the full scope of the
> bleeding between testcases.  There's also the case of
> testcases overriding procedures, like gdb.base/dbx.exp,
> but those are perhaps rare enough that we can continue
> handling it "manually" as before.

AFAICT, that test-case does an effort to undo the override, though I'm
not sure how certain it is that the undo will be executed.

Also, while working on the latest proposed patch, I also ran into
trouble related to the renaming of spawn in lib/tuiterm.exp. I wonder if
to address this we could wrap every tui test-case in
...
tui_env {
   ...
}
...
and letting the tui_env proc deal with renaming and restoring the spawn.
 We could even automate this in gdb_init/gdb_finish by calling
gdb_init_$subdir/gdb_finish_$subdir and handling it there.

Thanks,
- Tom