diff mbox

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

Message ID 20200210200221.14846-1-sergiodj@redhat.com
State New
Headers show

Commit Message

Sergio Durigan Junior Feb. 10, 2020, 8:02 p.m. UTC
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

Comments

Luis Machado Feb. 10, 2020, 9:55 p.m. UTC | #1
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.
Andrew Burgess Feb. 11, 2020, 11:10 a.m. UTC | #2
* 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
>
Sergio Durigan Junior Feb. 11, 2020, 4:37 p.m. UTC | #3
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.
diff mbox

Patch

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"