From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 107522 invoked by alias); 7 Feb 2020 11:47:19 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 107514 invoked by uid 89); 7 Feb 2020 11:47:19 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-wr1-f43.google.com Received: from mail-wr1-f43.google.com (HELO mail-wr1-f43.google.com) (209.85.221.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Feb 2020 11:47:16 +0000 Received: by mail-wr1-f43.google.com with SMTP id u6so2347984wrt.0 for ; Fri, 07 Feb 2020 03:47:16 -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:in-reply-to:user-agent; bh=RjoxVtjJkoYcjkEQ3SHQHqrLn64t0EfWjUNEoS7Jh8I=; b=OBAZ9x8TVPzwHTWRCOBOPP6c5MbaOSxpyelS7OMHiUV/EAOH2bbXpyVios8qTTiddO vklpD+hGl7SkiuevQqH+TZbBZmhNATmcaPlNAgrM64PfcFbLCfJmDY9vNT0RdQpWyiVZ YbG8RArQQZ3WXe69wtQeymhvIwUModlZs2yNnNYM7NXpfESncqBLLJQEl9CxBQNva8Rv dsiw9B+GT3ngFy168LE4Z8OAQTLkzBJc4fbCJPtaC0a6UK9gV4uzpMHyPdUGwjIxpGkv a0q3YjRgqUj3ZR2yt1IX5SljyWlJ2t17yN7IQGKxaQlZ7kJZQUkOk7XY+HgTJuZKd7E2 nqtw== Return-Path: Received: from localhost (host86-191-239-73.range86-191.btcentralplus.com. [86.191.239.73]) by smtp.gmail.com with ESMTPSA id l15sm3044675wrv.39.2020.02.07.03.47.13 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 07 Feb 2020 03:47:13 -0800 (PST) Date: Fri, 07 Feb 2020 11:47:00 -0000 From: Andrew Burgess To: Sergio Durigan Junior Cc: Luis Machado , GDB Patches Subject: Re: [PATCH] New testcase for PR tui/25126 (staled source cache) Message-ID: <20200207114712.GI4020@embecosm.com> References: <20200206225943.26709-1-sergiodj@redhat.com> <4a3db909-8677-fb84-39cf-84495266fdd8@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4a3db909-8677-fb84-39cf-84495266fdd8@linaro.org> X-Fortune: One planet is all you get. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg00150.txt.bz2 * Luis Machado [2020-02-07 06:41:12 -0300]: > On 2/6/20 7:59 PM, Sergio Durigan Junior wrote: > > I'm dealing with a Fedora GDB bug that is related to PR tui/25126, and > > I thought I'd write a specific testcase for it because I couldn't find > > one. > > > > The idea is to get the simple reproducer from the bug and tweak the > > testcase around it. This one was a bit hard because, since we need to > > modify the source file and recompile it, it involved a bit of TCL-foo > > to do things. Also for this reason, I'm only enabling the test for > > native boards. > > > > I tested this with an upstream GDB and made sure everything is > > passing. I also tested with a faulty GDB and made sure the test > > failed. > > > > gdb/testsuite/ChangeLog: > > 2020-02-07 Sergio Durigan Junior > > > > PR tui/25126 > > * gdb.base/cached-source-file.c: New file. > > * gdb.base/cached-source-file.exp: New file. > > > > Change-Id: Ib1b074342ebe8613c6d1dfde631691ebdf6d81c6 > > --- > > gdb/testsuite/gdb.base/cached-source-file.c | 37 ++++++++ > > gdb/testsuite/gdb.base/cached-source-file.exp | 94 +++++++++++++++++++ > > 2 files changed, 131 insertions(+) > > create mode 100644 gdb/testsuite/gdb.base/cached-source-file.c > > create mode 100644 gdb/testsuite/gdb.base/cached-source-file.exp > > > > diff --git a/gdb/testsuite/gdb.base/cached-source-file.c b/gdb/testsuite/gdb.base/cached-source-file.c > > new file mode 100644 > > index 0000000000..9f698dcffe > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/cached-source-file.c > > @@ -0,0 +1,37 @@ > > +/* This testcase is part of GDB, the GNU debugger. > > + > > + Copyright 2020 Free Software Foundation, Inc. > > + > > + This program is free software; you can redistribute it and/or modify > > + it under the terms of the GNU General Public License as published by > > + the Free Software Foundation; either version 3 of the License, or > > + (at your option) any later version. > > + > > + This program is distributed in the hope that it will be useful, > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + GNU General Public License for more details. > > + > > + You should have received a copy of the GNU General Public License > > + along with this program. If not, see . */ > > + > > Instead of pointing at a PR number, wouldn't it be better to state what this > particular source file plans to achieve or help achieve? > > Personally, i find it a bit more useful. > > Then again, if it is a simple test and it will be obvious from code > comments, it shouldn't be necessary. > > > +/* Test for PR tui/25126. > > + > > + The .exp testcase depends on the line numbers and contents from > > + this file If you change this file, make sure to double-check the > > + testcase. */ > > + > > +#include > > + > > +void > > +foo (void) > > +{ > > + printf ("hello\n"); /* break-here */ > > +} > > + > > +int > > +main () > > +{ > > + foo (); > > + return 0; > > +} > > diff --git a/gdb/testsuite/gdb.base/cached-source-file.exp b/gdb/testsuite/gdb.base/cached-source-file.exp > > new file mode 100644 > > index 0000000000..f98bec09ca > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/cached-source-file.exp > > @@ -0,0 +1,94 @@ > > +# Copyright (C) 2020 Free Software Foundation, Inc. > > + > > +# This program is free software; you can redistribute it and/or modify > > +# it under the terms of the GNU General Public License as published by > > +# the Free Software Foundation; either version 3 of the License, or > > +# (at your option) any later version. > > +# > > +# This program is distributed in the hope that it will be useful, > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +# GNU General Public License for more details. > > +# > > +# You should have received a copy of the GNU General Public License > > +# along with this program. If not, see . > > + > > Same as above about explaining what the test wants to achieve. > > > +# Test for PR tui/25126. > > Would it make the test run fine with remote stubs if the TUI wasn't used? > Just wondering. > > > +# This bug is reproducible even without using the TUI. > > + > > +standard_testfile > > + > > +# Only run on native boards. > > +if { [use_gdb_stub] || [target_info gdb_protocol] == "extended-remote" } { > > + return -1 > > +} > > + > > +# Because we need to modify the source file later, it's better if we > > Typo... "just copy it to our..." > > > +# just copy if to our output directory (instead of messing with the > > +# user's source directory). > > +set newsrc [standard_output_file $testfile].c > > +file copy -force -- $srcdir/$subdir/$srcfile $newsrc > > +set srcfile $newsrc > > + > > +if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } { > > + return -1 > > +} > > + > > +# Get the line number for the line with the "break-here" marker. > > +set bp_line [gdb_get_line_number "break-here" $srcfile] > > + > > I've learned about it recently, but i think using gdb_assert here would be > nice instead of the if/else block. > > > +if { [runto "$srcfile:$bp_line"] } { > > + pass "run to $srcfile:$bp_line" > > +} else { > > + fail "run to $srcfile:$bp_line" > > +} > > + > > +# Do a "list" and check that the printed line matches the line of the > > +# original source file. > > +gdb_test_no_output "set listsize 1" > > +gdb_test "list" "$bp_line\[ \t\]+printf \\(\"hello\\\\n\"\\); /\\* break-here \\*/" \ > > + "check the first version of the source file" > > + > > +# Modify the original source file, and add an extra line into it. > > +# This only works locally because of the TCL commands. > > +set bkpsrc [standard_output_file $testfile].c.bkp > > +set bkpsrcfd [open $bkpsrc w] > > +set srcfd [open $srcfile r] > > + > > +while { [gets $srcfd line] != -1 } { > > + if { [string first "break-here" $line] != -1 } { > > + # Put a "printf" line before the "break-here" line. > > + puts $bkpsrcfd " printf (\"foo\\n\");" > > + } > > + puts $bkpsrcfd $line > > +} > > + > > +close $bkpsrcfd > > +close $srcfd > > +file rename -force $bkpsrc $srcfile > > +# Give it some time to perform the renaming. For some reason, TCL > > +# needs some time after "file rename" in order to properly rename the > > +# file. > > In case a system takes longer, should we loop and make sure the renamed file > really exists, as opposed to sleeping for just a second? I took a little look through the tcl source code, and I can see no indication that a rename would be performed as a background task, either as a new process or thread - and I'd find that really surprising if that was the approach taken. So whatever you are observing here, I don't think the problem is TCL, rather I suspect it's either an OS or file system issue. I'm not suggesting that you need to track down the cause of this issue, but I agree with Luis that we should avoid arbitrary short pauses. I think you could probably use gdb_get_line_number to solve this problem, something like this completely untested code: # In some cases it has been observed that the file-system doesn't # immediately reflect the rename. Here we wait for the file to # reflect the expected new contents. proc wait_for_rename {} { global srcfile for { set i 0 } { $i < 5 } { incr i } { if { ![catch { gdb_get_line_number \ "pattern only matching the new line" \ ${srcfile} }] } { return } sleep 1 } error "file failed to rename correctly" } Obviously you'll need to supply a suitable pattern to match the new line that you add. Hope that helps, Thanks, Andrew > > > +sleep 1 > > + > > +# Recompile the modified source. We use "gdb_compile" here instead of > > +# "prepare_for_testing" because we don't want to call "clean_restart". > > +if { [gdb_compile "${srcfile}" "${binfile}" executable {debug}] != "" } { > > + return -1 > > +} > > + > > +# Rerun the program. This should not only force GDB to reload the > > I'm guessing line 6 is the one with the "break-here" marker? In that case, > should we mention that instead of the line number? Unless the line number is > really important. > > > +# source cache, but also to break at line 6 again, which now has > > +# different contents. > > +gdb_test_multiple "run" "rerun program" { > > + -re {The program being debugged has been started already\.\r\nStart it from the beginning\? \(y or n\) $} { > > + set binregex [string_to_regexp $binfile] > > + gdb_test "y" "\\`$binregex\\' has changed; re-reading symbols\.\r\nStarting program: ${binregex}.*" \ > > + "rerun program" > > + } > > +} > > + > > +# Again, perform the listing and check that the line indeed has > > +# changed for GDB. > > +gdb_test "list" "${bp_line}\[ \t\]+printf \\(\"foo\\\\n\"\\);" \ > > + "verify that the source code is properly reloaded" > > > > Otherwise LGTM. Thanks!