New testcase for PR tui/25126 (staled source cache)

  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
  

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/
>>
>>