From patchwork Tue Dec 4 21:34:20 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 30542 Received: (qmail 53532 invoked by alias); 4 Dec 2018 21:34:29 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 53405 invoked by uid 89); 4 Dec 2018 21:34:28 -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=2400, backing, folded X-HELO: mail-wm1-f67.google.com Received: from mail-wm1-f67.google.com (HELO mail-wm1-f67.google.com) (209.85.128.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Dec 2018 21:34:25 +0000 Received: by mail-wm1-f67.google.com with SMTP id q26so10796281wmf.5 for ; Tue, 04 Dec 2018 13:34:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=DVobNSENlw2Efg/NOTTbn2uyk4lxDPOQmHUzsmdPIBw=; b=Ed4/lQDi2gmWpue2RU05Ym9AE1nbo+ccKjC/GA/JVsmYDsf2RphUYSjfQvm6RN9ubr DF1oRItf2ZYHoxFM4vEm44088of9P5wEoewbvbVXzjcgZTPq1zmovMJ88W38GFMR3/s9 e86bmXpALwp72upWtJ8lfhyxIALUfBsJBdh2gzeNuvZE47L6I7+U52ZCUEClCeVFouFF 7uvO5Td4IVxeWl1zWublXnNAOiMFMMmRO1hvotm8LJNhEOBTrB1pU/prwtJYnnlPHB61 O5ZdM3PpFadAZOED9xPALH1wEz3ItKera/bgKHp+tSixKFnww3cYk9o1FvOh/Weq1sXr EfAA== Return-Path: Received: from localhost (host86-156-236-210.range86-156.btcentralplus.com. [86.156.236.210]) by smtp.gmail.com with ESMTPSA id o9sm9832960wmh.3.2018.12.04.13.34.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 04 Dec 2018 13:34:21 -0800 (PST) Date: Tue, 4 Dec 2018 21:34:20 +0000 From: Andrew Burgess To: Pedro Alves Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb/testsuite/sim: Remove redundant setting of timeout Message-ID: <20181204213420.GQ18841@embecosm.com> References: <20181204113345.717-1-andrew.burgess@embecosm.com> <07a27e33-ca57-f4be-a055-8d4070b03554@redhat.com> <37e9f834bac94720a257148d45bd0325@polymtl.ca> <3c7ef445a7a59fa94f8eaee578d24177@polymtl.ca> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Fortune: Vanilla wafer. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes * Pedro Alves [2018-12-04 16:33:20 +0000]: > 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 both for the feedback. In the end I went for (a) - making timeout global, backing it up, etc just to print a log message seemed like overkill, especially when we adjust the timeout in lots of other places without any logging at all. With the logging gone, folding the timeout into the gdb_expect call seemed like an obvious cleanup. The new patch is below. Thanks, Andrew --- gdb/testsuite/sim: Remove redundant setting of timeout 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. As having log messages about the timeout being adjusted could cause confusion I've removed all logging related to timeouts in this function, timeouts are adjusted thoughout the testsuite without any logging, there doesn't seem to be any good reason why these functions should get their own logging. With the logging gone there seems to be little need to a local timeout variable at all, and so I've folded the local timeout directly into the call to gdb_expect. gdb/testsuite/ChangeLog: * config/sim.exp (gdb_target_sim): Remove redundant adjustment of local timeout variable before return, and remove all local timeout variable entirely. (gdb_load): Likewise. --- gdb/testsuite/ChangeLog | 7 +++++++ gdb/testsuite/config/sim.exp | 12 ++---------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/gdb/testsuite/config/sim.exp b/gdb/testsuite/config/sim.exp index d9072febc6a..fd4d506ebb8 100644 --- a/gdb/testsuite/config/sim.exp +++ b/gdb/testsuite/config/sim.exp @@ -26,9 +26,7 @@ proc gdb_target_sim { } { set target_sim_options "[board_info target gdb,target_sim_options]" send_gdb "target sim $target_sim_options\n" - set timeout 60 - verbose "Timeout is now $timeout seconds" 2 - gdb_expect { + gdb_expect 60 { -re "Connected to the simulator.*$gdb_prompt $" { verbose "Set target to sim" } @@ -37,8 +35,6 @@ proc gdb_target_sim { } { return -1 } } - set timeout 10 - verbose "Timeout is now $timeout seconds" 2 return 0 } @@ -60,15 +56,11 @@ proc gdb_load { arg } { if [gdb_target_sim] then { return -1 } send_gdb "load\n" - set timeout 2400 - verbose "Timeout is now $timeout seconds" 2 - gdb_expect { + gdb_expect 2400 { -re ".*$gdb_prompt $" { 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 $" {