public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [RFC] Unset tcl variable addr to avoid clashing
@ 2015-04-10 11:51 Yao Qi
  2015-04-10 16:53 ` Doug Evans
  2015-04-11 17:03 ` Sergio Durigan Junior
  0 siblings, 2 replies; 10+ messages in thread
From: Yao Qi @ 2015-04-10 11:51 UTC (permalink / raw)
  To: gdb-patches

From: Yao Qi <yao.qi@linaro.org>

Hi,
I see some tcl ERRORs in gdb.sum recently:

ERROR: tcl error sourcing ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp.
ERROR: can't set "addr": variable is array
    while executing
"set addr "0x\[0-9a-zA-Z\]+""
    (file "../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp" line 45)
    invoked from within
"source ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 source ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp"
    invoked from within
"catch "uplevel #0 source $test_file_name""

It is OK to run single dmsym.exp.  This error is caused by the name
clashing with coredump-filter.exp, and it can be reproduced,

 $ make check RUNTESTFLAGS='coredump-filter.exp dmsym.exp exception.exp stepi-random-signal.exp'

as variable addr is used in all of them.  This patch is to unset array
addr, but manually unset variables isn't good to me.  Is there any
approaches we can do to avoid name clashing?

gdb/testsuite:

2015-04-10  Yao Qi  <yao.qi@linaro.org>

	* gdb.base/coredump-filter.exp: Unset addr.
---
 gdb/testsuite/gdb.base/coredump-filter.exp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/testsuite/gdb.base/coredump-filter.exp b/gdb/testsuite/gdb.base/coredump-filter.exp
index f3203be..2deb7b3 100644
--- a/gdb/testsuite/gdb.base/coredump-filter.exp
+++ b/gdb/testsuite/gdb.base/coredump-filter.exp
@@ -196,3 +196,5 @@ foreach item $all_anon_corefiles {
 with_test_prefix "loading and testing corefile for non-Private-Shared-Anon-File" {
     test_disasm $non_private_shared_anon_file_core $main_addr 1
 }
+
+unset addr
-- 
1.9.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] Unset tcl variable addr to avoid clashing
  2015-04-10 11:51 [RFC] Unset tcl variable addr to avoid clashing Yao Qi
@ 2015-04-10 16:53 ` Doug Evans
  2015-04-10 17:55   ` Keith Seitz
  2015-04-11 17:03 ` Sergio Durigan Junior
  1 sibling, 1 reply; 10+ messages in thread
From: Doug Evans @ 2015-04-10 16:53 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

On Fri, Apr 10, 2015 at 4:51 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> From: Yao Qi <yao.qi@linaro.org>
>
> Hi,
> I see some tcl ERRORs in gdb.sum recently:
>
> ERROR: tcl error sourcing ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp.
> ERROR: can't set "addr": variable is array
>     while executing
> "set addr "0x\[0-9a-zA-Z\]+""
>     (file "../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp" line 45)
>     invoked from within
> "source ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp"
>     ("uplevel" body line 1)
>     invoked from within
> "uplevel #0 source ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp"
>     invoked from within
> "catch "uplevel #0 source $test_file_name""
>
> It is OK to run single dmsym.exp.  This error is caused by the name
> clashing with coredump-filter.exp, and it can be reproduced,
>
>  $ make check RUNTESTFLAGS='coredump-filter.exp dmsym.exp exception.exp stepi-random-signal.exp'
>
> as variable addr is used in all of them.  This patch is to unset array
> addr, but manually unset variables isn't good to me.  Is there any
> approaches we can do to avoid name clashing?

Bleah. :-)
The first thing that comes to mind is of course a convention that
array globals must be prefixed with the name of the test.
OTOH gdb_base_coredump_filter_addr is painful.

>
> gdb/testsuite:
>
> 2015-04-10  Yao Qi  <yao.qi@linaro.org>
>
>         * gdb.base/coredump-filter.exp: Unset addr.
> ---
>  gdb/testsuite/gdb.base/coredump-filter.exp | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/gdb/testsuite/gdb.base/coredump-filter.exp b/gdb/testsuite/gdb.base/coredump-filter.exp
> index f3203be..2deb7b3 100644
> --- a/gdb/testsuite/gdb.base/coredump-filter.exp
> +++ b/gdb/testsuite/gdb.base/coredump-filter.exp
> @@ -196,3 +196,5 @@ foreach item $all_anon_corefiles {
>  with_test_prefix "loading and testing corefile for non-Private-Shared-Anon-File" {
>      test_disasm $non_private_shared_anon_file_core $main_addr 1
>  }
> +
> +unset addr

I'd prefer a comment explaining why that is there appear in the code
rather than having to go find it in commit logs or emails.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] Unset tcl variable addr to avoid clashing
  2015-04-10 16:53 ` Doug Evans
