From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) by sourceware.org (Postfix) with ESMTPS id 223C2385DC00 for ; Tue, 2 Jun 2020 15:38:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 223C2385DC00 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x442.google.com with SMTP id x6so3827278wrm.13 for ; Tue, 02 Jun 2020 08:38:33 -0700 (PDT) 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:in-reply-to; bh=KE6mt3MnT3kxCmsamd1GvVCE9Lov2svDE7wW7o3LZfg=; b=ZJVxQPagLJtxvfoNbTzB7sehF7G5TVJ5ivYIIQP7WJ4jDsa0+l/Ay41CMcdtNlDsgJ dNvgEDijnFHrs5s7lxx7Mv8V8ceuEIHX/NFZwGIDTwo0m7XUJ2NePTqculp2St9ES36j uasbzMUp+SSYWx5MEn4gn0PyTQuNwsncENpdTz9GVV4zXFRavfke9xhd59hLANyunc9D 5S4IZhapzbQFTAvUhdtFSohBtkEUwQv73Y+y4sx98mVhCRU4ejlrR2ECpkQSiGn4uDlO /ZLrfmLC8RWFTa+rdgY8OnSFzrWfeYA8KziMRFJNIp7PWr2un0P5wxr6yDVChDg88kHa z40g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=KE6mt3MnT3kxCmsamd1GvVCE9Lov2svDE7wW7o3LZfg=; b=sKV/ClX/Iv3sApefNjZmL/U9KbkFHeoomU+zP1xC9K8KtRdvcN+qI4G0KHQD6bIZTI l0zJ/q+cQogfn1tOFlFMStd/AkJYT7sWCCUPfKktpdmAbK10Hu+ySo72FK9YSdSPtroI hsBjmZdeyqWDvZhphvKZK9pWB/Cd1d3oD68T5iogFTUn/7bZDMgilu8PsJaGXLXuz18s LB3mhJ+7fIE43vus5BKnUWhaZWI85GUph1vWxUYmvB/5aGH4EyDYpTjQe/11kc+r9C1B 6AVU/yMc2xIoH7wFdQJRVPUB1ZTulb5hZF9gXZn4fopJFgCXUmCrwJSAVfjcUiEgKRp3 /6wA== X-Gm-Message-State: AOAM533civNGPRJuJULZ7bAiKflkLOb3PIXBOwcPjH2KFYUoVaMQpHEt lIEgBrjeCI7gUqWALqtj8MiZKc985HE= X-Google-Smtp-Source: ABdhPJw2rfJ4H90ALW1iibQN8sTPdX22dg1RL6+KDoo/1yNOE3d1Oix8v6Rvi4Qw+YDbkqhES3+4jA== X-Received: by 2002:adf:dfcf:: with SMTP id q15mr26184609wrn.373.1591112312094; Tue, 02 Jun 2020 08:38:32 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id k17sm4466400wrl.54.2020.06.02.08.38.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Jun 2020 08:38:31 -0700 (PDT) Date: Tue, 2 Jun 2020 16:38:30 +0100 From: Andrew Burgess To: Tom de Vries Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/3][gdb/testsuite] Warn about leaked global array Message-ID: <20200602153830.GZ3522@embecosm.com> References: <20200519163004.GA9045@delia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200519163004.GA9045@delia> X-Operating-System: Linux/5.5.17-200.fc31.x86_64 (x86_64) X-Uptime: 16:34:06 up 43 days, 6:08, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Jun 2020 15:38:36 -0000 * Tom de Vries [2020-05-19 18:30:06 +0200]: > Hi, > > A variable name cannot be used both as scalar and array without an > intermediate unset. Trying to do so will result in tcl errors, for > example, for: > ... > set var "bla" > set var(1) "bla" > ... > we get: > ... > can't set "var(1)": variable isn't array > ... > and for the reverse statement order we get: > ... > can't set "var": variable is array > ... > > So, since a global name in one test-case can leak to another > test-case, setting a global name in one test-case can result in > a tcl error in another test-case that reuses the name in a different > way. > > Warn about leaking a global array from a test-case. > > Also, add a possibility to skip the warning in a given test-case using > variable gdb_skip_check_global_vars, and use it in gdb.mi/mi2-var-child.exp > and gdb.mi/mi-var-cp.exp. > > Tested on x86_64-linux. > > Any comments? If we're going to add code to loop over all globals anyway, then why not, instead of warning about bad cases, and then wrapping tests in a namespace, just have this code "fix" the leaked globals by deleting them? My thinking is that any global that exists when we start a test should continue to exist at the end of the test. Any other global should just be unset when the test script finishes. If there really is some global state that is lazily created by a particular test script then (a) this seems like a bug anyway, and (b) this is easy to fix by giving it an earlier creation / initialisation. In this way, folk can just write test scripts, dump their junk all over the global namespace as they like, and we'll just clean up for them. Thoughts? Thanks, Andrew > > Thanks, > - Tom > > [gdb/testsuite] Warn about leaked global array > > gdb/testsuite/ChangeLog: > > 2020-05-18 Tom de Vries > > PR testsuite/25996 > * lib/gdb.exp (global_array_exists, global_unset, save_global_vars) > (check_global_vars): New proc. > (gdb_init): Call save_global_vars. > (gdb_finish): Call check_global_vars. > * gdb.mi/mi-var-cp.exp: Set gdb_skip_check_global_vars. > * gdb.mi/mi2-var-child.exp: Same. > > --- > gdb/testsuite/gdb.mi/mi-var-cp.exp | 3 ++ > gdb/testsuite/gdb.mi/mi2-var-child.exp | 3 ++ > gdb/testsuite/lib/gdb.exp | 94 ++++++++++++++++++++++++++++++++++ > 3 files changed, 100 insertions(+) > > diff --git a/gdb/testsuite/gdb.mi/mi-var-cp.exp b/gdb/testsuite/gdb.mi/mi-var-cp.exp > index 8c6a624f80..dc32ddc346 100644 > --- a/gdb/testsuite/gdb.mi/mi-var-cp.exp > +++ b/gdb/testsuite/gdb.mi/mi-var-cp.exp > @@ -23,6 +23,9 @@ if [mi_gdb_start] { > continue > } > > +# FIXME. See check_global_vars in lib/gdb.exp. > +set gdb_skip_check_global_vars 1 > + > standard_testfile .cc > > if [get_compiler_info "c++"] { > diff --git a/gdb/testsuite/gdb.mi/mi2-var-child.exp b/gdb/testsuite/gdb.mi/mi2-var-child.exp > index e32858fbd3..b96ac41815 100644 > --- a/gdb/testsuite/gdb.mi/mi2-var-child.exp > +++ b/gdb/testsuite/gdb.mi/mi2-var-child.exp > @@ -27,6 +27,9 @@ if [mi_gdb_start] { > continue > } > > +# FIXME. See check_global_vars in lib/gdb.exp. > +set gdb_skip_check_global_vars 1 > + > standard_testfile var-cmd.c > > if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { > diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp > index f7d20bd94f..a0d558c943 100644 > --- a/gdb/testsuite/lib/gdb.exp > +++ b/gdb/testsuite/lib/gdb.exp > @@ -5048,6 +5048,98 @@ proc standard_testfile {args} { > } > } > > +# Returns 1 if GLOBALVARNAME is a global array. > + > +proc global_array_exists { globalvarname } { > + # Introduce local alias of global variable $globalvarname. Using > + # "global $globalvarname" instead is simpler, but this may result in a > + # clash with local name "globalvarname". > + upvar #0 $globalvarname globalvar > + return [array exists globalvar] > +} > + > +# Unset global variable GLOBALVARNAME. > + > +proc global_unset { globalvarname } { > + # Introduce local alias of global variable $globalvarname. > + upvar #0 $globalvarname globalvar > + unset globalvar > +} > + > +# Save global vars to variable gdb_global_vars. > + > +proc save_global_vars { test_file_name } { > + # Save for the warning. > + global gdb_test_file_name > + set gdb_test_file_name $test_file_name > + > + # Sample state before running test. > + global gdb_global_vars > + set gdb_global_vars [info globals] > + > + global gdb_skip_check_global_vars > + if { [info exists gdb_skip_check_global_vars]} { > + unset gdb_skip_check_global_vars > + } > +} > + > +# Check global variables not in gdb_global_vars. > + > +proc check_global_vars { } { > + global gdb_skip_check_global_vars > + if { [info exists gdb_skip_check_global_vars]} { > + return > + } > + # Sample state after running test. > + global gdb_global_vars > + set vars [info globals] > + > + set skip [list "expect_out" "spawn_out" "auto_index"] > + > + foreach var $vars { > + if { ![global_array_exists $var] } { > + continue > + } > + > + set found [lsearch -exact $gdb_global_vars $var] > + if { $found != -1 } { > + # Already present before running test. > + continue > + } > + > + set found [lsearch -exact $skip $var] > + if { $found != -1 } { > + continue > + } > + > + # A variable name cannot be used both as scalar and array without an > + # intermediate unset. Trying to do so will result in tcl errors, for > + # example, for: > + # set var "bla" > + # set var(1) "bla" > + # we get: > + # can't set "var(1)": variable isn't array > + # and for the reverse statement order we get: > + # can't set "var": variable is array > + # > + # So, since a global name in one test-case can leak to another > + # test-case, setting a global name in one test-case can result in > + # a tcl error in another test-case that reuses the name in a different > + # way. > + # > + # Warn about leaking a global array from the test-case. > + # > + # For a description on how to fix this, see "Wrapping test-cases in > + # Tcl namespaces" in gdb/testsuite/README. > + global gdb_test_file_name > + warning "$gdb_test_file_name.exp defined global array $var" > + > + # If the variable remains set, we won't warn for the next test where > + # it's leaked, so unset. > + global_unset $var > + } > +} > + > # The default timeout used when testing GDB commands. We want to use > # the same timeout as the default dejagnu timeout, unless the user has > # already provided a specific value (probably through a site.exp file). > @@ -5177,10 +5269,12 @@ proc gdb_init { test_file_name } { > global gdb_instances > set gdb_instances 0 > > + save_global_vars $test_file_name > return [default_gdb_init $test_file_name] > } > > proc gdb_finish { } { > + check_global_vars > global gdbserver_reconnect_p > global gdb_prompt > global cleanfiles