Re: [PATCH] New testcase for PR tui/25126 (staled source cache)

[Sergio Durigan Junior <sergiodj@redhat.com>]

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,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/