Message ID | 87r3rqppgq.fsf@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 40442 invoked by alias); 11 Apr 2015 17:03:26 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 40431 invoked by uid 89); 11 Apr 2015 17:03:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Sat, 11 Apr 2015 17:03:22 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3BH3JjI030488 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Sat, 11 Apr 2015 13:03:19 -0400 Received: from localhost (unused-10-15-17-126.yyz.redhat.com [10.15.17.126]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3BH3I8f020204 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA256 bits=256 verify=NO); Sat, 11 Apr 2015 13:03:19 -0400 From: Sergio Durigan Junior <sergiodj@redhat.com> To: Yao Qi <qiyaoltc@gmail.com> Cc: gdb-patches@sourceware.org Subject: Re: [RFC] Unset tcl variable addr to avoid clashing References: <1428666671-12926-1-git-send-email-qiyaoltc@gmail.com> X-URL: http://blog.sergiodj.net Date: Sat, 11 Apr 2015 13:03:17 -0400 In-Reply-To: <1428666671-12926-1-git-send-email-qiyaoltc@gmail.com> (Yao Qi's message of "Fri, 10 Apr 2015 12:51:11 +0100") Message-ID: <87r3rqppgq.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
Commit Message
Sergio Durigan Junior
April 11, 2015, 5:03 p.m. UTC
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?
Comments
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) > } > } > }
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>
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
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) } } }