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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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!