[PATCH] New testcase for PR tui/25126 (staled source cache)
Andrew Burgess
andrew.burgess@embecosm.com
Fri Feb 7 11:47:00 GMT 2020
* 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!
More information about the Gdb-patches
mailing list