@ 2015-04-10 17:55   ` Keith Seitz
  2015-04-10 18:08     ` Doug Evans
  0 siblings, 1 reply; 10+ messages in thread
From: Keith Seitz @ 2015-04-10 17:55 UTC (permalink / raw)
  To: Doug Evans, Yao Qi; +Cc: gdb-patches

On 04/10/2015 09:53 AM, Doug Evans wrote:
> Bleah. :-)
> The first thing that comes to mind is of course a convention that
> array globals must be prefixed with the name of the test.
> OTOH gdb_base_coredump_filter_addr is painful.

I've had to do this in several places, too, with my big locations patchset.

In the end, I felt the path-of-least-resistance was to encapsulate the 
whole test in its own namespace and then declare variables:

namespace eval $testfile {
     variable addr
     variable linespec
     variable location

     # do all test stuff
}

namespace delete $testfile

This is rather inconvenient, so I played for a short while with trying 
to automate this in some way. The only solution that I could devise that 
didn't involve modifying dejagnu was to track the global variable list 
in standard_testfile or prepare_for_testing, unset'ing "new" globals 
every time the procedure was called.

It proved a bit problematic, and I ended up with the namespace approach. 
It was far less risky/invasive. If there is any interest, I could dig 
that up and play with it some more.

Keith

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] Unset tcl variable addr to avoid clashing
  2015-04-10 17:55   ` Keith Seitz
@ 2015-04-10 18:08     ` Doug Evans
  0 siblings, 0 replies; 10+ messages in thread
From: Doug Evans @ 2015-04-10 18:08 UTC (permalink / raw)
  To: Keith Seitz; +Cc: Yao Qi, gdb-patches

On Fri, Apr 10, 2015 at 10:55 AM, Keith Seitz <keiths@redhat.com> wrote:
> On 04/10/2015 09:53 AM, Doug Evans wrote:
>>
>> Bleah. :-)
>> The first thing that comes to mind is of course a convention that
>> array globals must be prefixed with the name of the test.
>> OTOH gdb_base_coredump_filter_addr is painful.
>
>
> I've had to do this in several places, too, with my big locations patchset.
>
> In the end, I felt the path-of-least-resistance was to encapsulate the whole
> test in its own namespace and then declare variables:
>
> namespace eval $testfile {
>     variable addr
>     variable linespec
>     variable location
>
>     # do all test stuff
> }
>
> namespace delete $testfile
>
> This is rather inconvenient, so I played for a short while with trying to
> automate this in some way. The only solution that I could devise that didn't
> involve modifying dejagnu was to track the global variable list in
> standard_testfile or prepare_for_testing, unset'ing "new" globals every time
> the procedure was called.

Tucking the thing away in a namespace works too.

[for a definition of "works" that recognizes we have to be pragmatic here :-)]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] Unset tcl variable addr to avoid clashing
  2015-04-10 11:51 [RFC] Unset tcl variable addr to avoid clashing Yao Qi
  2015-04-10 16:53 ` Doug Evans
@ 2015-04-11 17:03 ` Sergio Durigan Junior
  2015-04-12 17:22   ` Doug Evans
  1 sibling, 1 reply; 10+ messages in thread
From: Sergio Durigan Junior @ 2015-04-11 17:03 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

On Friday, April 10 2015, Yao Qi wrote:

> From: Yao Qi <yao.qi@linaro.org>
>
> Hi,
> I see some tcl ERRORs in gdb.sum recently:
>
> ERROR: tcl error sourcing ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp.
> ERROR: can't set "addr": variable is array
>     while executing
> "set addr "0x\[0-9a-zA-Z\]+""
>     (file "../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp" line 45)
>     invoked from within
> "source ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp"
>     ("uplevel" body line 1)
>     invoked from within
> "uplevel #0 source ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp"
>     invoked from within
> "catch "uplevel #0 source $test_file_name""
>
> It is OK to run single dmsym.exp.  This error is caused by the name
> clashing with coredump-filter.exp, and it can be reproduced,

It seems that coredump-filter.exp is causing lots of headaches
lately...  Sorry about that.

>  $ make check RUNTESTFLAGS='coredump-filter.exp dmsym.exp exception.exp stepi-random-signal.exp'
>
> as variable addr is used in all of them.  This patch is to unset array
> addr, but manually unset variables isn't good to me.  Is there any
> approaches we can do to avoid name clashing?

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.

diff --git a/gdb/testsuite/gdb.base/coredump-filter.exp b/gdb/testsuite/gdb.base/coredump-filter.exp
index f3203be..8c94e94 100644
--- a/gdb/testsuite/gdb.base/coredump-filter.exp
+++ b/gdb/testsuite/gdb.base/coredump-filter.exp
@@ -38,7 +38,7 @@ proc do_save_core { filter_flag core ipid } {
 }
 
 proc do_load_and_test_core { core var working_var working_value } {
-    global hex decimal addr
+    global hex decimal coredump_var_addr
 
     set core_loaded [gdb_core_cmd "$core" "load core"]
     if { $core_loaded == -1 } {
@@ -47,9 +47,9 @@ proc do_load_and_test_core { core var working_var working_value } {
     }
 
     # Access the memory the addresses point to.
-    gdb_test "print/x *(char *) $addr($var)" "\(\\\$$decimal = <error: \)?Cannot access memory at address $hex\(>\)?" \
+    gdb_test "print/x *(char *) $coredump_var_addr($var)" "\(\\\$$decimal = <error: \)?Cannot access memory at address $hex\(>\)?" \
 	"printing $var when core is loaded (should not work)"
-    gdb_test "print/x *(char *) $addr($working_var)" " = $working_value.*" \
+    gdb_test "print/x *(char *) $coredump_var_addr($working_var)" " = $working_value.*" \
 	"print/x *$working_var ( = $working_value)"
 }
 
@@ -163,7 +163,7 @@ foreach item $all_anon_corefiles {
 	set test "print/x $name"
 	gdb_test_multiple $test $test {
 	    -re " = \($hex\)\r\n$gdb_prompt $" {
-		set addr($name) $expect_out(1,string)
+		set coredump_var_addr($name) $expect_out(1,string)
 	    }
 	}
     }

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] Unset tcl variable addr to avoid clashing
  2015-04-11 17:03 ` Sergio Durigan Junior
