Message ID | 20181204113345.717-1-andrew.burgess@embecosm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 79562 invoked by alias); 4 Dec 2018 11:33:58 -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 79533 invoked by uid 89); 4 Dec 2018 11:33:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=H*RU:209.85.128.68, Hx-spam-relays-external:209.85.128.68, sim X-HELO: mail-wm1-f68.google.com Received: from mail-wm1-f68.google.com (HELO mail-wm1-f68.google.com) (209.85.128.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Dec 2018 11:33:53 +0000 Received: by mail-wm1-f68.google.com with SMTP id a18so8985505wmj.1 for <gdb-patches@sourceware.org>; Tue, 04 Dec 2018 03:33:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id; bh=WS/y72tmBuyUkjGGpKzRjPfBrlnP0opHcCqeXUS3ntg=; b=Oh9iWHXHPqpeqqGST9igdmOfyeaTjlAgIspR3DeRtGM25pC2331JTTSaQi38QE46Jj JBSRdUZPrND3K3vizFfg62rOwaYNhhiT3+eELCP8+qpUr34x4QSZXSs3NZcn5I0gBl83 8nWtGjpXZ8DYbfVVkmtcWqcEQ6ONqix9/sU43utiaC88OxjzHlXbTa0n+beGL46b/klh MLFYtFgFo4aGj4KQ6hBUof15tHg32k+2SPuUfuHmfWBi9hPAuvDwjqu44wEPhbWPw9nV +xZVDgE/5Himmv1ryPfL54+hPhrzpQ6xFUeBnSzYSoWVxscAdMIrAYv7N7lFN48MAYkB AwKg== Return-Path: <andrew.burgess@embecosm.com> Received: from localhost (host86-156-236-210.range86-156.btcentralplus.com. [86.156.236.210]) by smtp.gmail.com with ESMTPSA id g201sm7532853wme.43.2018.12.04.03.33.50 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 04 Dec 2018 03:33:50 -0800 (PST) From: Andrew Burgess <andrew.burgess@embecosm.com> To: gdb-patches@sourceware.org Cc: Andrew Burgess <andrew.burgess@embecosm.com> Subject: [PATCH] gdb/testsuite/sim: Remove redundant setting of timeout Date: Tue, 4 Dec 2018 11:33:45 +0000 Message-Id: <20181204113345.717-1-andrew.burgess@embecosm.com> X-IsSubscribed: yes |
Commit Message
Andrew Burgess
Dec. 4, 2018, 11:33 a.m. UTC
In the config/sim.exp file two functions are defined. Both of these functions define local timeout variables and then call gdb_expect, which (through a call to get_largest_timeout) will find the local definition of timeout. However, both of these functions set the local timeout to some arbitrary value and print a log message for this "new" timeout just before returning. As in both cases, the timeout is a local variable, this final setting of the timeout has no effect and can be removed. gdb/testsuite/ChangeLog: * config/sim.exp (gdb_target_sim): Remove redundant adjustment of local timeout variable before return. (gdb_load): Likewise. --- gdb/testsuite/ChangeLog | 6 ++++++ gdb/testsuite/config/sim.exp | 4 ---- 2 files changed, 6 insertions(+), 4 deletions(-)
Comments
On 2018-12-04 06:33, Andrew Burgess wrote: > In the config/sim.exp file two functions are defined. Both of these > functions define local timeout variables and then call gdb_expect, > which (through a call to get_largest_timeout) will find the local > definition of timeout. > > However, both of these functions set the local timeout to some > arbitrary value and print a log message for this "new" timeout just > before returning. > > As in both cases, the timeout is a local variable, this final setting > of the timeout has no effect and can be removed. Hi Andrew, Can you verify whether the remaining "set timeout" in those functions have any effect at all? As you said, they are just local variables, so I don't expect them to influence the behavior of gdb_expect. Either we need "global timeout", or we pass the timeout directly as an argument to gdb_expect (the latter sounds better). Simon
On 12/04/2018 03:43 PM, Simon Marchi wrote: > On 2018-12-04 06:33, Andrew Burgess wrote: >> In the config/sim.exp file two functions are defined. Both of these >> functions define local timeout variables and then call gdb_expect, >> which (through a call to get_largest_timeout) will find the local >> definition of timeout. >> >> However, both of these functions set the local timeout to some >> arbitrary value and print a log message for this "new" timeout just >> before returning. >> >> As in both cases, the timeout is a local variable, this final setting >> of the timeout has no effect and can be removed. > > Hi Andrew, > > Can you verify whether the remaining "set timeout" in those functions have any effect at all? As you said, they are just local variables, so I don't expect them to influence the behavior of gdb_expect. Either we need "global timeout", or we pass the timeout directly as an argument to gdb_expect (the latter sounds better). Keep this in mind, from 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. Mimicking that behavior, gdb_test, gdb_test_multiple and gdb_expect pick the local timeout variable in the caller via upvar. E.g.: proc gdb_test { args } { global gdb_prompt upvar timeout timeout gdb_expect is a little more disguised, but it does the same, here, in the get_largest_timeout path: proc gdb_expect { args } { ... # A timeout argument takes precedence, otherwise of all the timeouts # select the largest. if [info exists atimeout] { set tmt $atimeout } else { set tmt [get_largest_timeout] } ... } and then get_largest_timeout does: proc get_largest_timeout {} { upvar #0 timeout gtimeout upvar 2 timeout timeout ^^^^^^^^^^^^^^^^^^^^^^^ ... Thanks, Pedro Alves
On 2018-12-04 10:54, Pedro Alves wrote: > On 12/04/2018 03:43 PM, Simon Marchi wrote: >> On 2018-12-04 06:33, Andrew Burgess wrote: >>> In the config/sim.exp file two functions are defined. Both of these >>> functions define local timeout variables and then call gdb_expect, >>> which (through a call to get_largest_timeout) will find the local >>> definition of timeout. >>> >>> However, both of these functions set the local timeout to some >>> arbitrary value and print a log message for this "new" timeout just >>> before returning. >>> >>> As in both cases, the timeout is a local variable, this final setting >>> of the timeout has no effect and can be removed. >> >> Hi Andrew, >> >> Can you verify whether the remaining "set timeout" in those functions >> have any effect at all? As you said, they are just local variables, >> so I don't expect them to influence the behavior of gdb_expect. >> Either we need "global timeout", or we pass the timeout directly as an >> argument to gdb_expect (the latter sounds better). > > Keep this in mind, from 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. > > > Mimicking that behavior, gdb_test, gdb_test_multiple and gdb_expect > pick the > local timeout variable in the caller via upvar. E.g.: > > proc gdb_test { args } { > global gdb_prompt > upvar timeout timeout > > gdb_expect is a little more disguised, but it does the same, here, > in the get_largest_timeout path: > > proc gdb_expect { args } { > ... > # A timeout argument takes precedence, otherwise of all the > timeouts > # select the largest. > if [info exists atimeout] { > set tmt $atimeout > } else { > set tmt [get_largest_timeout] > } > ... > } > > and then get_largest_timeout does: > > proc get_largest_timeout {} { > upvar #0 timeout gtimeout > upvar 2 timeout timeout > ^^^^^^^^^^^^^^^^^^^^^^^ > ... That's very confusing, to say the least. Simon
On 12/04/2018 04:08 PM, Simon Marchi wrote: > On 2018-12-04 10:54, Pedro Alves wrote: >> On 12/04/2018 03:43 PM, Simon Marchi wrote: >>> On 2018-12-04 06:33, Andrew Burgess wrote: >>>> In the config/sim.exp file two functions are defined. Both of these >>>> functions define local timeout variables and then call gdb_expect, >>>> which (through a call to get_largest_timeout) will find the local >>>> definition of timeout. >>>> >>>> However, both of these functions set the local timeout to some >>>> arbitrary value and print a log message for this "new" timeout just >>>> before returning. >>>> >>>> As in both cases, the timeout is a local variable, this final setting >>>> of the timeout has no effect and can be removed. >>> >>> Hi Andrew, >>> >>> Can you verify whether the remaining "set timeout" in those functions have any effect at all? As you said, they are just local variables, so I don't expect them to influence the behavior of gdb_expect. Either we need "global timeout", or we pass the timeout directly as an argument to gdb_expect (the latter sounds better). >> >> Keep this in mind, from 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. >> >> >> Mimicking that behavior, gdb_test, gdb_test_multiple and gdb_expect pick the >> local timeout variable in the caller via upvar. E.g.: >> >> proc gdb_test { args } { >> global gdb_prompt >> upvar timeout timeout >> >> gdb_expect is a little more disguised, but it does the same, here, >> in the get_largest_timeout path: >> >> proc gdb_expect { args } { >> ... >> # A timeout argument takes precedence, otherwise of all the timeouts >> # select the largest. >> if [info exists atimeout] { >> set tmt $atimeout >> } else { >> set tmt [get_largest_timeout] >> } >> ... >> } >> >> and then get_largest_timeout does: >> >> proc get_largest_timeout {} { >> upvar #0 timeout gtimeout >> upvar 2 timeout timeout >> ^^^^^^^^^^^^^^^^^^^^^^^ >> ... > > That's very confusing, to say the least. Don't shoot the messenger. :-) Thanks, Pedro Alves
On 2018-12-04 11:11, Pedro Alves wrote: > On 12/04/2018 04:08 PM, Simon Marchi wrote: >> That's very confusing, to say the least. > > Don't shoot the messenger. :-) Hehe, of course. In light of this information, I think Andrew's patch is fine. Do you? Simon
On 12/04/2018 04:15 PM, Simon Marchi wrote: > On 2018-12-04 11:11, Pedro Alves wrote: >> On 12/04/2018 04:08 PM, Simon Marchi wrote: >>> That's very confusing, to say the least. >> >> Don't shoot the messenger. :-) > > Hehe, of course. > > In light of this information, I think Andrew's patch is fine. Do you? Sort of. At least with the removing the tail "set timeout" part, I agree it's not doing anything. As for the verbose call, we print "Timeout is now ..." messages in a lot of places, and if you're looking at the log, I think seeing a "Timeout is now ..." indication without seeing it changed again reads like the timeout was never restored... That's a preexisting problem, of course, since currently we give the impression that we actually changed the timeout at the end of the function but we actually didn't... Still, IMHO, one of these would be a better change: a) - remove the initial verbose call too, or, b) - add "global timeout" at the start of the function, and restore the on-entry value on exit. That way both "Timeout is now ..." messages will be truthful. This is what e.g., testsuiteconfig/sid.exp does. In either case, there will be no imbalance in the verbose output. Thanks, Pedro Alves
diff --git a/gdb/testsuite/config/sim.exp b/gdb/testsuite/config/sim.exp index d9072febc6a..47146c6662e 100644 --- a/gdb/testsuite/config/sim.exp +++ b/gdb/testsuite/config/sim.exp @@ -37,8 +37,6 @@ proc gdb_target_sim { } { return -1 } } - set timeout 10 - verbose "Timeout is now $timeout seconds" 2 return 0 } @@ -67,8 +65,6 @@ proc gdb_load { arg } { if $verbose>1 then { send_user "Loaded $arg into $GDB\n" } - set timeout 30 - verbose "Timeout is now $timeout seconds" 2 return 0 } -re "$gdb_prompt $" {