Message ID | 20200206225943.26709-1-sergiodj@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 115681 invoked by alias); 6 Feb 2020 23:00:45 -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 114090 invoked by uid 89); 6 Feb 2020 23:00:25 -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=sergiodj@redhat.com, U*sergiodj, sergiodjredhatcom, boards X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (207.211.31.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Feb 2020 23:00:04 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1581030002; 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; bh=JFgafBdDVplVOWn9V7PrMtkyTohPibRf8HR1c94P8RU=; b=Jog60GoQNgKiUy97HXL0JIAfOn2+ut1jKDQs08S2EG4n9f/i+DV3ZufzswqxpnJgy3phDr MtrSth2QTaQq/KGFi1wCrq0megx1EmHZMEk2BKTUra99ky8q9a6FYVNpIUEyYzFjzvl6Ea JINHx5sr9ECdZSjKOiujg03Pj0FkjsM= 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-377-gh3W939qO66K8UgCq6JH9w-1; Thu, 06 Feb 2020 17:59:58 -0500 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 95F8910CE783 for <gdb-patches@sourceware.org>; Thu, 6 Feb 2020 22:59:57 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id CE9A71001B05; Thu, 6 Feb 2020 22:59:54 +0000 (UTC) From: Sergio Durigan Junior <sergiodj@redhat.com> To: GDB Patches <gdb-patches@sourceware.org> Cc: Sergio Durigan Junior <sergiodj@redhat.com> Subject: [PATCH] New testcase for PR tui/25126 (staled source cache) Date: Thu, 6 Feb 2020 17:59:43 -0500 Message-Id: <20200206225943.26709-1-sergiodj@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes |
Commit Message
Sergio Durigan Junior
Feb. 6, 2020, 10:59 p.m. UTC
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 <sergiodj@redhat.com> 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
Comments
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 <sergiodj@redhat.com> > > 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 <http://www.gnu.org/licenses/>. */ > + 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 <stdio.h> > + > +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 <http://www.gnu.org/licenses/>. > + 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? > +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!
* Luis Machado <luis.machado@linaro.org> [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 <sergiodj@redhat.com> > > > > 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 <http://www.gnu.org/licenses/>. */ > > + > > 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 <stdio.h> > > + > > +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 <http://www.gnu.org/licenses/>. > > + > > 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!
On Friday, February 07 2020, Luis Machado wrote: > 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 <sergiodj@redhat.com> >> >> 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 <http://www.gnu.org/licenses/>. */ >> + > > 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. Thanks for the review. Sure, I can put a more descriptive comment here. >> +/* 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 <stdio.h> >> + >> +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 <http://www.gnu.org/licenses/>. >> + > > 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. TUI isn't being used. The reason why I chose to restrict the test to native boards is because of the native TCL commands I use below (to copy/rename/modify files). I.e., the test doesn't use the "remote_exec..." functions. >> +# 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..." Heh, I was trying to find this excerpt in the text above, but then noticed that you are reviewing things *below* your comments. Thanks, I'll fix this. >> +# 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. OK. >> +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? Yeah, that's a good idea. On a side note, it seems really strange to me that TCL needs this "extra time". rename (the syscall) is atomic, so I don't know why I am seeing this behaviour. It'd be great if you (or someone else) could remove the "sleep 1", run the test a few times and see if it still passes. >> +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. Ah, right. Will do that. >> +# 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! Thanks,
On Friday, February 07 2020, Andrew Burgess wrote: > * Luis Machado <luis.machado@linaro.org> [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 <sergiodj@redhat.com> >> > >> > 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 <http://www.gnu.org/licenses/>. */ >> > + >> >> 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 <stdio.h> >> > + >> > +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 <http://www.gnu.org/licenses/>. >> > + >> >> 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. 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,
On Friday, February 07 2020, I wrote: > On Friday, February 07 2020, Andrew Burgess wrote: >> 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. OK, after trying your code, I can say that the problem is not on TCL. wait_for_rename returns successfully, and I've checked that gdb_get_line_number returns the correct value for the line. So, for TCL, the rename succeeded. Here's an interesting thing: I put a gdb_interact after the second "run" command, and then did: (gdb) list 35 printf ("hello\n"); /* break-here */ (gdb) shell gdb. gdb.log gdb.sum (gdb) shell outputs/gdb.base/cached-source-file/cached-source-file foo hello See how, for GDB, the inferior doesn't have the 'printf ("foo\n");' line, but when I run it externally I can see "foo" being printed? This means that GCC compiled the correct file, but GDB did not load it again, somehow. I find it extremely interesting how putting a "sleep 1" after the rename magically solves this problem. I would be less intrigued if we had to put "sleep 1" after "gdb_compile", because then it would hint at some race condition happening with GCC and GDB (very unlikely, but easier to understand). I didn't want to, but I guess I'll have to keep investigating this. Unless you (or someone) have any other ideas. Thanks,
On Friday, February 07 2020, I wrote: > On Friday, February 07 2020, I wrote: > >> On Friday, February 07 2020, Andrew Burgess wrote: >>> 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. > > OK, after trying your code, I can say that the problem is not on TCL. > wait_for_rename returns successfully, and I've checked that > gdb_get_line_number returns the correct value for the line. So, for > TCL, the rename succeeded. > > Here's an interesting thing: I put a gdb_interact after the second "run" > command, and then did: > > (gdb) list > 35 printf ("hello\n"); /* break-here */ > (gdb) shell gdb. > gdb.log gdb.sum > (gdb) shell outputs/gdb.base/cached-source-file/cached-source-file > foo > hello > > See how, for GDB, the inferior doesn't have the 'printf ("foo\n");' > line, but when I run it externally I can see "foo" being printed? This > means that GCC compiled the correct file, but GDB did not load it again, > somehow. > > I find it extremely interesting how putting a "sleep 1" after the rename > magically solves this problem. I would be less intrigued if we had to > put "sleep 1" after "gdb_compile", because then it would hint at some > race condition happening with GCC and GDB (very unlikely, but easier to > understand). > > I didn't want to, but I guess I'll have to keep investigating this. > Unless you (or someone) have any other ideas. I think I found the issue. On symfile.c:reread_symbols, the check performed to see whether the new objfile being loaded is different than the previous one is based on calling 'stat' and checking 'st_mtime': ... new_modtime = new_statbuf.st_mtime; if (new_modtime != objfile->mtime) { printf_filtered (_("`%s' has changed; re-reading symbols.\n"), objfile_name (objfile)); ... According to stat(2), 'st_mtime' is actually 'st_mtim.tv_sec', which means the precision of this field is given in seconds. Since Linux 2.6 'st_mtim's precision is given in nanoseconds, but we still use the seconds field. Because the testing script runs so fast, it's really likely that the old and the new files will have the same 'st_mtime'. Here's the output of an 'fprintf' I put in the code: new_modtime = 1581105949, old_modtime = 1581105949 So yeah, we have a few options here: 1) For now, I think it's justifiable to use "sleep 1" in the code, to force 'st_mtime' to be different between the two files. 2) The GDB code could be modernized to use nanosecond precision, which should solve this problem. Thanks,
On Fri, Feb 7, 2020, 20:54 Sergio Durigan Junior <sergiodj@redhat.com> wrote: > On Friday, February 07 2020, I wrote: > > > On Friday, February 07 2020, Andrew Burgess wrote: > >> 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. > > OK, after trying your code, I can say that the problem is not on TCL. > wait_for_rename returns successfully, and I've checked that > gdb_get_line_number returns the correct value for the line. So, for > TCL, the rename succeeded. > > Here's an interesting thing: I put a gdb_interact after the second "run" > command, and then did: > > (gdb) list > 35 printf ("hello\n"); /* break-here */ > (gdb) shell gdb. > gdb.log gdb.sum > (gdb) shell outputs/gdb.base/cached-source-file/cached-source-file > foo > hello > > See how, for GDB, the inferior doesn't have the 'printf ("foo\n");' > line, but when I run it externally I can see "foo" being printed? This > means that GCC compiled the correct file, but GDB did not load it again, > somehow. > > I find it extremely interesting how putting a "sleep 1" after the rename > magically solves this problem. I would be less intrigued if we had to > put "sleep 1" after "gdb_compile", because then it would hint at some > race condition happening with GCC and GDB (very unlikely, but easier to > understand). > > I didn't want to, but I guess I'll have to keep investigating this. > Unless you (or someone) have any other ideas. > Wild guess... Is the operation of renaming and reloading back in gdb executing quickly enough that the bfd cache doesn't notice a change (lack of timestamp precision in the filesystem)? This is assuming the object file is the same between loads. > Thanks, > > -- > Sergio > GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 > Please send encrypted e-mail if possible > http://sergiodj.net/ > >
* Sergio Durigan Junior <sergiodj@redhat.com> [2020-02-07 15:11:46 -0500]: > On Friday, February 07 2020, I wrote: > > > On Friday, February 07 2020, I wrote: > > > >> On Friday, February 07 2020, Andrew Burgess wrote: > >>> 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. > > > > OK, after trying your code, I can say that the problem is not on TCL. > > wait_for_rename returns successfully, and I've checked that > > gdb_get_line_number returns the correct value for the line. So, for > > TCL, the rename succeeded. > > > > Here's an interesting thing: I put a gdb_interact after the second "run" > > command, and then did: > > > > (gdb) list > > 35 printf ("hello\n"); /* break-here */ > > (gdb) shell gdb. > > gdb.log gdb.sum > > (gdb) shell outputs/gdb.base/cached-source-file/cached-source-file > > foo > > hello > > > > See how, for GDB, the inferior doesn't have the 'printf ("foo\n");' > > line, but when I run it externally I can see "foo" being printed? This > > means that GCC compiled the correct file, but GDB did not load it again, > > somehow. > > > > I find it extremely interesting how putting a "sleep 1" after the rename > > magically solves this problem. I would be less intrigued if we had to > > put "sleep 1" after "gdb_compile", because then it would hint at some > > race condition happening with GCC and GDB (very unlikely, but easier to > > understand). > > > > I didn't want to, but I guess I'll have to keep investigating this. > > Unless you (or someone) have any other ideas. > > I think I found the issue. On symfile.c:reread_symbols, the check > performed to see whether the new objfile being loaded is different than > the previous one is based on calling 'stat' and checking 'st_mtime': > > ... > new_modtime = new_statbuf.st_mtime; > if (new_modtime != objfile->mtime) > { > printf_filtered (_("`%s' has changed; re-reading symbols.\n"), > objfile_name (objfile)); > ... > > According to stat(2), 'st_mtime' is actually 'st_mtim.tv_sec', which > means the precision of this field is given in seconds. Since Linux 2.6 > 'st_mtim's precision is given in nanoseconds, but we still use the > seconds field. > > Because the testing script runs so fast, it's really likely that the old > and the new files will have the same 'st_mtime'. Here's the output of > an 'fprintf' I put in the code: > > new_modtime = 1581105949, old_modtime = 1581105949 > > So yeah, we have a few options here: > > 1) For now, I think it's justifiable to use "sleep 1" in the code, to > force 'st_mtime' to be different between the two files. I think using sleep 1 is fine in this case, as the comment will now make it clear that it's not an arbitrary delay, but a specific _minimum_ delay to ensure the timestamp ticks over. > > 2) The GDB code could be modernized to use nanosecond precision, which > should solve this problem. Only if you want extra credit :) Thanks for taking the time (there must be a pun here somewhere) to investigate this. Thanks, Andrew > > Thanks, > > -- > Sergio > GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 > Please send encrypted e-mail if possible > http://sergiodj.net/ >
On Fri, 7 Feb 2020 at 17:18, Luis Machado <luis.machado@linaro.org> wrote: > > > On Fri, Feb 7, 2020, 20:54 Sergio Durigan Junior <sergiodj@redhat.com> > wrote: > >> On Friday, February 07 2020, I wrote: >> >> > On Friday, February 07 2020, Andrew Burgess wrote: >> >> 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. >> >> OK, after trying your code, I can say that the problem is not on TCL. >> wait_for_rename returns successfully, and I've checked that >> gdb_get_line_number returns the correct value for the line. So, for >> TCL, the rename succeeded. >> >> Here's an interesting thing: I put a gdb_interact after the second "run" >> command, and then did: >> >> (gdb) list >> 35 printf ("hello\n"); /* break-here */ >> (gdb) shell gdb. >> gdb.log gdb.sum >> (gdb) shell outputs/gdb.base/cached-source-file/cached-source-file >> foo >> hello >> >> See how, for GDB, the inferior doesn't have the 'printf ("foo\n");' >> line, but when I run it externally I can see "foo" being printed? This >> means that GCC compiled the correct file, but GDB did not load it again, >> somehow. >> >> I find it extremely interesting how putting a "sleep 1" after the rename >> magically solves this problem. I would be less intrigued if we had to >> put "sleep 1" after "gdb_compile", because then it would hint at some >> race condition happening with GCC and GDB (very unlikely, but easier to >> understand). >> >> I didn't want to, but I guess I'll have to keep investigating this. >> Unless you (or someone) have any other ideas. >> > > Wild guess... Is the operation of renaming and reloading back in gdb > executing quickly enough that the bfd cache doesn't notice a change (lack > of timestamp precision in the filesystem)? This is assuming the object file > is the same between loads. > Heh... Nevermind. I noticed you found out something similar a few minutes before i replied. I ran into this a while ago with GCC's testsuite. It uses repeated names for some files and sometimes the object files are the same, so things may not get updated properly in GDB. > > >> Thanks, >> >> -- >> Sergio >> GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 >> Please send encrypted e-mail if possible >> http://sergiodj.net/ >> >>
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 <http://www.gnu.org/licenses/>. */ + +/* 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 <stdio.h> + +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 <http://www.gnu.org/licenses/>. + +# Test for PR tui/25126. +# 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 +# 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] + +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. +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 +# 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"