* Re: [PATCH] New testcase for PR tui/25126 (staled source cache)
2020-02-06 23:00 [PATCH] New testcase for PR tui/25126 (staled source cache) Sergio Durigan Junior
@ 2020-02-07 9:41 ` Luis Machado
2020-02-07 11:47 ` Andrew Burgess
2020-02-07 17:07 ` Sergio Durigan Junior
2020-02-10 19:09 ` [PATCH v2] " Sergio Durigan Junior
2020-02-10 20:02 ` Sergio Durigan Junior
2 siblings, 2 replies; 17+ messages in thread
From: Luis Machado @ 2020-02-07 9:41 UTC (permalink / raw)
To: Sergio Durigan Junior, GDB Patches
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!
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] New testcase for PR tui/25126 (staled source cache)
2020-02-07 9:41 ` Luis Machado
@ 2020-02-07 11:47 ` Andrew Burgess
2020-02-07 17:09 ` Sergio Durigan Junior
2020-02-07 17:07 ` Sergio Durigan Junior
1 sibling, 1 reply; 17+ messages in thread
From: Andrew Burgess @ 2020-02-07 11:47 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: Luis Machado, GDB Patches
* 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!
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] New testcase for PR tui/25126 (staled source cache)
2020-02-07 11:47 ` Andrew Burgess
@ 2020-02-07 17:09 ` Sergio Durigan Junior
2020-02-07 19:54 ` Sergio Durigan Junior
0 siblings, 1 reply; 17+ messages in thread
From: Sergio Durigan Junior @ 2020-02-07 17:09 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Luis Machado, GDB Patches
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/
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] New testcase for PR tui/25126 (staled source cache)
2020-02-07 17:09 ` Sergio Durigan Junior
@ 2020-02-07 19:54 ` Sergio Durigan Junior
2020-02-07 20:11 ` Sergio Durigan Junior
2020-02-07 20:18 ` Luis Machado
0 siblings, 2 replies; 17+ messages in thread
From: Sergio Durigan Junior @ 2020-02-07 19:54 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Luis Machado, GDB Patches
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,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] New testcase for PR tui/25126 (staled source cache)
2020-02-07 19:54 ` Sergio Durigan Junior
@ 2020-02-07 20:11 ` Sergio Durigan Junior
2020-02-08 0:27 ` Andrew Burgess
2020-02-07 20:18 ` Luis Machado
1 sibling, 1 reply; 17+ messages in thread
From: Sergio Durigan Junior @ 2020-02-07 20:11 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Luis Machado, GDB Patches
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,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] New testcase for PR tui/25126 (staled source cache)
2020-02-07 20:11 ` Sergio Durigan Junior
@ 2020-02-08 0:27 ` Andrew Burgess
0 siblings, 0 replies; 17+ messages in thread
From: Andrew Burgess @ 2020-02-08 0:27 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: Luis Machado, GDB Patches
* 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/
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] New testcase for PR tui/25126 (staled source cache)
2020-02-07 19:54 ` Sergio Durigan Junior
2020-02-07 20:11 ` Sergio Durigan Junior
@ 2020-02-07 20:18 ` Luis Machado
2020-02-08 9:42 ` Luis Machado
1 sibling, 1 reply; 17+ messages in thread
From: Luis Machado @ 2020-02-07 20:18 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: Andrew Burgess, GDB Patches
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/
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] New testcase for PR tui/25126 (staled source cache)
2020-02-07 20:18 ` Luis Machado
@ 2020-02-08 9:42 ` Luis Machado
0 siblings, 0 replies; 17+ messages in thread
From: Luis Machado @ 2020-02-08 9:42 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: Andrew Burgess, GDB Patches
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/
>>
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] New testcase for PR tui/25126 (staled source cache)
2020-02-07 9:41 ` Luis Machado
2020-02-07 11:47 ` Andrew Burgess
@ 2020-02-07 17:07 ` Sergio Durigan Junior
1 sibling, 0 replies; 17+ messages in thread
From: Sergio Durigan Junior @ 2020-02-07 17:07 UTC (permalink / raw)
To: Luis Machado; +Cc: GDB Patches
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,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] New testcase for PR tui/25126 (staled source cache)
2020-02-06 23:00 [PATCH] New testcase for PR tui/25126 (staled source cache) Sergio Durigan Junior
2020-02-07 9:41 ` Luis Machado
@ 2020-02-10 19:09 ` Sergio Durigan Junior
2020-02-10 19:33 ` Luis Machado
2020-02-10 20:02 ` Sergio Durigan Junior
2 siblings, 1 reply; 17+ messages in thread
From: Sergio Durigan Junior @ 2020-02-10 19:09 UTC (permalink / raw)
To: GDB Patches; +Cc: Luis Machado, Andrew Burgess, Sergio Durigan Junior
Changes since v1:
- Fix typo.
- Improve testcase description.
- Use 'gdb_assert'.
- Improve description of "sleep 1" statement.
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-10 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/>. */
+
+/* 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"
--
2.21.0
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] New testcase for PR tui/25126 (staled source cache)
2020-02-10 19:09 ` [PATCH v2] " Sergio Durigan Junior
@ 2020-02-10 19:33 ` Luis Machado
2020-02-10 20:03 ` Sergio Durigan Junior
0 siblings, 1 reply; 17+ messages in thread
From: Luis Machado @ 2020-02-10 19:33 UTC (permalink / raw)
To: Sergio Durigan Junior, GDB Patches; +Cc: Andrew Burgess
I think you attached v1 again, no?
On 2/10/20 4:09 PM, Sergio Durigan Junior wrote:
> Changes since v1:
>
> - Fix typo.
> - Improve testcase description.
> - Use 'gdb_assert'.
> - Improve description of "sleep 1" statement.
>
>
> 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-10 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/>. */
> +
> +/* 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"
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] New testcase for PR tui/25126 (staled source cache)
2020-02-06 23:00 [PATCH] New testcase for PR tui/25126 (staled source cache) Sergio Durigan Junior
2020-02-07 9:41 ` Luis Machado
2020-02-10 19:09 ` [PATCH v2] " Sergio Durigan Junior
@ 2020-02-10 20:02 ` Sergio Durigan Junior
2020-02-10 21:55 ` Luis Machado
2020-02-11 11:10 ` Andrew Burgess
2 siblings, 2 replies; 17+ messages in thread
From: Sergio Durigan Junior @ 2020-02-10 20:02 UTC (permalink / raw)
To: GDB Patches; +Cc: Luis Machado, Andrew Burgess, Sergio Durigan Junior
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-10 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 | 43 ++++++++
gdb/testsuite/gdb.base/cached-source-file.exp | 99 +++++++++++++++++++
2 files changed, 142 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..6f62c29799
--- /dev/null
+++ b/gdb/testsuite/gdb.base/cached-source-file.c
@@ -0,0 +1,43 @@
+/* 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 bug is about a regression that makes GDB not reload its source
+ code cache when the inferior's symbols are reloaded, which leads to
+ wrong backtraces/listings.
+
+ This bug is reproducible even without using the TUI.
+
+ 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..f6e5f081f0
--- /dev/null
+++ b/gdb/testsuite/gdb.base/cached-source-file.exp
@@ -0,0 +1,99 @@
+# 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.
+#
+# The bug is about a regression that makes GDB not reload its source
+# code cache when the inferior's symbols are reloaded, which leads to
+# wrong backtraces/listings.
+#
+# 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 it 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]
+
+gdb_assert { [runto "$srcfile:$bp_line"] } \
+ "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\"); /* new-marker */"
+ }
+ puts $bkpsrcfd $line
+}
+
+close $bkpsrcfd
+close $srcfd
+file rename -force -- $bkpsrc $srcfile
+# Here, we have to wait 1 second because of the way GDB keeps track to
+# check whether the binary has changed or not. GDB uses stat(2) and
+# currently checks 'st_mtime', whose precision is measured in
+# seconds. Since the whole file-copying/rename operation can take
+# less than 1 second, GDB can mistakenly assume that the binary is
+# still the same if we don't wait here.
+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 BP_LINE 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\"\\); /\\\* new-marker \\\*/.*" \
+ "verify that the source code is properly reloaded"
--
2.21.0
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] New testcase for PR tui/25126 (staled source cache)
2020-02-10 20:02 ` Sergio Durigan Junior
@ 2020-02-10 21:55 ` Luis Machado
2020-02-11 11:10 ` Andrew Burgess
1 sibling, 0 replies; 17+ messages in thread
From: Luis Machado @ 2020-02-10 21:55 UTC (permalink / raw)
To: Sergio Durigan Junior, GDB Patches; +Cc: Andrew Burgess
On 2/10/20 5:02 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-10 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 | 43 ++++++++
> gdb/testsuite/gdb.base/cached-source-file.exp | 99 +++++++++++++++++++
> 2 files changed, 142 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..6f62c29799
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/cached-source-file.c
> @@ -0,0 +1,43 @@
> +/* 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 bug is about a regression that makes GDB not reload its source
> + code cache when the inferior's symbols are reloaded, which leads to
> + wrong backtraces/listings.
> +
> + This bug is reproducible even without using the TUI.
> +
> + 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..f6e5f081f0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/cached-source-file.exp
> @@ -0,0 +1,99 @@
> +# 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.
> +#
> +# The bug is about a regression that makes GDB not reload its source
> +# code cache when the inferior's symbols are reloaded, which leads to
> +# wrong backtraces/listings.
> +#
> +# 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 it 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]
> +
> +gdb_assert { [runto "$srcfile:$bp_line"] } \
> + "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\"); /* new-marker */"
> + }
> + puts $bkpsrcfd $line
> +}
> +
> +close $bkpsrcfd
> +close $srcfd
> +file rename -force -- $bkpsrc $srcfile
> +# Here, we have to wait 1 second because of the way GDB keeps track to
> +# check whether the binary has changed or not. GDB uses stat(2) and
> +# currently checks 'st_mtime', whose precision is measured in
> +# seconds. Since the whole file-copying/rename operation can take
> +# less than 1 second, GDB can mistakenly assume that the binary is
> +# still the same if we don't wait here.
> +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 BP_LINE 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\"\\); /\\\* new-marker \\\*/.*" \
> + "verify that the source code is properly reloaded"
>
Thanks. I have no further comments on this one.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] New testcase for PR tui/25126 (staled source cache)
2020-02-10 20:02 ` Sergio Durigan Junior
2020-02-10 21:55 ` Luis Machado
@ 2020-02-11 11:10 ` Andrew Burgess
2020-02-11 16:38 ` Sergio Durigan Junior
1 sibling, 1 reply; 17+ messages in thread
From: Andrew Burgess @ 2020-02-11 11:10 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: GDB Patches, Luis Machado
* Sergio Durigan Junior <sergiodj@redhat.com> [2020-02-10 15:02:20 -0500]:
> 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-10 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 | 43 ++++++++
> gdb/testsuite/gdb.base/cached-source-file.exp | 99 +++++++++++++++++++
> 2 files changed, 142 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..6f62c29799
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/cached-source-file.c
> @@ -0,0 +1,43 @@
> +/* 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 bug is about a regression that makes GDB not reload its source
> + code cache when the inferior's symbols are reloaded, which leads to
> + wrong backtraces/listings.
> +
> + This bug is reproducible even without using the TUI.
> +
> + 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..f6e5f081f0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/cached-source-file.exp
> @@ -0,0 +1,99 @@
> +# 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.
> +#
> +# The bug is about a regression that makes GDB not reload its source
> +# code cache when the inferior's symbols are reloaded, which leads to
> +# wrong backtraces/listings.
> +#
> +# 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 it 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]
> +
> +gdb_assert { [runto "$srcfile:$bp_line"] } \
> + "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\"); /* new-marker */"
> + }
> + puts $bkpsrcfd $line
> +}
> +
> +close $bkpsrcfd
> +close $srcfd
> +file rename -force -- $bkpsrc $srcfile
> +# Here, we have to wait 1 second because of the way GDB keeps track to
> +# check whether the binary has changed or not. GDB uses stat(2) and
> +# currently checks 'st_mtime', whose precision is measured in
> +# seconds. Since the whole file-copying/rename operation can take
> +# less than 1 second, GDB can mistakenly assume that the binary is
> +# still the same if we don't wait here.
Maybe rewrite this comment as:
We have to wait 1 second because of the way GDB checks whether the
binary has changed or not. GDB uses stat(2) and currently checks
'st_mtime', whose precision is measured in seconds. Since the copy,
rename, and rebuild can take less than 1 second, GDB might mistakenly
assume that the binary is unchanged.
Otherwise this looks good to me and can be applied.
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
> +# source cache, but also to break at BP_LINE 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\"\\); /\\\* new-marker \\\*/.*" \
> + "verify that the source code is properly reloaded"
> --
> 2.21.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] New testcase for PR tui/25126 (staled source cache)
2020-02-11 11:10 ` Andrew Burgess
@ 2020-02-11 16:38 ` Sergio Durigan Junior
0 siblings, 0 replies; 17+ messages in thread
From: Sergio Durigan Junior @ 2020-02-11 16:38 UTC (permalink / raw)
To: Andrew Burgess; +Cc: GDB Patches, Luis Machado
On Tuesday, February 11 2020, Andrew Burgess wrote:
> * Sergio Durigan Junior <sergiodj@redhat.com> [2020-02-10 15:02:20 -0500]:
[...]
>> +close $bkpsrcfd
>> +close $srcfd
>> +file rename -force -- $bkpsrc $srcfile
>> +# Here, we have to wait 1 second because of the way GDB keeps track to
>> +# check whether the binary has changed or not. GDB uses stat(2) and
>> +# currently checks 'st_mtime', whose precision is measured in
>> +# seconds. Since the whole file-copying/rename operation can take
>> +# less than 1 second, GDB can mistakenly assume that the binary is
>> +# still the same if we don't wait here.
>
> Maybe rewrite this comment as:
>
> We have to wait 1 second because of the way GDB checks whether the
> binary has changed or not. GDB uses stat(2) and currently checks
> 'st_mtime', whose precision is measured in seconds. Since the copy,
> rename, and rebuild can take less than 1 second, GDB might mistakenly
> assume that the binary is unchanged.
>
> Otherwise this looks good to me and can be applied.
Thanks, I adopted your version of the text and pushed the patch.
f6be87130b5b327075a09c05e78532816f186995
Thank you and Luis for the reviews.
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
^ permalink raw reply [flat|nested] 17+ messages in thread