[RFC] Unset tcl variable addr to avoid clashing

Message ID 87r3rmjzi5.fsf@redhat.com
State New, archived
Headers

Commit Message

Sergio Durigan Junior April 14, 2015, 7:12 p.m. UTC
  On Monday, April 13 2015, Pedro Alves wrote:

>>> FWIW, there is not strong reason to keep the variable name as "addr" in
>>> the testcase.  So, an easier solution would be to rename the variable to
>>> something else, like "coredump_var_addr" (I think this name is unique
>>> enough).  The patch below does that.
>>>
>>> WDYT?
>>>
>>> --
>>> Sergio
>>> GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
>>> Please send encrypted e-mail if possible
>>> http://sergiodj.net/
>>>
>>> gdb/testsuite/ChangeLog:
>>> 2015-04-11  Sergio Durigan Junior  <sergiodj@redhat.com>
>>>
>>>         * gdb.base/coredump-filter.exp: Rename variable "addr" to
>>>         "coredump_var_addr" to avoid naming conflict with other
>>>         testcases.
>> 
>> Ok by me with one nit.
>> There's an issue here that still needs to be documented so it becomes
>> part of the collective lore.
>> Can this include a note about the need to give global array variables
>> unique names to either testsuite/README or
>> https://sourceware.org/gdb/wiki/Internals%20GDB-Testsuite-Coding-Standards
>
> I don't agree with this resolution.  Renaming is not sufficient.
>
> The same .exp file can be run in the same dejagnu invocation.  Most
> commonly, RUNTESTFLAGS="--target_board=unix\{-m64,-m32\}",
> but can also be mix of native and gdbserver, like
> RUNTESTFLAGS="--target_board='unix native-gdbserver'"
>
> So it's not just conflicting with other testcases that we need
> to worry about, but also a testcase conflicting with itself.  Even
> though in that "multiple boards" scenario the variable will be arrays in
> all invocations, we should still clear it to avoid stale
> contents, like e.g., here:
>
>   https://sourceware.org/ml/gdb-patches/2015-04/msg00261.html
>
> Therefore I think the solution must be that we unset/clear the
> variable.  And if we must do that, then the renaming to avoid
> naming conflicts is not a necessary condition.

Good point, thanks for mentioning this.

What do you think of the following patch (obvious, but I decided to send
anyway)?  This is just to make things 100% correct, and I don't think
it's worth reverting the renaming.

I will also expand/fix the update I did in the wiki page.

Thanks,
  

Comments

Sergio Durigan Junior April 26, 2015, 7:44 p.m. UTC | #1
On Tuesday, April 14 2015, I wrote:

> What do you think of the following patch (obvious, but I decided to send
> anyway)?  This is just to make things 100% correct, and I don't think
> it's worth reverting the renaming.

FWIW, I've checked the patch in.

> I will also expand/fix the update I did in the wiki page.

And I've also updated the wiki.

> Thanks,
>
> -- 
> Sergio
> GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
> Please send encrypted e-mail if possible
> http://sergiodj.net/
>
> gdb/testsuite/ChangeLog:
> 2015-04-14  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* gdb.base/coredump-filter.exp: Clear variable "coredump_var_addr"
> 	before using it.
>
> diff --git a/gdb/testsuite/gdb.base/coredump-filter.exp b/gdb/testsuite/gdb.base/coredump-filter.exp
> index f872de0..72f756a 100644
> --- a/gdb/testsuite/gdb.base/coredump-filter.exp
> +++ b/gdb/testsuite/gdb.base/coredump-filter.exp
> @@ -170,6 +170,7 @@ gdb_test_multiple "print/x &main" "getting main's address" {
>  
>  # Obtain the address of each variable that will be checked on each
>  # case.
> +set coredump_var_addr ""
>  foreach item $all_anon_corefiles {
>      foreach name [list [lindex $item 3] [lindex $item 4]] {
>  	set test "print/x $name"
  

Patch

diff --git a/gdb/testsuite/gdb.base/coredump-filter.exp b/gdb/testsuite/gdb.base/coredump-filter.exp
index f872de0..72f756a 100644
--- a/gdb/testsuite/gdb.base/coredump-filter.exp
+++ b/gdb/testsuite/gdb.base/coredump-filter.exp
@@ -170,6 +170,7 @@  gdb_test_multiple "print/x &main" "getting main's address" {
 
 # Obtain the address of each variable that will be checked on each
 # case.
+set coredump_var_addr ""
 foreach item $all_anon_corefiles {
     foreach name [list [lindex $item 3] [lindex $item 4]] {
 	set test "print/x $name"