From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38386 invoked by alias); 7 Feb 2020 17:09:08 -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 38372 invoked by uid 89); 7 Feb 2020 17:09:07 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.5 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 autolearn=ham version=3.3.1 spammy= X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-2.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (205.139.110.61) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Feb 2020 17:09:05 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1581095344; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wCJtXMSProiFII407OQnDUqWluixHww0olLWfsy62nw=; b=e6oZykf1zDZeDNc/fQmmQlS4oz7LANA86dDNkMFaF32DGsEAJhtJDI8Nloj2thIYMF1O75 4nqwA0zcf7humoJxa9267IzpAFbtt2MTVZQrKD3RMHPuILGS7wSbDJgU2PBMWA1t2hfRxV sezhiwpvw1eEblfa7Vv2o0aca6J0ewA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-290-2gy4rnkSMay7kN6N2KZ0LA-1; Fri, 07 Feb 2020 12:09:01 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AAF688018A1; Fri, 7 Feb 2020 17:09:00 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7CFBB87B1C; Fri, 7 Feb 2020 17:09:00 +0000 (UTC) From: Sergio Durigan Junior To: Andrew Burgess Cc: Luis Machado , GDB Patches Subject: Re: [PATCH] New testcase for PR tui/25126 (staled source cache) References: <20200206225943.26709-1-sergiodj@redhat.com> <4a3db909-8677-fb84-39cf-84495266fdd8@linaro.org> <20200207114712.GI4020@embecosm.com> Date: Fri, 07 Feb 2020 17:09:00 -0000 In-Reply-To: <20200207114712.GI4020@embecosm.com> (Andrew Burgess's message of "Fri, 7 Feb 2020 11:47:12 +0000") Message-ID: <87sgjm9vf7.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg00177.txt.bz2 On Friday, February 07 2020, Andrew Burgess wrote: > * 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. >> >=20 >> > 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. >> >=20 >> > 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. >> >=20 >> > gdb/testsuite/ChangeLog: >> > 2020-02-07 Sergio Durigan Junior >> >=20 >> > PR tui/25126 >> > * gdb.base/cached-source-file.c: New file. >> > * gdb.base/cached-source-file.exp: New file. >> >=20 >> > 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 >> >=20 >> > diff --git a/gdb/testsuite/gdb.base/cached-source-file.c b/gdb/testsui= te/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 modi= fy >> > + 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 . */ >> > + >>=20 >> 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? >>=20 >> Personally, i find it a bit more useful. >>=20 >> Then again, if it is a simple test and it will be obvious from code >> comments, it shouldn't be necessary. >>=20 >> > +/* 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/tests= uite/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 . >> > + >>=20 >> Same as above about explaining what the test wants to achieve. >>=20 >> > +# Test for PR tui/25126. >>=20 >> Would it make the test run fine with remote stubs if the TUI wasn't used? >> Just wondering. >>=20 >> > +# This bug is reproducible even without using the TUI. >> > + >> > +standard_testfile >> > + >> > +# Only run on native boards. >> > +if { [use_gdb_stub] || [target_info gdb_protocol] =3D=3D "extended-re= mote" } { >> > + return -1 >> > +} >> > + >> > +# Because we need to modify the source file later, it's better if we >>=20 >> Typo... "just copy it to our..." >>=20 >> > +# 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] >> > + >>=20 >> I've learned about it recently, but i think using gdb_assert here would = be >> nice instead of the if/else block. >>=20 >> > +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\"\\); /\\* br= eak-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] !=3D -1 } { >> > + if { [string first "break-here" $line] !=3D -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. >>=20 >> 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. Thanks for taking a look. > 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. Could you perhaps run the testcase without the "sleep 1" and see if the test still passes for you? In my experience here, without the sleep the test would fail almost always. > 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" > } Ah, cool. I'll adjust that to the code. Thank you. > Obviously you'll need to supply a suitable pattern to match the new > line that you add. > > Hope that helps, Thanks, --=20 Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/