@ 2015-04-12 17:22   ` Doug Evans
  2015-04-13  6:47     ` Sergio Durigan Junior
  2015-04-13  8:26     ` Pedro Alves
  0 siblings, 2 replies; 10+ messages in thread
From: Doug Evans @ 2015-04-12 17:22 UTC (permalink / raw)
  To: Sergio Durigan Junior; +Cc: Yao Qi, gdb-patches

On Sat, Apr 11, 2015 at 10:03 AM, Sergio Durigan Junior
<sergiodj@redhat.com> wrote:
> On Friday, April 10 2015, Yao Qi wrote:
>
>> From: Yao Qi <yao.qi@linaro.org>
>>
>> Hi,
>> I see some tcl ERRORs in gdb.sum recently:
>>
>> ERROR: tcl error sourcing ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp.
>> ERROR: can't set "addr": variable is array
>>     while executing
>> "set addr "0x\[0-9a-zA-Z\]+""
>>     (file "../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp" line 45)
>>     invoked from within
>> "source ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp"
>>     ("uplevel" body line 1)
>>     invoked from within
>> "uplevel #0 source ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp"
>>     invoked from within
>> "catch "uplevel #0 source $test_file_name""
>>
>> It is OK to run single dmsym.exp.  This error is caused by the name
>> clashing with coredump-filter.exp, and it can be reproduced,
>
> It seems that coredump-filter.exp is causing lots of headaches
> lately...  Sorry about that.
>
>>  $ make check RUNTESTFLAGS='coredump-filter.exp dmsym.exp exception.exp stepi-random-signal.exp'
>>
>> as variable addr is used in all of them.  This patch is to unset array
>> addr, but manually unset variables isn't good to me.  Is there any
>> approaches we can do to avoid name clashing?
>
> 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

>
> diff --git a/gdb/testsuite/gdb.base/coredump-filter.exp b/gdb/testsuite/gdb.base/coredump-filter.exp
> index f3203be..8c94e94 100644
> --- a/gdb/testsuite/gdb.base/coredump-filter.exp
> +++ b/gdb/testsuite/gdb.base/coredump-filter.exp
> @@ -38,7 +38,7 @@ proc do_save_core { filter_flag core ipid } {
>  }
>
>  proc do_load_and_test_core { core var working_var working_value } {
> -    global hex decimal addr
> +    global hex decimal coredump_var_addr
>
>      set core_loaded [gdb_core_cmd "$core" "load core"]
>      if { $core_loaded == -1 } {
> @@ -47,9 +47,9 @@ proc do_load_and_test_core { core var working_var working_value } {
>      }
>
>      # Access the memory the addresses point to.
> -    gdb_test "print/x *(char *) $addr($var)" "\(\\\$$decimal = <error: \)?Cannot access memory at address $hex\(>\)?" \
> +    gdb_test "print/x *(char *) $coredump_var_addr($var)" "\(\\\$$decimal = <error: \)?Cannot access memory at address $hex\(>\)?" \
>         "printing $var when core is loaded (should not work)"
> -    gdb_test "print/x *(char *) $addr($working_var)" " = $working_value.*" \
> +    gdb_test "print/x *(char *) $coredump_var_addr($working_var)" " = $working_value.*" \
>         "print/x *$working_var ( = $working_value)"
>  }
>
> @@ -163,7 +163,7 @@ foreach item $all_anon_corefiles {
>         set test "print/x $name"
>         gdb_test_multiple $test $test {
>             -re " = \($hex\)\r\n$gdb_prompt $" {
> -               set addr($name) $expect_out(1,string)
> +               set coredump_var_addr($name) $expect_out(1,string)
>             }
>         }
>      }

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] Unset tcl variable addr to avoid clashing
  2015-04-12 17:22   ` Doug Evans
@ 2015-04-13  6:47     ` Sergio Durigan Junior
  2015-04-13  8:26     ` Pedro Alves
  1 sibling, 0 replies; 10+ messages in thread
From: Sergio Durigan Junior @ 2015-04-13  6:47 UTC (permalink / raw)
  To: Doug Evans; +Cc: Yao Qi, gdb-patches

On Sunday, April 12 2015, Doug Evans 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

Thanks, pushed:

  <https://sourceware.org/ml/gdb-cvs/2015-04/msg00095.html>
  8cd8f2f8ac49276437b7da37f275706ea1c1c925

And updated the internals manual:

  <https://sourceware.org/gdb/wiki/Internals%20GDB-Testsuite-Coding-Standards?action=recall&rev=3>

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] Unset tcl variable addr to avoid clashing
  2015-04-12 17:22   ` Doug Evans
  2015-04-13  6:47     ` Sergio Durigan Junior
@ 2015-04-13  8:26     ` Pedro Alves
  2015-04-14 19:12       ` Sergio Durigan Junior
  1 sibling, 1 reply; 10+ messages in thread
From: Pedro Alves @ 2015-04-13  8:26 UTC (permalink / raw)
  To: Doug Evans, Sergio Durigan Junior; +Cc: Yao Qi, gdb-patches

On 04/12/2015 06:22 PM, Doug Evans wrote:
> On Sat, Apr 11, 2015 at 10:03 AM, Sergio Durigan Junior
> <sergiodj@redhat.com> wrote:
>> On Friday, April 10 2015, Yao Qi wrote:
>>
>>> From: Yao Qi <yao.qi@linaro.org>
>>>
>>> Hi,
>>> I see some tcl ERRORs in gdb.sum recently:
>>>
>>> ERROR: tcl error sourcing ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp.
>>> ERROR: can't set "addr": variable is array
>>>     while executing
>>> "set addr "0x\[0-9a-zA-Z\]+""
>>>     (file "../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp" line 45)
>>>     invoked from within
>>> "source ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp"
>>>     ("uplevel" body line 1)
>>>     invoked from within
>>> "uplevel #0 source ../../../../../binutils-gdb/gdb/testsuite/gdb.base/dmsym.exp"
>>>     invoked from within
>>> "catch "uplevel #0 source $test_file_name""
>>>
>>> It is OK to run single dmsym.exp.  This error is caused by the name
>>> clashing with coredump-filter.exp, and it can be reproduced,
>>
>> It seems that coredump-filter.exp is causing lots of headaches
>> lately...  Sorry about that.
>>
>>>  $ make check RUNTESTFLAGS='coredump-filter.exp dmsym.exp exception.exp stepi-random-signal.exp'
>>>
>>> as variable addr is used in all of them.  This patch is to unset array
>>> addr, but manually unset variables isn't good to me.  Is there any
>>> approaches we can do to avoid name clashing?
>>
>> 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.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] Unset tcl variable addr to avoid clashing
  2015-04-13  8:26     ` Pedro Alves
@ 2015-04-14 19:12       ` Sergio Durigan Junior
  2015-04-26 19:44         ` Sergio Durigan Junior
  0 siblings, 1 reply; 10+ messages in thread
From: Sergio Durigan Junior @ 2015-04-14 19:12 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Doug Evans, Yao Qi, gdb-patches

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,

-- 
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"

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC] Unset tcl variable addr to avoid clashing
  2015-04-14 19:12       ` Sergio Durigan Junior
@ 2015-04-26 19:44         ` Sergio Durigan Junior
  0 siblings, 0 replies; 10+ messages in thread
From: Sergio Durigan Junior @ 2015-04-26 19:44 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Doug Evans, Yao Qi, gdb-patches

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"

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-04-26 19:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-10 11:51 [RFC] Unset tcl variable addr to avoid clashing Yao Qi
2015-04-10 16:53 ` Doug Evans
2015-04-10 17:55   ` Keith Seitz
2015-04-10 18:08     ` Doug Evans
2015-04-11 17:03 ` Sergio Durigan Junior
2015-04-12 17:22   ` Doug Evans
2015-04-13  6:47     ` Sergio Durigan Junior
2015-04-13  8:26     ` Pedro Alves
2015-04-14 19:12       ` Sergio Durigan Junior
2015-04-26 19:44         ` Sergio Durigan Junior

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).